[자동차 경주 게임] 박진훈 리뷰 부탁드립니다! #522
Conversation
기존에는 printStackTrace 로 출력하였으나, 메시지 출력방식을 원해서 변경 TryCommand 클래스에서 String->int 형변환시 예외 catch 를 NumberFormatException 으로 수정
Cars 객체 생성시 불변값으로 설정
상수화가 안된 메시지가 있어서 수정
| private static final int MOVABLE_MIN_NUMBER = 4; | ||
| private final String name; |
There was a problem hiding this comment.
컨벤션에 따라서 상수와 인스턴스 변수를 새 줄로 구분해주시는 게 좋을 것 같습니다!!
| private Cars(List<Car> cars) { | ||
| this.cars = Collections.unmodifiableList(cars); | ||
| } |
There was a problem hiding this comment.
불변 객체를 만들 때 생성자 단계에서는 방어적 복사를 고려해주시면 더 좋을 것 같아요!!
There was a problem hiding this comment.
오... 좋은글 감사합니다. 방어적복사는 처음 접해보는 개념이네요!
| public class Winner { | ||
|
|
||
| private final List<String> winner; |
There was a problem hiding this comment.
리스트의 네이밍은 복수형이 더 적절하다고 생각합니다!
Winner 클래스 자체도 승리자들의 이름을 담고있는 일급 컬렉션이기 때문에 Winners 라는 네이밍을 더 추천드립니다!
| private String convertMoveMark(int position) { | ||
| return Stream.generate(() -> MOVE_MARK).limit(position).collect(Collectors.joining()); | ||
| } |
There was a problem hiding this comment.
String에서 제공하는 repeat() 메서드를 활용해보시는 것도 추천드립니다!
There was a problem hiding this comment.
이런,, 스트림을 너무 남발했네요.. repeat() 라는 좋은 함수가 있는데 말이죠 ㅠ
|
|
||
| public void move(CarMoveNumberGenerator carMoveNumberGenerator) { | ||
| for (Car car : cars) { | ||
| car.move(carMoveNumberGenerator); |
There was a problem hiding this comment.
CarMoveNumberGenerator 를 Car 객체까지 넘겨주지 않아도 될 것 같습니다!
그러면 car.move() 메서드를 테스트하기도 훨씬 간편해질 것 같아요!
| @@ -0,0 +1,22 @@ | |||
| package racingcar.domain; | |||
|
|
|||
| public class RacingCarGame { | |||
There was a problem hiding this comment.
RacingCarGame 클래스의 메서드들이 대부분 Cars 클래스의 메서드를 호출하는 용도로만 사용되는 것 같아요!
RacingCarGame 클래스가 정말 필요한지, 아니면 어떤 식으로 RacingCarGame 만의 새로운 책임을 부여할 수 있을 지
생각해보시면 좋을 것 같아요!!
There was a problem hiding this comment.
원래 RacingCarGame 에서 시도횟수를 가지고있어서 Cars 를 시도횟수만큼 움직이는 로직이있었는데 controller 쪽으로 해당 로직을 옮기는바람에 쓸모없는 클래스가 되었네요.. 좋은 지적 감사합니다..!
|
|
||
| private static final int MIN_TRY = 1; | ||
| private static final int MAX_TRY = 100000; | ||
| private int tryCount; |
There was a problem hiding this comment.
값 객체의 내부 상태를 final 로 선언하는 방법도 있더라구요! 참고해보시면 좋을 것 같습니다!
There was a problem hiding this comment.
불변으로 해도 값을 변경하는 방법이 있었군요..
DDD 에서 나오는 개념이라 MVC 에는 어떻게 적용할지는 고민을 해봐야겠네요..
좋은글 감사합니다!
| for (String carName : words) { | ||
| cars.add(new Car(carName)); | ||
| } | ||
| return new Cars(cars); |
There was a problem hiding this comment.
스트림으로 로직을 바꾸시면 빈 리스트를 선언하지 않고 더 깔끔하게 구현할 수 있을 것 같아요!
| @@ -0,0 +1,10 @@ | |||
| package racingcar.exception; | |||
|
|
|||
| public class CarNameLengthException extends IllegalArgumentException{ | |||
There was a problem hiding this comment.
커스텀 예외들은 장단점이 있습니다! 그래서 정답은 없지만 제가 사용한 이유는
첫째로클래스 이름으로 어떤 예외인지 바로 알 수 있고
둘째로 보통 도메인 코드를 볼때 중요한것은 그 도메인 역할과 관련된 로직이지, 예외처리를 어떻게 하는지가 중요한게 아니기때문에 예외를 따로빼내어 좀 더 보기 편한게 만들기 위함입니다!
셋째로 대부분 Spring MVC 프로젝트에서는 커스텀 예외를 사용하기에 연습차원에서 커스텀 예외를 사용하고 있습니다.
No description provided.