-
Notifications
You must be signed in to change notification settings - Fork 907
[자동차 경주] 이선욱 미션 제출합니다. #923
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
base: main
Are you sure you want to change the base?
Conversation
HuitaePark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2주차도 수고하셨습니다~
제 의견은 개인적인 부분이니 가볍게 참고해 주시면 감사하겠습니다!
| private List<String> readAndValidateCarNames() { | ||
| List<String> carNames = InputView.readCarNames(); | ||
| String input = String.join(",", carNames); | ||
| InputValidator.validateCarNames(input); | ||
| return carNames; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으론 메서드 이름에 and를 넣는건 지양하는 편입니다.
두가지 일을 동시에 한다는 인상을 주기에 충분하기 때문입니다!
또한 validate의 일은 이미 Validator에게 넘기셔서 read의 동작만 나타내는 네이밍을 하셔도 괜찮지 않을까 싶습니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇군요
감사합니다! 확실히 하나의 확실한 네이밍이 더 직관적인거 같네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 and 주의해야겠어요. 감사합니다
| public RacingGame(List<String> carNames, int rounds) { | ||
| this.cars = carNames.stream() | ||
| .map(Car::new) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
자바 17부턴 .toList()만 사용하셔도 됩니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
옛날 자료를 보고 공부했더니 몰랐네요 감사합니다!
최신 문서들 보고 문법공부를 더 해야겠네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 문법과 .toList()의 차이점이 있으니 사용하기 전에 한번 확인해보시면 좋을 것 같아요!
| throw new IllegalArgumentException("시도 횟수는 공백일 수 없습니다."); | ||
| } | ||
|
|
||
| int count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
선언을 try 바깥에서 한 이유가 있을까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 밑에 if문에서도 사용하니까 try문 내용과 구분하려고 바깥에서 선언을 한거 같은데
try문에서 같이 선언해주는게 좋을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 가독성이 더 좋아질 것 같아요 ㅎㅎ
그리고 앞선 리뷰와 마찬가지로 목적을 조금 더 구분하면, 코드가 더 명확해질 것 같아요!
현재는 DRY에 위배되는 것 같습니다!
| private int readAndValidateRoundCount() { | ||
| int roundCount = InputView.readRoundCount(); | ||
| InputValidator.validateRoundCount(String.valueOf(roundCount)); | ||
| return roundCount; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
구조상 NumberFormatException이 날수도 있을것 같은데 readRountCount 하기전에 먼저 검증을 하셔야 오류를 피할수 있을것 같습니당!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 감사합니다!
| private static void validateSingleCarName(String name) { | ||
| if (name.isEmpty()) { | ||
| throw new IllegalArgumentException("자동차 이름은 빈 문자열일 수 없습니다."); | ||
| } | ||
| if (name.isBlank()) { | ||
| throw new IllegalArgumentException("자동차 이름은 공백만으로 이루어질 수 없습니다."); | ||
| } | ||
| if (name.length() > MAX_NAME_LENGTH) { | ||
| throw new IllegalArgumentException("자동차 이름은 5자 이하여야 합니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Car 객체 생성자에서 검증하는 것과 동일한 코드인데, Car 클래스 내부 검증이 중요하므로 검증 로직을 Car 생성자에 넣은 것으로 이해했습니다. 다만, 이후에 규칙을 바꾸는 경우 두 곳 모두를 변경해야 하므로 유지보수에서 문제가 발생할 것 같습니다.
저의 경우는 입력 시에는 split한 값에 대해 검증하지 않고 입력 후 Car 객체 생성 시 생성자에서 호출하여 위 부분을 검증하도록 했습니다.(물론 제가 정답은 아닙니다..!)
그리고 문자열 입력값 자체를 검증하는 기능도 넣어주셨는데 저는 이 부분을 깜빡했네요. 꼼꼼하게 예외처리하신 것 배워갑니다.
추가로 isBlank()에서도 isEmpty()와 같이 빈 문자열일때 true를 반환하지만, 빈 문자열에 대한 예외를 강조하기 위해서 따로 조건문을 두신 것으로 이해했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
입력시에도 검증해야한다고 생각해 이렇게 작성했는데 말씀해주신 부분 생각해보니 코드 중복이 너무 심해질 수 있을거 같네요. 저도 객체에서만 검증하도록 하는것이 더 좋을거 같다는 생각이 듭니다.
피드백 감사합니다!!
isBlank는 말씀하신대로 두 경우의 차이에서 의도를 나타내기 위해 추가했는데 괜찮은 선택인지는 잘 모르겠네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fast Fail을 고려하신 것 같아서 전 보기 좋습니다!
하지만 목적을 명확히 구분하면 좋을 것 같습니다!
| this.rounds = rounds; | ||
| } | ||
|
|
||
| public List<Car> getCars() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른사람들의 코드를 보고 스스로 개선할점
get메소드를 구현시에는 매우 주의해야함, 만약에 꼭 필요한 상황이라도 List로 반환하면 참조값을 반환하게되어 외부에서 값이 수정될 가능성도 있음을 인지하고 상황에 맞는 자료형으로 반환하기위해 노력할 것
https://velog.io/@backfox/getter-쓰지-말라고만-하고-가버리면-어떡해요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
방어적 복사 좋습니다 :)
bebeis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 구조가 전반적으로 너무 깔끔해요! 코드가 너무 잘 읽혔습니다!
저를 포함한 다른 많은 분들이 VO, 일급 컬렉션을 적용하지만, 완벽하게 적용하는 분은 거의 없었던 것 같아요. 만약 실무였거나 코테였다면 선욱님 같은 스타일의 코드도 충분히 훌륭한 코드라고 생각합니다!
| private void playGameWithOutput(RacingGame game, int roundCount) { | ||
| for (int i = 0; i < roundCount; i++) { | ||
| game.playRound(); | ||
| printRoundResult(game); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분 메서드 작명에서 의도를 드러내신 부분 정말 잘하신 것 같아요! 👍
많은 분들이 playGame() 이렇게 작성하셨는데, Controller 본연의 역할(model과 view의 오케스트레이션)이 추상화되어 문제가 있다고 보였습니다. 선욱님은 그 부분을 명시해주신 부분이 인상깊어요
| for (Car car : cars) { | ||
| OutputView.printCarStatus(car.getName(), car.getPosition()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stream Api를 쓰는 것도 좋을 것 같습니다.
| } | ||
| OutputView.printRoundResult(); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EOF 신경쓰면 좋을 것 같아요
| throw new IllegalArgumentException("자동차 이름은 공백만으로 이루어질 수 없습니다."); | ||
| } | ||
| if (name.length() > MAX_NAME_LENGTH) { | ||
| throw new IllegalArgumentException("자동차 이름은 5자 이하여야 합니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5자 이하에서 "5"도 상수로 구성시키면 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DRY! 좋은 피드백인 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Junit 어노테이션을 잘 활용하신 부분 좋습니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OutputViewTest인데 사실상 formatting 검증만 하고 있네요.
OutputView와 Formatter를 분리해야 한다는 신호라고 생각합니다!
exena
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
|
|
||
| if (count <= 0) { | ||
| throw new IllegalArgumentException("시도 횟수는 0보다 커야 합니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3주차부터는 enum을 사용하라고 하셨으니까 예외 메세지를 모아놓은 enum을 쓰면 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사합니다! enum과 상수를 사용하도록 신경써야겠네요
|
|
||
| public static List<String> readCarNames() { | ||
| System.out.println(CAR_NAMES_INPUT_MESSAGE); | ||
| String input = Console.readLine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우테코에서 제공하는 missionutils 라이브러리 안에 Console.close() 함수가 있다고 하더라구요. 입력이 다 끝난 뒤 혹은 main 마지막에 이 함수를 쓰는게 좋아보여요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console.close()는 JVM이 inputStream을 알아서 처리해주기 때문에, 명시적으로 표현하지 않아도 될 것 같아요!
| } | ||
| } | ||
|
|
||
| private void tryMove(Car car) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인 주도 설계적으로 생각해봤을 때, 이 함수는 Car 안으로 들어가는게 나아보여요.
이건 절차지향적인 설계에 좀 더 가까워보여요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백 감사합니다!
| assertThatThrownBy(() -> new Car(name)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("이름"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParameterizedTest를 쓰신 부분 배워갑니다.
Fiddich-Dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2주차 미션도 고생하셨습니다!!
| private List<String> readAndValidateCarNames() { | ||
| List<String> carNames = InputView.readCarNames(); | ||
| String input = String.join(",", carNames); | ||
| InputValidator.validateCarNames(input); | ||
| return carNames; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 and 주의해야겠어요. 감사합니다
| } | ||
| OutputView.printRoundResult(); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RacingGame의 로그를 찍는 느낌이라 RacingGame내부에 작성하는건 어떤가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀하신대로 RacingGame에서 하는게 역할이 더 맞는거 같네요 감사합니다!
| private static final String ROUND_COUNT_INPUT_MESSAGE = "시도할 횟수는 몇 회인가요?"; | ||
|
|
||
| private InputView() { | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 static메서드를 사용할경우에는 생성자를 숨기는게 좋네요 하나 배워갑니다
| private static final String RESULT_MESSAGE = "실행 결과"; | ||
| private static final String WINNER_PREFIX = "최종 우승자 : "; | ||
| private static final String POSITION_SYMBOL = "-"; | ||
| private static final String CAR_STATUS_FORMAT = "%s : %s"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
출력 형식을 포맷 문자열로 정할수 있는거 잊고있었네요.
jihwankim128
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2주차 미션 정말 고생하셨습니다!
좋은 코드 잘보고갑니다! 책임 분리가 잘되어 있네요!!
| throw new IllegalArgumentException("자동차 이름은 공백만으로 이루어질 수 없습니다."); | ||
| } | ||
| if (name.length() > MAX_NAME_LENGTH) { | ||
| throw new IllegalArgumentException("자동차 이름은 5자 이하여야 합니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DRY! 좋은 피드백인 것 같아요!
| this.rounds = rounds; | ||
| } | ||
|
|
||
| public List<Car> getCars() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
방어적 복사 좋습니다 :)
| private static void validateSingleCarName(String name) { | ||
| if (name.isEmpty()) { | ||
| throw new IllegalArgumentException("자동차 이름은 빈 문자열일 수 없습니다."); | ||
| } | ||
| if (name.isBlank()) { | ||
| throw new IllegalArgumentException("자동차 이름은 공백만으로 이루어질 수 없습니다."); | ||
| } | ||
| if (name.length() > MAX_NAME_LENGTH) { | ||
| throw new IllegalArgumentException("자동차 이름은 5자 이하여야 합니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fast Fail을 고려하신 것 같아서 전 보기 좋습니다!
하지만 목적을 명확히 구분하면 좋을 것 같습니다!
| throw new IllegalArgumentException("시도 횟수는 공백일 수 없습니다."); | ||
| } | ||
|
|
||
| int count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 가독성이 더 좋아질 것 같아요 ㅎㅎ
그리고 앞선 리뷰와 마찬가지로 목적을 조금 더 구분하면, 코드가 더 명확해질 것 같아요!
현재는 DRY에 위배되는 것 같습니다!
|
|
||
| public static List<String> readCarNames() { | ||
| System.out.println(CAR_NAMES_INPUT_MESSAGE); | ||
| String input = Console.readLine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console.close()는 JVM이 inputStream을 알아서 처리해주기 때문에, 명시적으로 표현하지 않아도 될 것 같아요!
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| class InputViewTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
View는 테스트 하지 않아도 될 것 같아요!
그 이유에 대해 조금 더 고민해보시면 좋을 것 같아요!
| void createCarWithBlankName(String name) { | ||
| // when & then | ||
| assertThatThrownBy(() -> new Car(name)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("이름"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
83번줄 테스트와 함께 묶을 수 있습니다!
@NullAndEmptySource와 @valuesource를 같이 활용해봐요!
| void comparePosition() { | ||
| // given | ||
| Car car1 = new Car("pobi"); | ||
| Car car2 = new Car("woni"); | ||
|
|
||
| car1.moveForward(); | ||
| car1.moveForward(); | ||
| car2.moveForward(); | ||
|
|
||
| // when & then | ||
| assertThat(car1.getPosition()).isGreaterThan(car2.getPosition()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 직접 비교하는 것이라, 비교 메서드가 있으면 더 명확하겠어요!
when으로 한 번 고려해보시면 좋을 것 같아요!!
JYL35
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 코드 잘 보고 갑니다!
2주차도 고생하셨고, 3주차도 화이팅입니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
하나의 메소드에서 여러 검증을 하는 것 같은데 분리하는게 좋아보입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백 감사합니다!
| public static int parseRoundCount(String input) { | ||
| return Integer.parseInt(input); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파싱하기 전에 검증이 필요해보입니다!
| public RacingGame(List<String> carNames, int rounds) { | ||
| this.cars = carNames.stream() | ||
| .map(Car::new) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 문법과 .toList()의 차이점이 있으니 사용하기 전에 한번 확인해보시면 좋을 것 같아요!
| public void play() { | ||
| for (int i = 0; i < rounds; i++) { | ||
| playRound(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 메소드는 테스트를 위한 메소드로 확인이 되는데,
테스트에 구현해서 사용하는게 더 좋을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
문자 출력에 대한 역할 분리를 잘하신 것 같습니다!

No description provided.