[디디] 레이싱게임 리뷰요청 드립니다. #86
Conversation
StringAdderTest 클래스 생성 StringAdderTest 첫번째 테스트 코드 구현 StringAdder 클래스 생성 StringAdderTest 첫번째 테스트 코드에 맞게 실행되도록 구현
StringAdderTest 클래스에 입력이 null이거나 빈문자열인 경우 테스트 구현 StringAdder 클래스에 해당 테스트에 맞게 구현
StringAdderTest 클래스에 숫자1개만 들어오는 경우 테스트 구현 StringAdder 클래스에 해당 테스트에 맞게 구현
StringAdderTest 클래스에 숫자 여러개가 들어오는 경우 테스트 구현 StringAdder 클래스에 해당 테스트에 맞게 구현
StringAdderTest 클래스에 음수가 들어오는 경우 테스트 구현 StringAdder 클래스에 해당 테스트에 맞게 구현
StringAdderTest 커스텀 구분자가 사용될 경우에 대한 테스트 구현
StringAdder 클래스에 해당 테스트에 맞게 구현
예외사항
- 커스텀 구분자는 무조건 맨 처음에만 오는가?
- 커스텀 구분자는 1글자만 가능한가?
- 커스텀 구분자를 쓰면 기존 구분자(',' ':')는 무효화되는가?
- 정규표현식에서 사용되는 문자('?','*' 등)는 사용이 불가능한가?
- 커스텀 구분자로 숫자도 사용할 수 있는가?
README 내에 요구사항, 구현단위(테스트 단위), 예외처리에 대해 작성하였다
StringAdderTest 커스텀 구분자만 들어오고 식이 없는 경우에 대한 테스트 구현 StringAdderTest 숫자가 아닌 문자값이 들어오는 경우에 대한 테스트 구현 StringAdder 클래스에 해당 테스트에 맞게 구현
InputView 입력을 위한 화면 출력과 Scanner 입력 연산 구현 Input 입력받은 문자열을 저장, 검증, split해서 리스트로 반환하는 기능 구현
StringAdder 구조 개선 및 Input/InputView 사용을 위하여 리팩토링 작업을 수행
StringAdderTest Test 로직을 ParameterizedTest로 변경하였다.
StringAdderTest Test 로직에 DisplayName를 추가 StringAdderTest 강의자료 내 테스트 케이스 추가 Input 프린트 메서드 삭제 StringAdder 사용하지 않는 패키지 삭제
Input checkInputEmpty 메서드에 list 파라미터를 추가하여 인덴트를 줄였다.
Input 클래스의 checkInputEmpty 분리하여 validateInput, validateList 메서드 생성 Input 클래스의 changeInput 메서드 생성
StringAdder 클래스의 add 메서드내 리스트 parameterized type Intger로 변경 StringAdder 클래스 내의 validateValues 메서드 생성
InputView 사용하지 않는 클래스 삭제 StringAdder, Input 클린코드 리팩토링
Input 클래스 내의 접근제어자 수정
README 자동차 경주 게임의 기능 목록을 작성하였다.
RacingGameTest 클래스에 이름길이 검증 테스트 구현 RacingGame 테스트에 맞게 기능 구현
RacingGameTest 클래스에 문자열 split 검증 테스트 구현 RacingGame 테스트에 맞게 기능 구현
RacingGameTest 클래스에 여러명 입력시 이름 길이 검증 테스트 구현 RacingGame 테스트에 맞게 기능 구현
RacingGame validateInputList 매개변수 타입 List<String>으로 수정 RacingGameTest 리팩토링에 맞춰 테스트 코드 변경
RacingGameTest 횟수 입력시 타입과 음수인 경우 예외 처리 RacingGame 테스트에 맞게 기능 구현
RacingGameTest 이름을 InputTest로 변경하였음.
Input 입력값에 대한 검증과 split 메서드 구현 InputView 이름과 횟수 입력을 받는 클래스 구현
RacingGameTest 입력값이 4이상이면 true를 반환하는지 테스트 RacingGame 테스트에 맞게 기능 구현
RacingGameTest 난수값을 올바르게 발생시키는지 반복 테스트 RacingGame 테스트에 맞게 기능 구현
RacingGameTest Car에서 move 메서드가 올바르게 작동하는지 테스트 RacingGame, Car 테스트에 맞게 기능 구현
RacingGameTest Car에서 play 메서드가 올바르게 작동하는지 테스트 RacingGame, Car 테스트에 맞게 기능 구현 Input getRepeat 메서드 구현
OutputView 출력을 담당하는 View 클래스 구현 Output 출력값에 대한 전달과 변형을 담당하는 클래스 구현 RacingGame, Car 뷰를 통해 자동차의 로그를 출력하는 메서드 작성
Output 최종 결과 출력 관련 메서드 구현 OutputView 최종 결과 출력 관련 메서드 구현 RacingGame OutputView를 통해 출력 기능 수행하도록 구현 InputView OutputView를 통해 메세지 출력 구현
Main 클래스 내에 메인 메서드 구현
All 패키지를 domain, controller, view로 분리 Input 필드 타입 변경 및 구조 리팩토링
Output 클래스에 필드 추가, 인스턴스를 사용하도록 설계 변경 Car,RacingGame,Main Output 클래스에 맞춰 설계 변경
ALL 컨벤션에 맞춰 코드 전반적인 인덴트, 접근제어 등 리팩토링
README 구현단위 종류별로 정리하여 작성
ALL 컨벤션에 맞춰 코드 전반적인 인덴트, 접근제어 등 리팩토링
자바 컨벤션에 맞게 상수 필드 추가, 주석 추가
| } | ||
|
|
||
| private static void validateNameLength(String name) { | ||
| if (name == null || name.isEmpty() || name.length() > 5) { |
There was a problem hiding this comment.
if 판별조건을 명시적으로 알 수 있게 메소드로 분리해 보면 어떨까요?
|
|
||
| public void validateInput() { | ||
| List<String> list = splitInputByComma(); | ||
| if (list == null) { |
There was a problem hiding this comment.
Objects.isNull(list) 을 사용 해보면 어떨까요?
https://multifrontgarden.tistory.com/205
| if (list == null) { | ||
| throw new IllegalArgumentException(LENGTH_ERROR_MESSAGE); | ||
| } | ||
| list.stream().forEach(Input::validateNameLength); |
There was a problem hiding this comment.
|
|
||
| public static void play(Input input, Output output) { | ||
| List<String> names = input.splitInputByComma(); | ||
| List<Car> cars = new ArrayList<>(); |
There was a problem hiding this comment.
아래 글을 읽고 일급컬렉션을 적용 해 보세요!
https://jojoldu.tistory.com/412
|
|
||
| public class Car { | ||
| private static final int FORWARD_NUMBER = 4; | ||
| private static int maxPosition = 0; |
There was a problem hiding this comment.
maxPosition을 static 필드로 모든 Car 인스턴스가 공유하게되면 어떤 문제가 있을까요?
예를 들어 자동차 경주가 병렬로 수행되면 어떤 문제가 생기게 될까요?
| } | ||
|
|
||
| public static int generateRandom() { | ||
| return new Random().nextInt(NUMBER_BOUND); |
There was a problem hiding this comment.
아래 글 참고해보세요!
https://withseungryu.tistory.com/1
| import java.util.Scanner; | ||
|
|
||
| public class InputView { | ||
| private static final Scanner s = new Scanner(System.in); |
There was a problem hiding this comment.
축약어는 쓰지 않는게 좋을 것 같아요.
https://developerfarm.wordpress.com/2012/01/30/object_calisthenics_6/
src/main/java/stringadder/Input.java
Outdated
| } | ||
|
|
||
| private String getDelimiter() { | ||
| Matcher m = Pattern.compile(CUSTOM_DELIMITER).matcher(expression); |
There was a problem hiding this comment.
src/main/java/stringadder/Input.java
Outdated
|
|
||
| private void validateInput() { | ||
| if (expression == null || expression.isEmpty()) { | ||
| throw new NullPointerException("식을 입력해 주세요."); |
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| public class Input { |
There was a problem hiding this comment.
아래 글을 읽고 mvc 패턴에 대해 좀더 고민 해 보는건 어떨까요?
https://apeltop.blogspot.com/2019/05/mvc-model-view-controller.html
Scanner 인스턴스명을 scanner로 수정
Main 클래스와 Input, Output 클래스를 MVC 디자인 패턴에 맞게 이동 그에따른 Import 문 수정
Random 값을 매번 다르게 생성하기 위해 generateRandom 메서드의 리턴 랜덤의 시드를 시간으로 설정
이름 길이를 검증하는 validateNameLength 메서드의 if 판별조건인 name == null 과 name.length()>5 조건을 각각 boolean 값을 리턴하는 isNullName 과 isLongerThanFive 메서드로 분리
패턴을 컴파일 하는 메서드의 오버헤드를 줄이기 위해 먼저 커스텀 구분자가 존재하는지 String.matches 메서드를 통해 확인한 뒤 존재하는 경우에만 Matcher 인스턴스를 생성하도록 메서드 설계를 변경
메서드 내에서 사용하는 List<Car> 필드들을 일급컬렉션 Cars, Winners 로 분리
공백과 오타 수정
| Random rand = new Random(); | ||
| rand.setSeed(System.currentTimeMillis()); | ||
| return rand.nextInt(NUMBER_BOUND); |
There was a problem hiding this comment.
DM으로 말씀 드렸던 부분 코멘트로 다시 한번 남겨드려요 ㅜㅜ
해당 부분 확인 확인 결과 currentTimeMillis값이 차이가 나지 않아서 같은 결과가 나오는 것 같아요.
(System.nanoTime()을 사용 하니 다른 값이 나오더라구요.)
그리고 Seed값과 별개로 new Random()을 할때 System.nanoTime을 고려해서 seed값을 생성되고 nextInt처럼 난수를 생성 할때마다 seed값이 바뀌고 있네요!
혼란을 드려서 죄송합니다 😭
| boolean isMove = car.move(rand); | ||
| String log = car.makeLog(); | ||
| OutputView.printLog(log); |
There was a problem hiding this comment.
Model과 View를 분리 해보면 어떨까요?
지금처럼 View가 Model안에서 실행되는게 아니라 Model에게 값을 리턴 받아 View를 그려주면 좋을 것 같아요.
| public String makeLog() { | ||
| StringBuilder log = new StringBuilder(name); | ||
| log.append(" : "); | ||
| for (int i = 0; i < position; i++) { | ||
| log.append("-"); | ||
| } | ||
| return log.toString(); |
There was a problem hiding this comment.
이부분은 View의 로직인 것 같아요. Car의 position에 따라 예를들어 - 로 그릴지 = 로 그릴지는 화면에 보여지는 부분인데 이에따라 Model의 코드가 바뀌는게 맞을지 생각해보면 좋을 것 같아요 :)
| } | ||
| } | ||
|
|
||
| public void moveAllCars() { |
There was a problem hiding this comment.
컨트롤러에서 자동차 경주가 실행되기 위해 호출되는 Cars의 유일한 메소드인데 테스트가 없네요.
아마도 random값때문에 moveAllCars를 호출했을때 모든 자동자가 움직였는지 여부를 체크하기 힘들기 때문 인 것 같습니다.
public boolean move(int random) {
if (random >= FORWARD_NUMBER) {
position++;
return true;
}
return false;
}
현재 Car의 move 메소드가 위와 같은데 랜덤값으로 자동차의 전진 가능 여부를 인터페이스로 추상화 하고 테스트 시에는 추상화 한 부분을 어떤 random값이 들어와도 true로 return되게 구현된 구현체를 넘겨 moveAllCars를 호출 했을때 모든 자동차가 이동 했는지 테스트 해보면 어떨까요?
Strategy pattern을 이용해보세요!
https://limkydev.tistory.com/84
generateRandom 메서드 시드를 nanoTime()으로 수정
Model과 View를 구분하기 위해서, Log를 만드는 메서드들을 모두 OutputView 클래스로 이동 View에서 Model의 정보를 얻기 위해 Getter를 추가 Output 클래스와 Winner 클래스의 기능이 중복되어 하나의 클래스로 병합
Input 클래스를 Names 클래스와 Repeat 클래스로 분리 MVC 패턴에 맞게 Controller가 View와 Model사이의 연결고리 역할을 하도록 설계 변경 View 와 Model 의 역할을 분리하도록, Controller 에서 View 메서드가 호출되도록 변경 변경된 설계에 맞도록 결과를 출력하도록 OutputView 메서드 변경 변경된 설계에 맞도록 테스트가 수행되도록 InputTest, RacingGameTest 메서드 변경
앞서 수정한 프로그램 설계에 따라 적절한 테스트 메서드와 클래스 추가 전략 패턴을 통해 MoveCars 테스트 수행 MoveCarsOnlyFalse는 항상 모든 차들을 전진시키지 않는 전략구현객체이고, MoveCarsOnlyTrue는 항상 모든 차들을 전진시키는 전략구현객체이다. 이를통해 RacingGameTest에서 MaxPosition이 적절하게 수정되는지 테스트가 가능하다.
| @@ -0,0 +1,12 @@ | |||
| package racinggame.domain; | |||
|
|
|||
| public class MoveCarsOnlyTrue implements Strategy { | |||
There was a problem hiding this comment.
전략패턴 적용 위치를 조금 더 고민 해보았으면 좋겠어요 🤔
현재 자동차의 전진 조건은 전진하는 조건은 0에서 9 사이에서 random 값을 구한 후 random 값이 4 이상일 경우 전진하고, 3 이하의 값이면 멈춘다. 이고 전진 조건을 판별하는 로직은 Car클래스 안에 있어요. 따라서 피드백 드린 random값과 상관없이 자동차를 전진 시키려면 Car클래스 안에 있는 판별 로직에 대해 전략패턴을 적용하는 것이 맞을 것 같아요.
현재 코드 상으로는 Strategy의 moveCars를 각각 구현 후 테스트 했기때문에 프러덕션 코드인 RacingGame에 대한 테스트가 되었다고 보기 힘들 것 같아요 ㅠㅠ
| int maxPosition = 0; | ||
|
|
||
| OutputView.printResultFormat(); | ||
| for (int i = 0; i < repeat.getRepeat(); i++) { | ||
| maxPosition = racingGame.moveCars(cars, maxPosition); | ||
| OutputView.printCarsLog(cars); | ||
| } | ||
| Winners winners = cars.makeWinners(maxPosition); |
There was a problem hiding this comment.
maxPosition값을 매번 전달 하기보단 경주가 끝난 자동차들을 기준으로 가장 큰 Position 값을 뽑고 해당 값으로 우승자를 판별 하는 것이 좋지 않을까요?
| } | ||
|
|
||
| public boolean move(int random) { | ||
| if (random >= FORWARD_NUMBER) { |
src/test/java/WinnersTest.java
Outdated
| car.move(6); | ||
| car.move(i); | ||
| i += 3; |
There was a problem hiding this comment.
로직을 수행하지 말고 Cars에 position이 셋팅된 List를 넘겨서 Cars를 만들어 보는 것은 어떨까요?
|
|
||
| OutputView.printResultFormat(); | ||
| for (int i = 0; i < repeat.getRepeat(); i++) { | ||
| maxPosition = racingGame.moveCars(cars, maxPosition); |
RandomGenerator를 Strategy 를 implements 하는 세가지 구현객체로 생성 MoveGenerator는 항상 전진하는 정수를 반환 StopGenerator는 항상 정지하는 정수를 반환 RandomGenerator는 매번 다른 값을 반환하도록 구현 테스트시엔 Move와 Stop의 경우 정상적으로 메서드가 작동하는지 테스트
| for (Car car : cars) { | ||
| if (car.move(generateRandom())) { | ||
| if (car.move(randomGenerator)) { |
| for (Car car : cars) { | ||
| if (car.move(generateRandom())) { | ||
| if (car.move(randomGenerator)) { | ||
| maxPosition = car.getMaxPosition(maxPosition); |
There was a problem hiding this comment.
음... 매번 움직일때 마다 MaxPosition을 선정 해 줄 필요가 있을까요?
| public Cars(List<Car> cars){ | ||
| this.cars = cars; | ||
| } | ||
|
|
||
| public Winners makeWinners(int maxPosition) { |
There was a problem hiding this comment.
외부로 부터 maxPosition을 받아오지말고 아래처럼 내부에 있는 Car list를 통해 maxPosition을 구해도 되지 않을까요?
public Winners makeWinners() {
int maxPosition = getMaxPosition();
List<Car> winners = cars.stream()
.filter(car -> car.isSamePosition(maxPosition))
.collect(Collectors.toList());
return new Winners(winners);
}
private int getMaxPosition() {
return cars.stream()
.mapToInt(Car::getPosition)
.max()
.orElse(0);
}
감사합니다.