[2단계 - 자동차 경주 리팩터링] 더즈(이동규) 미션 제출합니다. #357

ldk980130 · 2022-02-16T07:37:29Z

안녕하세요! 2단계 피드백도 잘 부탁드립니다.

다음은 제가 리펙토링 때 고려한 사항들입니다.

Repository에서 DTO 사용 제거
우승자를 구하기 위한 로직에서 getPosition()을 사용하지 않기 위해 Car -> CarDto -> getPosition()의 로직을 사용했었습니다. 이러면 DTO를 잘못 사용하고 있는것 같기도 하고 데이터 전달이 목적인 DTO가 Repository에서 쓰이는게 마음에 들지 않았습니다. 그래서 Car 객체가 알아서 position에 의해 정렬되도록 comepareTo()를 오버라이드하여 Repository에서 가장 position이 큰 Car를 가져오는 방식으로 우승자를 구현하였습니다.

컨트롤러에서 수행되던 비즈니스 로직 서비스로 이동
시도 횟수를 반복할 때마다 뷰를 출력해야 했기에 시도횟수만큼 반복하는 로직이 원래 컨트롤러에 있었습니다. 토니는 그것도 괜찮을것 같다고 했지만 고민한 결과 시도 횟수마다 자동차의 위치 결과를 RacingResult에 모아놨다가 한 번에 뷰로 출력하도록 리펙터링 했습니다.

@DisplayName 사용
테스트 코드에서 메서드명은 영어로 하고 DisplayName을 사용하도록 변경했습니다.

난수 생성 인터페이스인 RandomUtil -> MovingStrategy 이름 변경
Car 클래스의 move() 함수가 움직이기 위해 받는 정수는 사실 랜덤이 아니여도 되고 랜덤한 수 말고도 다른 방법으로더 정수가 생성되어 들어올 수도 있다고 생각이 들어 다음과 같이 이름을 변경했었습니다. 구현체였던 RandomUtilImpl도 RandomMovingStrategy로 변경하였습니다.

상수, 변수 선언 순서 변경
컨벤션에서 제가 착각한 것이 있었습니다. 클래스에서 상수가 제일 상단에 오도록 변경했습니다.

자동차 이름 중복 검사 로직 추가
자동차 이름이 같으면 예외를 발생시키는 기능을 추가하였습니다.

toneyparky

빠르게 2단계 미션을 처리해주셨네요 더즈!!

1단계 때에 MVC 패턴을 잘 적용해주셔서 구조적으로 큰 수정은 없었네요 !!

제가 1단계에 남겨드린 피드백 이상으로 고민하시고 이번 단계에 반영하신 점이 인상깊었습니다 👍👍

코드는 머지해도 충분하지만 더즈와 이야기를 더 나누고 싶기에(?) 질문을 몇 개 더 남겨봤어요.

충분히 고민해보시고 수정사항 반영 및 답변주시면 학습에 도움이 될거에요!

화이팅입니다!

toneyparky · 2022-02-16T14:56:35Z

src/main/java/racingcar/domain/RacingResult.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class RacingResult {


요 녀석이 반복 로직을 서비스로 가져오면서 추가된 객체군요.

제 의견 이상으로 더즈가 구현하고 싶은 생각을 펼쳐나간 게 좋네요 😀

원래 고민하고 있던 것도 맞지만 페어의 리뷰어가 비슷한 질문에 '그럼 데이터를 모아놨다가 한 번에 출력하는건 어떨까'하고 피드백 주셨다고 해서 한 번 적용해보았습니다!

오오 좋네요! 페어와 꾸준히 소통하시는게 멋집니다!

저도 우테코를 했는데요, 개발도 정말 즐거웠지만 페어나 다른 크루들과 함께하는 시간이 소중했어요!

더즈도 남은 우테코 기간을 많은 크루들과 소통하며 보내시길 바라요 !

toneyparky · 2022-02-16T22:04:13Z

src/main/java/racingcar/util/MovingStrategy.java

+public interface MovingStrategy {
+
+ int generate();
+}


요 부분 수정하신 것도 좋아요. 자동차의 이동이라는 비즈니스 로직/전략을 랜덤에만 국한하지 않고 다양한 이름의 전략을 구현할 수 있네요!

적절한 추상화 👍

정말 역할에 필요한 이름인지 고심해서 지어야겠다고 생각했습니다. 이래서 이름 짓기가 힘들다고 하는 거군요ㅋㅋㅋㅋ

맞아요.

일종의 예술. 가끔은 고통스러운(?)

toneyparky · 2022-02-16T22:05:26Z

src/test/java/racingcar/CarTest.java

+ @DisplayName("자동차 생성 position 음수")
+ public void createCarPositionWithNegative() {


더즈가 @DisplayName을 적용하시면서 기존의 한글 이름 메서드와 비교하여 느낀점이 있을까요?

메서드명이 한글 지원이 되는데 @DisplayName을 적용한 이유가 TDD에 대한 강의가 떠올라서 였습니다. 한글로 적어두면 Non-ASCII characters in an identifier warn이 발생하는데 실무에서는 이런 warn도 다확인한다고 들어서 이렇게 바꿔보았습니다.

맞아요, 메서드 이름을 한글로 적으면 워닝이 뜨죠!

일반적으로도 수정해주신 것과 같은 패턴으로 많이 사용해요.

다만 여기서 메서드 이름의 컨벤션은 조금씩 달라지구요.

머지되고 시간이 남을테니 테스트 메서드의 이름에 대해서 조사해보셔도 좋겠네요!

toneyparky · 2022-02-16T22:14:21Z

src/test/java/racingcar/RacingServiceTest.java

+ @DisplayName("자동차 저장")
+ public void saveCar() {
 int carCount = carRepository.count();

 assertThat(carCount).isEqualTo(2);
 }

 @Test
- public void 자동차_경주() {
- racingService.race(bound -> 5);
- racingService.race(bound -> 5);
+ @DisplayName("자동차 이름 중복")
+ public void duplicateCarName() {
+ assertThatThrownBy(() -> carRepository.addCar(Car.from("pobi")))
+ .isInstanceOf(IllegalStateException.class);
+ }


자동차 저장과 자동차 이름 중복 테스트에서는 어떤 객체가 사용되나요? RacingService일까요? RacingRepository일까요?

테스트하고자 하는 객체를 명확히 판별하고 해당 객체의 테스트 클래스에 테스트 코드가 작성되면 코드를 읽는 사람이 더 파악하기 쉽겠죠???

추가로 자동차 저장과 자동차 이름 중복이라는 이름은 코드를 확인하지 않고는 맥락을 파악하기 어려워요 😭

어떠한 상황에서 어떤 행위를 했을 때 어떤 결과가 나왔다. 와 같이 이름이 길어지더라도 읽는이가 맥락을 파악하기 쉽도록 수정해봐도 좋겠네요!

CarRepository가 해야할 일은 CarRepositoryTest에서 테스트하도록 바꾸었고 테스트 이름도 좀 더 명확히 수정해보았습니다!

좋습니다. 추후에도 테스트 이름은 보는 사람이 코드를 다 읽지 않아도 파악할 수 있게 적어주시면 좋아요!

toneyparky · 2022-02-16T22:17:22Z

src/main/java/racingcar/service/RacingService.java

- private final CarRepository carRepository = CarRepository.getInstance();
-
- private static final int RANDOM_VALUE_RANGE = 10;
 private static final int MINIMUM_NUMBER_OF_RACE_POSSIBLE = 2;
+ private static final int MINIMUM_ATTEMPT_NUMBER = 1;

 private static final String NUMBER_OF_CAR_ERROR_MESSAGE = "레이싱에 필요한 자동차 수는 2대 이상입니다.";
+ private static final String ATTEMPT_NUMBER_RANGE_ERROR_MESSAGE = "시도 횟수는 1회 이상이어야 합니다.";
+
+ private final CarRepository carRepository = CarRepository.getInstance();
+ private final MovingStrategy movingStrategy;


선언 순서에 대한 꼼꼼한 확인 좋습니다! 적절한 접근 제어자와 final도 인상 깊네요😀👍

toneyparky · 2022-02-16T22:19:52Z

src/main/java/racingcar/domain/Car.java

@@ -1,9 +1,8 @@
 package racingcar.domain;

-public class Car {
+import java.util.Objects;


2 단계 리팩터링 요구사항인, 도메인 패키지의 객체는 뷰 패키지의 객체에 의존하지 않는다를 잘 지켜주셨네요!

toneyparky · 2022-02-16T22:22:57Z

src/main/java/racingcar/service/RacingService.java

+ for (int i = 0; i < attemptNumber; i++) {
+ cars.forEach(car -> car.move(movingStrategy.generate()));
+ racingResult.addRecord(findCarDtos());
+ }


모든 원시 값을 포장하라!

만일 attemptNumber를 객체로 만든다면 어떤 장점이 있을까요??? 그리고 어떤 단점이 있을까요???

더즈의 생각을 듣고 싶어요! 자유롭게 써주세요 😬

직접 한 번 해 보았습니다. AttemptNumber 객체를 만들어 int 대신 사용해보았습니다. 우선 제가 느낀바로 장점은 시도 횟수만의 로직(유효성 검사 등)을 AttemptNumber 안에서 가지고 있어서 이 객체를 사용하는 쪽에서는 추가적인 검증이 필요가 없었습니다. 그리고 그저 int 값이었을 때에 비해 이게 무엇을 하는 값인지(클래스인지) 명확하게 와닿았습니다.

단점은 너무 간단한 값들은 일일이 다 포장하면 할 일이 많아진다는 것 정도...? 정말 복잡한 의미나 개념을 갖고 있는 원시 값에는 필수적으로 적용해야겠다는 느낌이 딱 오긴 왔어요. (형식이 있는 문자열이나 단위가 정해진 숫자 등등)

여기서 궁금한 점이 Car의 name 변수도 유효성 검사를 몇 가지 해야해서 포장하면 좋긴할텐데 이름이라는 개념은 Car 말고도 여기저기서 많이 쓰일 수도 있다고 생각해요. 그럼 각 객체마다 name의 검증 내용이 달라질 수도 있는데 이런 경우는 어떻게 해결하나요? 추상 클래스로 만들어서 이를 상속받은 여러 XXXName으로 만들어야 할까요?

느낌을 받으셨다니 좋네요! 질문 드린 보람(?)이 있슴니다 ㅋㅋㅋ

말씀주신거처럼 모든 원시값을 포장하면 코드가 너무 많아져요. 그렇지만 고민해보고 이 값에 특정한 책임이 부여되어야 하거나 행위를 해야한다는 판단이 서면 객체로 격상시키면 됩니다.

좋은 고민이네요 더즈!

미래에 있을 요구사항을 고려하고 계시는군요. 1단계 미션에서도 제가 요구사항에 대해 많이 언급했지요.

이건 선택의 문제에요. 정말 일어나지 않을 일에 대해서 고려하다가 오버 엔지니어링을 할 수도 있구요. 혹은 대비하지 않고 있다가 변화에 유연하지 않은 구조가 될 수도 있구요. 저도 어려워서 고민하고 공부하는 부분이랍니다.

다만 지금은! 우테코! 공부하는 시간이죠. 이름이 여러 곳에서 사용될 수 있다는 요구사항이 있다고 가정해볼까요?
이럴 때에는 변하는 것과 변하지 않는 것을 구분해야해요. 더즈가 적어주셨네요.

그럼 각 객체마다 name의 검증 내용이 달라질 수도 있는데

이름이라는 객체에서 변하는 것은 검증하는 내용. 변하지 않는 건 문자열을 가지고 있다는 점.

이럴 경우에는 변하는 것을 추상화하여 유연하게 대처할 수 있는 환경을 만들어주면 좋답니다!

고민해보시면 좋을 것 같아요. 조금 더 힌트를 드리자면! 자동차 객체에서 변하지 않는 것과 변하는 것을 구분해보고 변하는 것을 유연하게 대처하기 위해 더즈가 어떻게 대쳐했는지를 상기해보면 좋겠네요!

실제로 구현해봐도 좋아요. 대신 생각하신 내용은 꼭 댓글에 남겨주세요!

toneyparky · 2022-02-16T22:24:16Z

docs/racingcar/README.md

+- [예외] 같은 이름의 자동차가 2대 이상 있을 경우
+ - `자동차 이름이 중복되면 안됩니다.` 메세지를 출력한다.


이 요구사항은 어떻게 추가된건가요? 요구사항이 적혀있는 문서가 있을까요?

요구사항에는 없었지만 같은 이름의 자동차는 있으면 안될 것 같아서 추가해주었습니다!

아하! 저는 2 단계 미션에 숨겨진 요구사항이 있는줄 알았어요 😂

toneyparky · 2022-02-16T22:26:00Z

src/main/java/racingcar/domain/RacingResult.java

+ records.add(record);
+ }
+
+ public List<List<CarDto>> get() {


무얼 반환하는 메서드일까요? 메서드 이름에서 의도가 충분하게 드러나지 않아요.

RacingResult에서 가져오는 거라 경주 결과를 가져오겠거니 get()만 써도 되지 않을까 싶었는데 다른 사람이 봤을 땐 이해하기 어려울 수도 있을것 같습니다. 좀 더 명확한 이름으로 바꾸어보았습니다!

네 좋습니다!

toneyparky · 2022-02-16T22:29:28Z

src/main/java/racingcar/controller/RacingController.java

+ racingResult.get().stream()
+ .forEach(OutputView::printRacingInfo);


스트림...! 정말 좋은 친구라고 생각해요. 그 이유는 내부 반복, 체이닝, 함수형 프로그래밍의 활용 등이 있겠죠!

다만 위와 같은 단순 반복문 .forEach()에 대해서는 일반 for문이 아니라 스트림을 적용하는 것도 좋은 방법이 될 수 있어요!

꼭 한가지를 택해야한다는 규칙은 아니고, 여러가지 의견을 알아두면 좋다는 생각에서 글을 공유합니다.

스트림에 대해서 더 공부해보셔도 재미있을거에요!

스트림이 진리는 아니다에 대해선 어렴풋이 알고 있긴 했었는데 저런 내용이 있었군요. 뭔가 스트림을 처음 접했을 때는 스타일리쉬하고 코드 라인이 줄어드는게 좋아서 막 썼었는데 앞으로 단순 반복문에 대해서는 한 번 더 고민하고 쓰도록 하겠습니다. 스트림에 대해서는 언젠가 더 파볼 생각이에요!

좋아요! 모든 기술에는 장단점이 있다라는 사실을 알고 가시면 충분합니다.

스트림에 대해서 더 공부하고 싶으시다면 이 책을 추천드려요. 내용이 조금 어려울 수 있기에 필요한 부분만 읽어보시면 좋습니다.

화이팅!

…로 이동

toneyparky

리뷰 반영과 댓글에 답변도 빠르게 남겨주셨네요!

Name 과 관련된 리뷰에 대해서는 더 구현해보셔도 좋고, 더즈가 고민하신 결과를 댓글로 남겨주세요!

2 단계의 요구사항을 잘 만족하셨기에 머지하도록 할게요.

우테코 첫 미션 고생 많이 하셨습니다 😉

ldk980130 added 7 commits February 15, 2022 13:53

refactor: CarRepository에서 Dto 사용 로직 없애고 position이 제일 큰 List<Car> 반환

33ecfcf

refactor: findMaxPositionCars() 로직 정렬 없이 변경

f67e8db

refactor: 시도횟수만큼 반복하는 로직 RacingController에서 RacingService로 이동

a0b631e

test: 테스트 메서드 이름 @DisplayName 사용

992a4a1

refactor: 난수 생성 인터페이스 이름 MovingStrategy로 변경

63c093d

style : 코딩 컨벤션에 맞게 상수와 변수 선언 순서 수정

f6e69e1

feat : 자동차 이름 중복 예외 기능 구현

158ffb9

toneyparky requested changes Feb 16, 2022

View reviewed changes

ldk980130 added 4 commits February 17, 2022 10:57

style : 테스트 메서드 이름 더 명확하게 수정

468d427

test : RacingServiceTest에서 이루어지던 CarRepository 테스트를 CarRepositoryTest…

e5599d3

…로 이동

style : RacingResult의 변수명과 메서드 명을 더 명확하게 변경

973bae7

refactor : 시도 횟수 AttemptNumber 객체로 포장

4b089df

toneyparky approved these changes Feb 17, 2022

View reviewed changes

toneyparky merged commit 1babff0 into woowacourse:ldk980130 Feb 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 자동차 경주 리팩터링] 더즈(이동규) 미션 제출합니다. #357

[2단계 - 자동차 경주 리팩터링] 더즈(이동규) 미션 제출합니다. #357

ldk980130 commented Feb 16, 2022

toneyparky left a comment

toneyparky Feb 16, 2022

ldk980130 Feb 17, 2022

toneyparky Feb 17, 2022

toneyparky Feb 16, 2022

ldk980130 Feb 17, 2022

toneyparky Feb 17, 2022

toneyparky Feb 16, 2022

ldk980130 Feb 17, 2022

toneyparky Feb 17, 2022

toneyparky Feb 16, 2022

ldk980130 Feb 17, 2022

toneyparky Feb 17, 2022

toneyparky Feb 16, 2022

toneyparky Feb 16, 2022

toneyparky Feb 16, 2022

ldk980130 Feb 17, 2022

toneyparky Feb 17, 2022

toneyparky Feb 16, 2022

ldk980130 Feb 17, 2022

toneyparky Feb 17, 2022

toneyparky Feb 16, 2022

ldk980130 Feb 17, 2022

toneyparky Feb 17, 2022

toneyparky Feb 16, 2022

ldk980130 Feb 17, 2022

toneyparky Feb 17, 2022

toneyparky left a comment

		@DisplayName("자동차 생성 position 음수")
		public void createCarPositionWithNegative() {

		- [예외] 같은 이름의 자동차가 2대 이상 있을 경우
		- `자동차 이름이 중복되면 안됩니다.` 메세지를 출력한다.

		racingResult.get().stream()
		.forEach(OutputView::printRacingInfo);

[2단계 - 자동차 경주 리팩터링] 더즈(이동규) 미션 제출합니다. #357

[2단계 - 자동차 경주 리팩터링] 더즈(이동규) 미션 제출합니다. #357

Conversation

ldk980130 commented Feb 16, 2022

toneyparky 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

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

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

Choose a reason for hiding this comment

toneyparky left a comment

Choose a reason for hiding this comment