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

🚀 4단계 - 자동차 경주(우승자) #6036

Merged
merged 12 commits into from
Mar 22, 2025

Conversation

jiyoung928
Copy link

안녕하세요 리뷰어님!
저번 step3에서 역할과 책임을 분리하여 리팩토링을 한 덕분에 요구사항을 좀 더 수월하게 반영할 수 있었던것 같습니다! 😆
이번에 step4도 리뷰 잘 부탁드립니다! 🙇‍♀️

@neojjc2 neojjc2 self-requested a review March 19, 2025 01:36
@neojjc2 neojjc2 self-assigned this Mar 19, 2025
@neojjc2
Copy link

neojjc2 commented Mar 19, 2025

#tag @neojjc2

Copy link

@neojjc2 neojjc2 left a comment

Choose a reason for hiding this comment

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

안녕하세요 지영님 🙇

워낙 이전단계에서 고생을 해주신 덕에(?)
4단계 요구사항은 큰 수정 없이 잘 반영해주셨네요 😄
고생하셨습니다 🙇

큰 개선까진 아니고 조금 정리되면 좋을 부분들이 있어서
의견 드렸습니다. 한번 보시고 정리차원에서
한번 의견 검토 해주시면 감사하겠습니다 🙏

그럼 재 리뷰 요청 기다리고 있겠습니다 🙇





Copy link

Choose a reason for hiding this comment

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

쓸데 없는 공백은 제거 되도 좋을 것 같습니다 😄

@@ -22,4 +39,15 @@ public void moveAll(MovingStrategy strategy) {
public List<Car> getCurrentStatus() {
return Collections.unmodifiableList(cars);
}

public String getWinner() {
Copy link

Choose a reason for hiding this comment

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

요구사항 구현 깔끔하게 해주셨네요 👍

return cars.stream()
.filter(car -> car.getMoveCount() == maxMoveCount)
.map(Car::getName)
.collect(Collectors.joining(", "));
Copy link

Choose a reason for hiding this comment

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

", " 역시 출력 형식에 따라서 계속적으로 변경이 될 수 밖에 없는 부분일 것 같습니다 😄
요 부분도 Cars가 만들어주기 보단 Cars는 우승자 이름만 List형식으로 반환해주고
어떻게 보여줄지는 ResultView에서 하는게 더 괜찮을 것 같습니다 🙇

Copy link
Author

Choose a reason for hiding this comment

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

넵! Winners는 우승자 이름 List만 리턴하고 result view에서 해당 list를 출력하도록 변경하였습니다!

ResultView.viewRacingCarWithName(cars.getCurrentStatus());
}
ResultView.viewRacingCarWinner(cars);
}
Copy link

Choose a reason for hiding this comment

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

요구사항 구현 👍

public Car(String name) {
this.name = name;
this.moveCount = 1;
}
Copy link

Choose a reason for hiding this comment

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

이렇게 될 경우 Car를 직접 생성한다면 중요한 검증이 누락 될 수 있습니다.
Cars에서 검증을 하고 계시긴 합니다만, Cars를 통해서 Car를 생성하지 않고
바로 생성할 수 도 있기 때문에 누락을 피하려면 name을 가진 주체 도메인인
Car에서 검증을 하는게 더 자연스러울 것 같습니다 😄

Copy link
Author

Choose a reason for hiding this comment

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

넵! name에대한 검증을 car 에서 하도록 수정하였습니다! ( 기존 inpuview에 있던 검증 로직도 이동하였습니다. )

throw new IllegalArgumentException("자동차 이름은 5자를 초과할 수 없습니다.");
}
}
}
Copy link

Choose a reason for hiding this comment

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

위에서도 의견드렸지만 name을 관리하는 객체에서 검증책임까지 가져가는게 더 자연스러울 것 같습니다 😄
추가로 이 부분에 대한 검증이 필요할 것 같아요 🙏 (중복입력에 관한 부분)
image

Copy link
Author

Choose a reason for hiding this comment

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

중복에대한 검증은 어디서 하면 좋을지 고민을 해봤는데 Cars에서 검증하도록 추가를 했습니다!
이미 Car에서 name에대해 검증을 하기 때문에 namelist를 따로 추가해서 중복 검증을 할지,
Cars 에서 name list를 받아 Car 객체를 구현하는 부분이 있기 때문에 해당 부분에서 검증을 할지,
Race에서 해당 부분 검증 로직을 추가할지 고민하다가 현재 제가 구현한 코드에서는 Car객체를 생성하는 로직이 Cars에 있기때문에
Cars에 중복 검증 로직을 추가해봤습니다! 검증 로직을 어디에 구현하는지 고민이 많이 되는 부분이네요 😂

@@ -1,5 +1,6 @@
package racingcar.strategy;

@FunctionalInterface
Copy link

Choose a reason for hiding this comment

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

요청사항 반영 👍


// Then
assertThat(result).isEqualTo("pobi, crong");
}
Copy link

Choose a reason for hiding this comment

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

테스트 작성 좋네요 👍

Race race = new Race(
new Cars(InputView.inputValidatedNameOfCar())
, InputView.inputValidatedNumberOfAttempts());
race.startWithName(createRandomStrategy());
Copy link

Choose a reason for hiding this comment

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

소소한 의견입니다만 RaceCars가 정적 팩토리 메소드를 제공한다면
사용하는 객체 입장에서 훨씬 수월할 것 같습니다 😄

이미 객체들의 정의는 잘 해주셨으니 사용성 개선을 위한 인터페이스쪽도
한번 정리하고 가시면 좋을 것 같습니다.
요 내용한번 보시면 도움되실 것 같습니다 🙏

https://johngrib.github.io/wiki/pattern/static-factory-method/

Copy link
Author

Choose a reason for hiding this comment

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

정적 팩토리 메소드는 생각을 안해봤는데 덕분에 배우고 갑니다!
아직 어떻게 구현해야 좋을지 감이 안와서 일단 Race를 생성하는 create와 Cars의 생성자를 fromNames로 구현해봤습니다!

@jiyoung928
Copy link
Author

회사일에 치이다보니 평일에 리뷰요청을 많이 못보냈네요 😅
주말동안이라도 열심히 해보겠습니다..! 이번 리뷰도 잘부탁드립니다 🙇‍♀️

@neojjc2
Copy link

neojjc2 commented Mar 22, 2025

회사일에 치이다보니 평일에 리뷰요청을 많이 못보냈네요 😅 주말동안이라도 열심히 해보겠습니다..! 이번 리뷰도 잘부탁드립니다 🙇‍♀️

이 과정이 회사다니면서 병행을 하기가 생각보다 만만치 않습니다 😅
그래도 꾸준히 조금씩 조금씩 하시다 보면 완주하시게 될 거라 생각합니다.
지치지만 않으시면 됩니다 🙏

Copy link

@neojjc2 neojjc2 left a comment

Choose a reason for hiding this comment

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

안녕하세요 지영님 🙇

요청사항 반영하시느라 수고 많으셨습니다.
한결 정리된 느낌이네요 👍

어느덧 마지막 단계만 남았습니다.
조금만 더 힘내주세요 🙏

이만 merge 하겠습니다 🙇

if (name.length() > CAR_NAME_SIZE) {
throw new IllegalArgumentException("자동차 이름은 5자를 초과할 수 없습니다.");
}
}
Copy link

Choose a reason for hiding this comment

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

💯 💯 💯 💯 💯

validNames(names);
this.cars = Arrays.stream(names)

public static Cars fromNames(String[] names) {
Copy link

Choose a reason for hiding this comment

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

좋네요 👍

}
private static void validCarNames(String[] names) {
if (Arrays.stream(names).distinct().count() < names.length) {
throw new IllegalArgumentException("자동차 이름은 중복될 수 없습니다.");
Copy link

Choose a reason for hiding this comment

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

고민하신 부분인 것 같은데 저도 이 자리가 맞다고 생각합니다 😄
소소한 의견입니다만 Set을 활용해보시는것도 방법중에 하나 입니다 🙏

return cars.stream()
.filter(car -> car.getMoveCount() == maxMoveCount)
.map(Car::getName)
.collect(Collectors.joining(", "));
.collect(Collectors.toList());
Copy link

Choose a reason for hiding this comment

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

비즈니스 로직 본연에 더 집중되었네요 😄

아주 소소한 의견이긴 합니다만 Cars는 최종결과만 저장하고 있는 구조입니다.
예를들어 최종 경기를 끝내고 난 뒤 각 라운드 별 1위를 구한다던지,
각 라운드 별 뭔가 정보를 조회하려면 Cars만으로는 좀 어려울 것 같습니다.
경기 결과를 따로 저장하고 있는 객체가 있다면 위와 같은 변경사항에도 좀 더
유연할 것 같습니다.

개선 사항 까진 아니고 구조적인 의견이니 참고 정도만 해주세요 😄
지금 요구사항에서는 이정도 구현도 충분합니다 🙏

@@ -12,6 +13,12 @@ public Race(Cars cars, int totalAttempts) {
this.totalAttempts = totalAttempts;
}

public static Race create() {
String[] carNames = InputView.inputdNameOfCar();
int attempts = InputView.inputValidatedNumberOfAttempts();
Copy link

Choose a reason for hiding this comment

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

attempts도 원시객체로 포장되면 어떨까요 ?? 😄
이 후 미션에서 많이 연습하시게 될 텐데 한번 검토해주시면 좋을 것 같습니다 🙏

요 내용 한번 보고 가시면 좋을 것 같습니다 🙇

https://velog.io/@kanamycine/Java-%EC%9B%90%EC%8B%9C%EA%B0%92-%ED%8F%AC%EC%9E%A5

@neojjc2 neojjc2 merged commit a1a4e38 into next-step:jiyoung928 Mar 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants