[2단계 - 자동차 경주 리팩터링] 제이미(임정수) 미션 제출합니다. #611

JJ503 · 2023-02-13T01:28:52Z

안녕하세요 파즈!
지난 1단계 리뷰들 감사합니다.
해당 리뷰들 적용 후 말씀해 주신 추가 요구사항에 대한 구현도 진행했습니다.

리뷰에 따라 2단계에서 집중한 부분은 아래와 같습니다.

메서드 네이밍
- 의도와 목적이 명확한 네이밍을 짓도록 함
추가 요구사항 구현
1. pobi,pobi,pobi 의 입력이 현재로서는 가능하지만, 동일 이름일 시에는 불가능하도록 하기
2. pobi, crong, honux 입력 시에 앞뒤 공백은 제외하고 이름을 측정할 수 있도록 하기

혹시 제가 추가적으로 생각해 보거나 수정 및 추가하면 좋을 문제가 있다면 말씀해 주시면 감사하겠습니다.
이번 리뷰도 잘 부탁드립니다!

궁금한 점

getter 메서드 사용

getter 메서드를 통해 List를 반환할 시 Collections.unmodifiableList(cars)를 통해 처리해 주었습니다.
불변 객체를 위해 위와 같이 처리한 의도와는 다르게, List는 수정할 수 없지만 List에 담겨있는 Car 객체에 대한 수정은 가능했습니다.
이러한 경우 Car 객체에 대한 수정도 막기 위해 List.copyOf()를 사용하는 것이 더 좋을까요?
Car 객체의 변수인 name과 location은 List 타입의 변수와는 다르게 Collections.unmodifiableList()를 사용할 수 없습니다.
이러한 경우 name과 location 변수에 대한 getter 메서드에서는 별다른 작업을 해주지 않아도 괜찮은지 궁금합니다.
혹은 이러한 이유 때문에 getter를 최대한 지양해야 하는 걸까요?

moveResult()를 moveForRound()로 수정함 result가 붙으면 result가 반환되어야 할 것 같지만, void 메서드이므로 수정이 필요해 변경함

repeatMoving()를 movePerRounds()로 수정함 void 메서드로는 어울리지 않는 메서드명으로 수정이 필요해 변경함

상수 변수들에는 static final 필드를 설정함

자동차 이름 중 중복이 있는 경우에 대한 예외 메시지 추가

IllegalArgumentException가 발생할 수 있도록 수정

- 빈 값을 입력한 경우에 대한 예외 발생 테스트 - 띄어쓰기 값만 입력한 경우에 대한 예외 발생 테스트 - 시도 횟수에 따른 예외 발생 입력 테스트

Cars에서 출력문 반환하는 대신, 우승자 목록만 반환한 후 OutputView에서 출력 형태로 변환줌

기존에는 우승자 이름 목록을 문자열로 반환했는데, 수정사항이 생기며 리스트 형태로 반환되기에 이에 맞게 수정함

Be-poz

안녕하세요 제이미 미션 수고하셨습니다. 1단계에서 사실 다 해두셔서 2단계는 크게 리뷰할게 없네요~!
이외 여러가지 디테일 신경써주신 부분도 잘해주신 것 같습니다. 요구사항 하나 더 추가해봤는데 한 번 적용해보시면 좋을 것 같습니다.

