Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

step 5 : 5단계 - 자동차 경주(리팩토링) #5804

Open
wants to merge 6 commits into
base: slowth-kim
Choose a base branch
from

Conversation

Slowth-KIM
Copy link

변경내용

  • 도메인 패키지 안에서 내용을 좀 더 분리했습니다.
  • 테스트 코드로 코드와 일치시키고 이름을 변경했습니다.
  • String.Join 을 사용하고, RacingCars.create 부분을 수정했습니다.
  • 더 수정해야할 부분이 있다면 말씀주시면 감사하겠습니다.

Copy link

@csh0034 csh0034 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5단계도 구현 잘해주셨네요 😄
몇가지 간단한 코멘트 남겨두었는데 확인 부탁드려요!

@@ -1,4 +1,7 @@
package racinggame.racingcar;
package racinggame.domain.race;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race 의 경우 RacingCars 만 포함하고 있습니다.

Race 의 로직을 RacingCars 로 옮긴다면 Race 는 제거 가능하지 않을까요?

@@ -0,0 +1,23 @@
package racinggame.domain.racingcar;

public class CarName {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CarName 에 대해서도 단위테스트를 작성해보는건 어떨까요?

@@ -42,8 +41,10 @@ private void printMessage(final String message) {
private List<String> readValidNames() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

도메인은 ui 에 접근하지 않는걸 권장하지만 ui 에서 도메인 접근은 문제 없다고 생각해요.
List<CarName> 을 그대로 리턴하도록 해보는건 어떨까요?

@@ -11,11 +13,9 @@ private RacingCars(List<RacingCar> cars) {
}

public static RacingCars create(MoveStrategy moveStrategy, List<String> nameList) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변경이 가능하기 때문에 변수명엔 가능하면 자료형을 포함하지 않도록 해보면 좋을것 같아요.

list 는 목록이지만 프로그래밍에선 컬렉션의 하위 자료형이기 때문에
현재 인스턴스 변수명인 cars 와 같이 주로 s 를 붙혀서 복수로 표현하기도 합니다.

Copy link
Contributor

@javajigi javajigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이전 리뷰어 분이 남긴 피드백을 반영하지 않은 상태에서 리뷰 요청을 했는데요.
혹시 빠트린 것인지, 아니면 수정할 부분이 없는지 한번 확인하고 리뷰 요청해주세요.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants