[터틀] 자동차 경주 미션 제출합니다. #107
Conversation
- 숫자값을 상수로 변경 - indent 수정
- 자동차가 전진했는지 멈췄는지 확인하는 테스트 구현
- 참가자의 이름을 알기 위해 getName 생성
- 우승자를 산출하는 getWinner와 우승자의 이름을 구하는 getWinnerName 메소드 구현 - 우승자를 구하기 위해 Car에 Combarable 인터페이스 구현
- RacingCars 클래스 제거 - RacingCarGame의 인스턴스 변수 제거
- RacingCarGame에서 사용되는 UI로직을 Application으로 이동
- 이름과 위치의 검증 로직과 상수를 해당 객체에서 관리하도록 변경
- RacingCarGame에서는 로직의 흐름만 관리하도록 변경
- UI로직을 Application으로 이동
- CarStatus 출력부분을 StringBuilder로 변경 - 자동차 전진 테스트를 CarTest클래스로 이동
- 필드, 메소드명 수정 - RacingCars 클래스에서 maxPosition을 get하는 방식에서 확인하는 방식으로 변경
- for문을 stream으로 변경 - 필드 이름 수정
- 하드코딩한 값들을 상수로 변경
- CarDto의 클래스 명을 CarStatus로 변경
| if (isOnlyOneNumber(expression)) { | ||
| return stringToInteger(expression); | ||
| } |
There was a problem hiding this comment.
굳이 예외 처리 하지 않고 하나의 로직으로 풀 수 있을 것 같네요! :)
| Integer resultWithOtherDelimiter = splitWithOtherDelimiter(expression); | ||
| if (resultWithOtherDelimiter != null) { | ||
| return splitWithOtherDelimiter(expression); |
| Matcher m = Pattern.compile("//(.)\n(.*)").matcher(s); | ||
| if (m.find()) { | ||
| String customDelimiter = m.group(1); | ||
| String[] tokens = m.group(2).split(customDelimiter); | ||
| return addArrayElements(tokens); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
불필요하게 null을 리턴하는것 보다는 expression.split(",|:");를 통한 로직을 실행하는게 더 좋지 않을까요?
| } | ||
|
|
||
| private static Integer splitWithOtherDelimiter(String s) { | ||
| Matcher m = Pattern.compile("//(.)\n(.*)").matcher(s); |
There was a problem hiding this comment.
이펙티브 자바 아이템6 불필요한 객체 생성을 피하라 한번 읽어보시면 좋을 것 같아요!
Pattern 객체의 경우 생성 비용이 비싸고 재사용이 가능한 객체이므로 static final로 관리하는걸 권장드려요. :)
| } | ||
|
|
||
| private ArrayList<CarStatus> processGame(RepeatTimes repeatTimes, RacingCars racingCars, MoveDecider moveDecider) { | ||
| ArrayList<CarStatus> racingResult = new ArrayList<>(); |
There was a problem hiding this comment.
이펙티브 자바 아이템 64. 객체는 인터페이스를 사용해 참조하라 읽어보세요!
메서드의 리턴값도 마찬가지입니다. :)
|
|
||
| import racingcargame.domain.Car; | ||
|
|
||
| class consoleOutput { |
| public String getWinner() { | ||
| Collections.sort(cars); | ||
| int maxPosition = cars.get(0).getCarPosition(); | ||
|
|
||
| List<String> winnerCar = cars.stream() | ||
| .filter(car -> car.isMaxPosition(maxPosition)) | ||
| .map(Car::getCarName) | ||
| .collect(Collectors.toList()); | ||
| return String.join(DELIMITER, winnerCar); |
There was a problem hiding this comment.
위너의 String을 만들어 주는건 도메인이 아닌 뷰에 종속적인 부분 인 것 같네요.
위너의 CarName을 돌려주고 이를 뷰에서 활용해보면 어떨까요?
또한 기능상 메서드가 단수형이 아닌 복수형에 더 적합해보입니다. :)
| private static int parseStringToInt(String inputRepeat) { | ||
| try { | ||
| return Integer.parseInt(inputRepeat); | ||
| } catch (NumberFormatException e) { | ||
| throw new IllegalArgumentException("정수를 입력해주세요."); | ||
| } | ||
| } |
There was a problem hiding this comment.
RepeatTimes 스스로가 이를 체크하도록 할 수 있지 않을까요? :)
| private static void printRaceResult(List<CarStatus> carStatus) { | ||
| carStatus.forEach(consoleApp::printEachRaceStatus); | ||
| } | ||
|
|
||
| private static void printEachRaceStatus(CarStatus carStatus) { | ||
| carStatus.getCars().forEach(consoleOutput::printRaceStatus); | ||
| System.out.println(); | ||
| } |
| @Test | ||
| @DisplayName("우승자의 이름이 제대로 추출되는지") | ||
| void checkWinnerName() { | ||
| // cars.get(0).decideGoOrStop(8); | ||
| // cars.get(1).decideGoOrStop(1); | ||
| // cars.get(2).decideGoOrStop(0); | ||
| assertThat(racingCars.getWinner()).isEqualTo("car1"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("우승자가 복수일 때 이름이 제대로 추출되는지") | ||
| void checkWinnersName() { | ||
| // cars.get(0).decideGoOrStop(8); | ||
| // cars.get(1).decideGoOrStop(5); | ||
| // cars.get(2).decideGoOrStop(0); | ||
| // assertThat(racingCarGame.getWinner(cars)).isEqualTo("car1, car2"); | ||
| } |
There was a problem hiding this comment.
랜덤 요소때문에 테스트가 어려우셨을 것 같아요.
MoveDecider라는 객체로 분리하신건 정말 잘하셨는데, 여기서 조금만 더 추상화하면 테스트하기 용이하실거에요.
현재는 MoveDecider가 랜덤만을 리턴하지만, 이를 인터페이스로 만들면
랜덤으로 리턴하는 MoveDecider 구현체, 원하는 값을 리턴하는 MoveDecider 구현체를 만들수도 있겠네요. :)
There was a problem hiding this comment.
원하는 값을 리턴하는 MoveDecider 구현체
말씀해주신 원하는 값을 리턴하는 기능은 프로덕션코드에는 사용하지 않을 것 같은데,
테스트코드만을 위한 프로덕션코드는 허용되는 부분인가요?😮
There was a problem hiding this comment.
꼭 프로덕션에 있을 필요는 없어요. 테스트 코드 내에 해당 인터페이스의 구현체를 두고 테스트 용도로도 사용할 수 있어요. :)
테스트하기 어려운 부분을 격리하고 원하는 값을 돌려주는 Mock을 실제 테스트에서도 많이 사용해요.
테스트 더블에 대해서 기회 되시면 공부해보셔도 좋을 것 같네요. :)
There was a problem hiding this comment.
몰랐던 새로운 내용을 알려주셔서 감사합니다! 공부해보겠습니다 :)
- README.md 수정
- domain패키지에 Calculator, Expression클래스 추가
- view패키지에 InputView 클래스 생성
- null을 입력한 경우 - 빈 문자열을 입력한 경우 - 숫자만 입력한 경우
- CalculatorApp클래스는 계산기 실행과 입출력 역할만 하도록 변경
- 기존에 문자열 구분을 테스트하던 코드를 StringSplitterTest 클래스로 이동
- 피연산자들의 검증로직과 합을 구하는 메소드 추가 - StringSplitter가 Operand의 리스트를 반환하도록 변경 - Calculator 클래스에서 입력값을 분리해 Expression에게 넘겨주도록 변경
- 기존에 null, 빈 문자열을 검증하는 로직을 Expression을 생성할 때에서 연산할 때로 변경 - 연산의 결과가 오버플로우 발생시 처리 로직 추가 - 예외처리 되는 테스트코드들의 확인방법을 클래스가 아닌 에러메시지로 변경
- 구분자를 명확하게 입력받을 수 있도록 변경
|
코드도 마음에 들지 않고, TDD도 제대로 적용하지 못한것 같아서 주말을 이용해 테스트코드 기반으로 다시 구현했습니다. |
- view패키지의 클래스명을 대문자로 시작하도록 convention 수정 - RacingCarApp클래스에서 출력하는 로직을 OutputView클래스로 이동 - RacingCars에서 우승자를 String으로 변환하는 로직을 OutputView 클래스로 이동 - RacingCars의 setCars 메소드 이름을 createCars로 변경 - RacingCarController에서 racingResult를 인터페이스를 사용해 참조하도록 변경
- 게임 실행 후 RacingResult를 반환하도록 변경 - CarStatus의 클래스명을 RacingStatus로 변경 - RacingResult 객체가 경기의 모든 결과를 갖고있도록 변경 - 반복 횟수의 입력값을 int로 파싱하는 로직을 RepeatTimes 클래스로 이동 - 경기 결과를 출력하는 로직을 하나로 통합
- Car와 관련된 클래스를 car패키지로 이동 - 이동에 따라 접근제어자를 알맞게 변경
- Car 리스트를 받아 CarDto로 변환하는 기능 추가 - Car 리스트를 복사하는 메소드 제거 - Car 리스트를 복사하는 메소드에서만 쓰이던 Car, CarPosition생성자 제거 - 가장 멀리간 차를 구하는 부분을 메소드로 분리
- 테스트를 위해 Car, CarDto 클래스에 equals 메소드 추가 - 테스트를 위해 Car클래스의 move 추상화
|
피드백 주신 내용 반영했습니다 :) |
- 생성자에 null 체크 로직 추가 - maxPosition을 구하는 방식 변경, 그에따라 CarDto클래스의 compareTo 변경 - RacingCars를 생성할때 구분자만 들어온 경우를 검증하는 로직 추가 - RacingResult에서 우승자를 구할때 RacingLog 리스트 사이즈가 0인 경우를 검증하는 로직 추가
- Car 클래스의 테스트 추가 - RacingCars 클래스의 테스트코드 추가 - RacingLog 클래스의 테스트코드 추가 - RacingResut 클래스의 테스트코드 추가 - RepeatTimes 클래스의 테스트코드 추가
- RacingLog클래스에 toString메소드 구현
| try { | ||
| return new Operand(Integer.parseInt(input)); | ||
| } catch (NumberFormatException e) { | ||
| throw new OperandException(OperandException.NOT_NUMBER); |
|
|
||
| public Operand calculate() { | ||
| if (operands == null || operands.isEmpty()) { | ||
| return new Operand(0); |
There was a problem hiding this comment.
new Operand(0);같이 자주 사용되는 객체 같은 경우는
미리 만들어 놓고 static final 변수로 제공하기도 해요. :)
BigDecimal.ZERO등의 구현 참고해보세요!
| public class RacingCarController { | ||
| public RacingResult run(String names, RepeatTimes repeatTimes) { | ||
| final RacingCars racingCars = new RacingCars(names); | ||
| final MoveDecider moveDecider = new MoveDecider(); |
There was a problem hiding this comment.
이펙티브 자바 아이템 64. 객체는 인터페이스를 사용해 참조하라
참고해주세요. :)
There was a problem hiding this comment.
또한 OCP,DIP 관점에서 RacingCarController내부에서 구현체를 생성하는 게 옳은지도 고민해보시면 좋을 것 같아요.
이럴 경우 마찬가지로 RacingCarController의 테스트가 어렵지 않을까요? :)
There was a problem hiding this comment.
이펙티브 자바 아이템 64. 객체는 인터페이스를 사용해 참조하라
이미 피드백 주신 내용인데도 적용을 못했네요 ㅠㅠ 제가 만든 인터페이스라서 신경을 못 썼는데 꼭 습관화하겠습니다!
또한 OCP,DIP 관점에서 RacingCarController내부에서 구현체를 생성하는 게 옳은지도 고민해보시면 좋을 것 같아요.
이럴 경우 마찬가지로 RacingCarController의 테스트가 어렵지 않을까요? :)
Domain만 신경쓰고있었는데 알려주셔서 감사합니다. RacingCarApp에서 생성해 전달하는 방식으로 리팩토링해보겠습니다 :)
| @Override | ||
| public int getMoveNumber() { | ||
| return random.nextInt(MOVE_BOUND) + STOP_BOUND; | ||
| } | ||
|
|
||
| @Override | ||
| public int getStopNumber() { | ||
| return random.nextInt(STOP_BOUND); | ||
| } |
There was a problem hiding this comment.
테스트코드를 위해 구현했었고, 테스트코드에 적용을 못하고 있었는데, 적용해보겠습니다!
| @Test | ||
| @DisplayName("우승자의 이름이 제대로 추출되는지") | ||
| void checkWinnerName() { | ||
| // cars.get(0).decideGoOrStop(8); | ||
| // cars.get(1).decideGoOrStop(1); | ||
| // cars.get(2).decideGoOrStop(0); | ||
| assertThat(racingCars.getWinner()).isEqualTo("car1"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("우승자가 복수일 때 이름이 제대로 추출되는지") | ||
| void checkWinnersName() { | ||
| // cars.get(0).decideGoOrStop(8); | ||
| // cars.get(1).decideGoOrStop(5); | ||
| // cars.get(2).decideGoOrStop(0); | ||
| // assertThat(racingCarGame.getWinner(cars)).isEqualTo("car1, car2"); | ||
| } |
There was a problem hiding this comment.
꼭 프로덕션에 있을 필요는 없어요. 테스트 코드 내에 해당 인터페이스의 구현체를 두고 테스트 용도로도 사용할 수 있어요. :)
테스트하기 어려운 부분을 격리하고 원하는 값을 돌려주는 Mock을 실제 테스트에서도 많이 사용해요.
테스트 더블에 대해서 기회 되시면 공부해보셔도 좋을 것 같네요. :)
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| public RacingLog processOneRace(MoveDecider moveDecider) { |
There was a problem hiding this comment.
이펙티브 자바 아이템 64. 객체는 인터페이스를 사용해 참조하라 :)
|
|
||
| import java.util.Random; | ||
|
|
||
| public class MoveDecider implements MoveStrategy { |
There was a problem hiding this comment.
MoveDecider가 어떤 구현을 담고있는지를 조금 더 명확하게 드러낼 수 있는 이름이 더 좋지 않을까요? :)
- MoveDecider클래스의 이름을 NumberGenerator로 변경 - RacingCars에서 랜덤 숫자를 생성해 decideMoveOrStop 메소드로 넘겨주도록 변경
- 테스트코드의 접근제어자 변경 - 불필요한 인스턴스 변수 제거
|
바쁘실텐데 꼼꼼하게 리뷰해주셔서 정말 감사합니다🙇🏻♂️ |
리뷰 요청드립니다🙇🏻♂️