List.copyOf() 사용해야 되나요?
-> 사용하실 필요 없을 것 같습니다. getter 를 통해서 해당 필드를 가져오는 역할로 충분한 것 같습니다. 이 getter가 List에 담겨있는 객체까지 불변을 막겠다는거는 본인 역할 이외인 것 같습니다. Car의 수정을 막고 싶다면 Car 객체 내부 필드들을 불변하게끔 만드는 것이 맞다고 봅니다.([1단계 - 자동차 경주 구현] 제이미(임정수) 미션 제출합니다. #498 (comment) 이 피드백을 통해서 수정되었으니 상관없을 것 같습니다)
name과 location 변수에 대한 getter 메서드에서는 별다른 작업을 해주지 않아도 괜찮은지 궁금합니다.
-> list를 getter 호출 시에 왜 Collection.modifiableList() 로 반환해줬었죠? 그리고 name, location getter 호출 시에는 왜 별도의 작업을 해주어야 한다고 생각하셨을까요? 어떤 점이 우려됐던건지 한 번 생각해보시고 그걸 실험해보시면 답을 찾으실 수 있을 것 같습니다.

Be-poz · 2023-02-13T11:29:18Z

src/main/java/controller/Controller.java

@@ -17,7 +17,7 @@ public class Controller {

 public void runGame() {
 Cars cars = setCars();


setCars 말고 다른 이름으로 해볼까요? 의미는 맞긴한데 cars라는 필드를 가지고 있다고 오해할 수도 있을 것 같습니다.

그럴 수 있겠군요...!
initCarData()로 수정해 보았는데 어떨까요?

Be-poz · 2023-02-13T11:36:00Z

src/main/java/validation/Validation.java

@@ -3,16 +3,16 @@
 import constant.ExceptionMessage;

 public class Validation {
- private final int MIN_TRY_COUNT_INPUT = 1;
+ private static final int MIN_TRY_COUNT_INPUT = 1;

 public void validateCarNames(String carNames) throws IllegalAccessException {


throws 절 삭제 고고~

바로 삭제했습니다!!👍

Be-poz · 2023-02-13T11:39:15Z

src/main/java/domain/Car.java

+ private static final int CAR_LOCATION_INITIALIZATION = 0;
+ private static final int MAX_CAR_NAME_LENGTH = 5;
+ private static final int CAR_MOVE_FORWARD = 1;
+ private static final int MIN_NUMBER_FOR_CAR_MOVE = 4;

 private final String name;


다른 부분들을 너무 잘해주셔가지고 크게 터치할 것들이 없는 것 같습니다.
name을 객체로 만들어서 관련 validation들과 로직, validDuplication 메서드도 name 객체를 이용해서 비교하게끔 변경해보면 좋을 것 같습니다.

Name 도메인 클래스를 만들면서 validDuplication()에 대한 기능을 Name 객체에게 부담하도록 해보았습니다.
말씀해 주신 내용이 맞는지 확인해 주시면 감사하겠습니다!
그런데 제가 생각한 코드는 Cars에서 중복인지 확인해 달라고 요청을 보냈을 때, Name에서 동일한지 확인하지만 Car을 통해 연결되는 상태입니다.
해당 코드가 최선인지 궁금합니다..!
또한, Car에서 이름을 가져오는 getter 메서드가 괜찮은지 궁금합니다.

넵 검증과 같은 내용들을 Name 객체한테 주도록 의도한 것 맞습니다 !
Car에서 이름 가져오는 getter 메서드 getName 인데 괜찮습니다!

public String getName() { return this.name.getName(); }

의 상태인데 Name의 toString 을 오버라이드해서 Name 객체의 name 필드를 리턴하게끔 하고 위 구현에서 그냥 바로 this.name을 리턴하게끔 해도 괜찮을 것 같긴합니다.

중복 확인은 현재 Name을 통해서 하게끔 되어있는데, Car 객체의 equals, hashcode 를 오버라이드 해서 비교자체를 Name 을 통해 비교를 하게끔하고, 입력받은 자동차 이름으로 Car들을 추가할 때에 기존에 들어가있는 Car들과 equal 한 Car가 아니면 List에 add , 그렇지 않으면 중복이름 exception throw 이렇게 구현할 수도 있을 것 같네요.

equals를 오버라이드 해 사용하기 위해 조사하며, hashcode의 역할이 궁금했는데 좀 더 찾아봐야겠네요!
좀 더 찾아본 뒤 hashcode를 통해 비교할 수 있도록 수정해 보도록 하겠습니다!

비교자체는 equals만 오버라이드 해도 상관없습니다. 하지만 equals가 다르다고해서 hashcode 까지 다른건 아니기 때문에 같이 재정의를 해주는 것입니다. 해시충돌이 일어날 수도 있거든요. hashcode 를 재정의 하는 이유를 한 번 찾아보시면 좋을 것 같습니다.
객체의 hashcode는 .hashCode() 메서드를 이용하면 확인하실 수 있습니다. 해시 충돌은 해시맵을 이용할 때에 굉장히 중요합니다. 찾아보시면 바로 나올겁니다. 화이튕

Be-poz · 2023-02-13T11:42:26Z

src/main/java/domain/Cars.java

 return cars.stream()
 .filter(car -> car.isLocationEqual(getMaxLocation()))
 .map(Car::getName)
- .collect(Collectors.joining(COMMA + BLANK));
+ .collect(Collectors.toList());
 }

 public int getMaxLocation() {


optional return 일 때에는 get으로 바로 받지말고 orElse, orElseThrow로 잡아주면 좋을 것 같습니다.

orElseThrow를 통해 만약 max 값이 없다면 예외를 발생시키도록 수정했습니다.
이 의도가 맞을까요??

또한, max 값을 가져올 때 오류가 발생하는 경우가 언제가 있을까요?
cars의 경우 객체가 하나라도 있어야 하기에 Null인 경우는 없을 것이고 max 값은 0이어도 상관이 없기에 null이 존재할지 모르겠습니다.
만약 오류가 발생하는 경우가 없더라도 이러한 예외처리를 항상 해주는 것이 좋은지 궁금합니다.

null 인 경우에 예외가 발생하는 것이 맞습니다.
논리적으로는 사실 오류가 발생할 일이 없긴합니다만, 잡아줄 수 있으면 잡아주는 것이 만일의 사태를 대비한다고 생각합니다.

이해했습니다!
확실히 처리해 주면 예상치 못한 이슈에 대한 처리가 가능해지겠네요.
감사합니다!

Be-poz · 2023-02-13T11:45:54Z

src/main/java/domain/Cars.java

+ public List<String> getWinners() {
 return cars.stream()
 .filter(car -> car.isLocationEqual(getMaxLocation()))
 .map(Car::getName)
- .collect(Collectors.joining(COMMA + BLANK));
+ .collect(Collectors.toList());


Be-poz · 2023-02-13T11:50:39Z

src/test/java/domain/CarTest.java

- @ParameterizedTest
- @ValueSource(strings = {"pobi", "crong", "honux", "hi hi", "jj503"})
- @DisplayName("Car 객체 생성 테스트")
+ @ParameterizedTest(name = "Car 객체 생성 테스트 name = {0}")


우승자 선정에 있어 문제가 생겼을 때에 대한 예외 추가

setCars를 initCarData로 수정 cars라는 필드를 가지고 있다 오해할 수도 있기 때문

JJ503 · 2023-02-14T05:58:45Z

안녕하세요 파즈!
2단계 미션 리뷰도 감사합니다.
리뷰에 대해 수행해 보며 궁금한 점들을 답변으로 달아두었습니다.
시간이 괜찮으실 때 천천히 답변해 주시면 정말 감사하겠습니다!

그리고 제가 질문드렸던 getter 메서드에서 우려된 사항을 고민해 보았습니다.
고민해 본 내용과 궁금한 점은 아래와 같습니다.

생각을 해보니 제가 고민한 부분은 Car 객체의 필드의 불변성보단, Car의 메서드에 대한 접근이 어디서든 가능하다는 점인 것 같습니다.
제 OutputView의 printResult()를 확인해 보면 파라미터로 List가 타입인 Cars를 받는 것을 확인할 수 있습니다.
예를 들어 printResult()에서 Car 메서드인 moveByNumber()에 접근이 가능한 경우를 생각하고 있습니다.
이렇게 되었을 때 OutputView에선 Car 객체에 접근이 가능하게 되는데 이는 크게 문제 되지 않을까요? (굳이 printResult()에서 moveByNumber()를 수행할 일은 없기에…?)
혹은 OutputView에선 Car 객체에 접근이 되지 않도록 이름에 대한 리스트와 현재 위치에 대한 값 리스트에 대해서만 파라미터로 전달해주었어야 했을까요?
위와 같은 부분에서 고민이 되는 상태입니다.

Be-poz · 2023-02-14T15:27:16Z

그런 고민이셨군요! 저는 getter를 통해서 location과 name을 가져와서 값 변경을 했을 때의 문제를 우려하시는줄 알았네요!
moveByNumber가 view에서 호출이 될 수 있는 문제점을 걱정하셨는데 아주 타당한 걱정이십니다. 사실 구현오류이기도 하지만, 코드 오류로 인해 충분히 우려하실 수 있습니다.

public으로 두었기 때문에 필요없는 곳에서 메서드를 볼 수 있고 호출할 수 있는건 당연합니다. getter 자체는 문제가 없기 때문에 이런 경우에는 dto를 통해서 다른 layer에 정보를 넘겨줍니다. 제이미의 경우에도 파라미터로 dto 를 사용해서 넘기면 해결이 될 문제일 것 같네요!

차차 배우시게 될겁니다!

Be-poz

안녕하세요 제이미~~ 여러 고민들을 가지고 계셔서 답하는 재미가 있었습니다.
충분히 고민이 들만한 포인트여서 아주 잘 짚고 넘어가신 것 같습니다.
추가로 질문 남겨주신 부분들에 대해서 코멘트를 달아놨습니다.
이번 미션은 이제 머지해도 괜찮을 것 같네요! 코멘트 추가로 달아주시면 계속 답변은 할 것이니 추가로 궁금한 점이 생기면 코멘트 달아주세요 수고하셨습니다.

JJ503 added 22 commits February 13, 2023 07:34

refactor: model 패키지 domain으로 변경

fe8ca0c

refactor: moveResult() 변수명 수정

c39797b

moveResult()를 moveForRound()로 수정함 result가 붙으면 result가 반환되어야 할 것 같지만, void 메서드이므로 수정이 필요해 변경함

refactor: repeatMoving() 변수명 수정

df428fb

repeatMoving()를 movePerRounds()로 수정함 void 메서드로는 어울리지 않는 메서드명으로 수정이 필요해 변경함

refactor: 상수 변수에 대한 필드 설정

7b7182d

상수 변수들에는 static final 필드를 설정함

feat(ExceptionMessage): 예외 메시지 추가

73e4430

자동차 이름 중 중복이 있는 경우에 대한 예외 메시지 추가

feat(Cars): 자동차 이름 중복 제거 기능 추가

698d845

feat(Car): 자동차 이름 저장 시 앞뒤 공백 제거 기능 추가

5a13e36

test(Car): 앞뒤 공백이 추가된 이름 저장에 대한 테스트 추가

15a599e

fix(Car): 앞뒤 공백 제거를 자동차 이름 검증 전에 수행 하도록 수정

709a584

fix(ExceptionMessage): 중복 예외 메시지 내용 수정

e111a22

test(Cars): Cars 객체 생성 시 중복으로 인한 예외 발생 테스트 추가

ecdc72b

fix(Cars): 중복 확인이 잘못된 문제 해결

b48fc53

refactor: 공백 컨벤션에 맞게 리팩토링

d0e946c

refactor: for문 컨벤션에 맞게 리팩토링

9941d4b

refactor: 테스트 argument 별 디스플레이명이 출력되도록 수정

e29ed69

fix(InputViewTest): 예외 발생 테스트가 메시지도 체크하도록 수정

a8ac31c

refactor: 사용하지 않는 import문 제거

bd1b17e

fix(Validation): 예외 발생 종류 수정

b11c7e6

IllegalArgumentException가 발생할 수 있도록 수정

feat(InputViewTest): 예외 발생 종류 추가

a171619

test(validation): validation에 대한 테스트 추가

a7d50d3

- 빈 값을 입력한 경우에 대한 예외 발생 테스트 - 띄어쓰기 값만 입력한 경우에 대한 예외 발생 테스트 - 시도 횟수에 따른 예외 발생 입력 테스트

refactor: 비즈니스 로직과 UI 로직의 분리하기

d46eee2

Cars에서 출력문 반환하는 대신, 우승자 목록만 반환한 후 OutputView에서 출력 형태로 변환줌

fix(CarsTest): getWinners()가 List를 반환하는 형태로 수정

fe42fe2

기존에는 우승자 이름 목록을 문자열로 반환했는데, 수정사항이 생기며 리스트 형태로 반환되기에 이에 맞게 수정함

Be-poz requested changes Feb 13, 2023

View reviewed changes

JJ503 added 7 commits February 14, 2023 00:11

refactor(Validation): 사용하지 않는 throws 절 제거

5a68ef6

feat(Name): Name을 도메인 객체로 생성

9ad6cb3

feat(Name): 이름 가져오기 기능 추가

311004d

fix: Name 도메인 객체 생성에 대한 수정 사항 해결

80ab1a0

feat(ExceptionMessage): 예외 메시지 추가

8626ee9

우승자 선정에 있어 문제가 생겼을 때에 대한 예외 추가

feat(Car): Name 객체를 통해 값을 비교해주는 기능 추가

9e57794

refactor(Cars): Name 객체를 통해 중복 값이 있는지 확인하도록 수정

8041011

JJ503 added 2 commits February 14, 2023 11:31

refactor(Name): 구분을 위해 객체 변수에 this 붙임

c0d9e87

refactor(Controller): 변수명 수정

4e725ca

setCars를 initCarData로 수정 cars라는 필드를 가지고 있다 오해할 수도 있기 때문

Be-poz approved these changes Feb 14, 2023

View reviewed changes

Be-poz merged commit 44df2f6 into woowacourse:jj503 Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 자동차 경주 리팩터링] 제이미(임정수) 미션 제출합니다. #611

[2단계 - 자동차 경주 리팩터링] 제이미(임정수) 미션 제출합니다. #611

JJ503 commented Feb 13, 2023 •

edited

Loading

Be-poz left a comment

Be-poz Feb 13, 2023

JJ503 Feb 14, 2023

Be-poz Feb 13, 2023

JJ503 Feb 14, 2023

Be-poz Feb 13, 2023

JJ503 Feb 14, 2023 •

edited

Loading

Be-poz Feb 14, 2023

JJ503 Feb 16, 2023

Be-poz Feb 16, 2023

Be-poz Feb 13, 2023

JJ503 Feb 14, 2023

Be-poz Feb 14, 2023

JJ503 Feb 16, 2023

Be-poz Feb 13, 2023

Be-poz Feb 13, 2023

JJ503 commented Feb 14, 2023

Be-poz commented Feb 14, 2023

Be-poz left a comment

		@@ -17,7 +17,7 @@ public class Controller {

		public void runGame() {
		Cars cars = setCars();

[2단계 - 자동차 경주 리팩터링] 제이미(임정수) 미션 제출합니다. #611

[2단계 - 자동차 경주 리팩터링] 제이미(임정수) 미션 제출합니다. #611

Conversation

JJ503 commented Feb 13, 2023 • edited Loading

궁금한 점

getter 메서드 사용

Be-poz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JJ503 Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JJ503 commented Feb 14, 2023

Be-poz commented Feb 14, 2023

Be-poz left a comment

Choose a reason for hiding this comment

JJ503 commented Feb 13, 2023 •

edited

Loading

JJ503 Feb 14, 2023 •

edited

Loading