[1단계 - 자동차 경주 구현] 제이미(임정수) 미션 제출합니다. #498
Conversation
생성자 추가 및 자동차 유효성 검사
Cars 생성차 추가 및 자동차 이름 쉼표 기준으로 분리
Getter 사용 지양을 위해 Cars에서 처리하도록 수정
public에서 private으로 수정함
단일우승자, 다중우승자
최대 위치 값과 비교하는 메서드 추가
There was a problem hiding this comment.
안녕하세요 제이미 리뷰어 파즈입니다! 1단계 수고하셨습니다. 메서드 분리도 잘 하셨고 이미 1단계 레벨의 요구는 충족이 된 결과물인 것 같습니다. 코멘트 몇 개 남겨놨습니다. 의견을 묻는 코멘트는 그 부분이 잘못됐다 라는 것은 아니고 말 그대로 어떤 생각을 가지고 하셨는지 듣고 싶어서 묻는 것이니 부담 가지지 않으셨으면 좋겠습니다.
-
입력 값 검증의 위치
-> 자동차 이름에 대한 검증은 Car에, 시도횟수에 대한 검증은 InputView에 위치하셨는데요. 사실 정답은 없습니다. 제이미가 판단하기에 달렸습니다. 일단 제 생각을 말씀드리자면, 우선 InputView 라는 클래스가 Car라는 도메인에 대한 정보를 알게 할 것이냐 아니면 단순히 입력만 받는 클래스로 둘 것이냐 부터 정할 것 같습니다. 제이미 코드를 살펴보면inputCarName이라는 메서드가 있지만 메서드 네이밍과 무관하게 단순히scanner.nextLine()으로 입력만 받고 있는 상태입니다.inputTryCount는 입력을 받고 검증까지 하고 있죠. 시도횟수에 대한 검증 로직도 로직이지만 메서드 네이밍을 통해 해당 IntputView 클래스는 Car라는 도메인에 대한 정보를 알고있는 상태의 클래스라고 볼 수 있을 것 같습니다. 현재 InputView는 입력을 받기도 하고 검증의 역할까지 맡고 있는데요, 잘못된 것은 아닙니다. 그렇지만 차 이름에 대한 입력은 이곳에서 검증을 받지 않는 반면, 시도횟수는 이곳에서 검증을 받고 있는 상황입니다. 제이미가 코멘트에 남겨주셨듯이 본인만의 확고한 생각과 뒷받침되는 주장이 있으면 그건 그것대로 정답인 것이라고 생각합니다. 굳이 통일을 하고자 한다면, InputView에서도inputCarName이라는 네이밍을 가지고 있는 것과 같이 이곳에서도 검증하고 Car 객체에서도 2차 검증을 하는 방식으로 가져가도 괜찮을 것 같긴 하네요. 두 번째 경우인 단순히 입력을 받기 위한 클래스로 둔다면 입력만 받는 메서드만 두고, 검증은 컨트롤러 단에서 하던가 컨트롤러에서 validation 관련 클래스를 두고 하던가 할 것 같네요. 이 상황에서도 차 이름에 대한 검증 로직을 Car 객체에 둘 것이냐, validation클래스에 다 때려박을 것이냐 로 갈릴 수 있을 것 같은데요. 장단점이 있을 것 같습니다. validation에 몰아두면 이후 이 레이싱과 관련된 valid 작업을 할 때에 해당 클래스만 보면 된다는 장점이 있을 것이고, 단점은 이런 valid 작업이 많아지면 해당 클래스의 크기가 너무 커지고 객체지향에서 조금 멀어진다는 단점이 있을 수 있겠네요. 모든 것은 트레이드 오프여서 코드 작성자의 판단과 생각이 제일 중요합니다. 제이미가 생각하기에 현재 방식이 적절하다고 여겨진다면 그것대로 오케이 입니다. 만약 이 부분에 대해서 다른 팀원이 다른 의견을 제시를 하고 그것이 납득이 간다면 그 방식으로 변경을 하면 되는 것이구요~. 개인적으로는 InputView에 검증작업을 두진 않을 것 같긴 합니다. -
정확한 랜덤값 테스트란 불가능합니다. 현재 코드에서
Random객체를 사용해서nextInt로 값을 뽑아내고 있는RandomGenerator를 사용하고 있는 상태인데, 이nextInt에서 정확히 랜덤값이 나온다 라는 것 부터 테스트를 해야 사실 맞는 것이죠. 하지만 그것이 불가능 한 상태고 Car는 RandomGenerator의generateNumber()(현 구현에서는 nextInt를 반환하는) 를 받아서 동작하고 있는 상태이니, 인터페이스를 사용하지 않고 바로 클래스만 생성해서generateNumber를 건네주어 테스트를 하자니 랜덤에 대한 테스트이기 때문에 의미가 없어집니다. 그래서NumberGenerator로 추상화를 한 번 하고 테스트 단계에서 하드코딩된 값으로 테스트를 하는 것이죠. Cars 객체의moveResult메서드가 파라미터로 List 타입을 받는다면 하드코딩으로 테스트가 가능하겠지만, 현재는NumberGenerator를 받고 있으니 이것을 테스트 하기 위해서는 추상화가 필요한 것입니다. 이것을 인터페이스가 아닌 클래스로 둔다면 앞에서 언급한 불가능한 랜덤값에 대한 테스트의 오염이moveResult까지 퍼지게 되는 것이죠. 일종의 차단막? 이라고 생각하시면 될 것 같습니다. 언급주신 하드코딩 값 전달은Car의move메서드에서 진행되고 있으니 상관없는 말인 것 같네요. -
이게 사실 Input에 대한 테스트가 아니라 검증에 대한 테스트인 것인데요, InputView가 입력과 동시에 검증까지 하고 있어서 생기는 테스트에 대한 불편함인 것 같습니다.
checkPositiveNumber메서드가 public이면 이런 고민을 할 필요도 없는데 바깥에서 호출하는 메서드가 아니니 public 하기엔 애매하고 그렇죠. 테스트마다InputView를 위한 테스트고 테스트마다 생성이 된다고 문제가 될 것 같다는 생각은 들지 않네요.
| @Override | ||
| public int generateNumber() { | ||
| Random random = new Random(); | ||
| return random.nextInt(9); |
There was a problem hiding this comment.
랜덤 범위 설정에 대해 착각했네요..!
10으로 수정해 두었습니다!
src/main/java/model/Car.java
Outdated
| public boolean checkLocationEqual(int maxLocation) { | ||
| return this.location == maxLocation; | ||
| } |
There was a problem hiding this comment.
check~~ 로 시작하는 네이밍은 valid~~ 로 시작하는 메서드 네이밍과 같이 내부적으로 체크 후 throw를 하는 void 메서드의 느낌이 강한 것 같습니다.
boolean 리턴 메서드는 is, has, can 로 시작하면 더 의미전달이 될 수 있을 것 같습니다.
There was a problem hiding this comment.
is, has, can 등으로 시작하는 것이 의미전달에 있어 확실하겠네요! 말씀해 주신 대로 메서드명을 수정했습니다.
다만, validCarName 메서드의 경우 내부적으로 예외 발생 시 throw로 처리해 주는 void 메서드가 맞아 그대로 두었는데 괜찮을까요?
src/main/java/model/Car.java
Outdated
| import constant.ExceptionMessage; | ||
|
|
||
| public class Car { | ||
| private String name; |
There was a problem hiding this comment.
값이 바뀔 일이 없는 변수들은 final을 통해 수정을 막아주는 것이 좋겠네요.
해당 부분에 대해 수정해 두었습니다.
| private int setTryCount() { | ||
| messageView.printTryCountMessage(); | ||
| try { | ||
| return inputView.inputTryCount(); | ||
| } catch (Exception e) { | ||
| System.out.println(e.getMessage()); | ||
| return setTryCount(); | ||
| } | ||
| } | ||
|
|
||
| private void repeatMoving(Cars cars, int tryCount) { | ||
| messageView.printResultMessage(); | ||
|
|
||
| for (int count = 0; count < tryCount; count++) { | ||
| cars.moveResult(randomNumberGenerator); | ||
| outputView.printResult(cars); | ||
| } | ||
| } |
There was a problem hiding this comment.
run 메서드에서 repeatMoving 메서드를 setTryCount보다 먼저 호출하고 있는데요.
메서드의 순서 또한 호출된 순서로 배치를 하면 코드 가독에 더 도움이 될 것 같습니다.
There was a problem hiding this comment.
모든 메서드들의 배치를 호출 순서에 맞게 수정해 두었습니다.
| } | ||
|
|
||
| public List<Car> getCars() { | ||
| return Collections.unmodifiableList(cars); |
src/main/java/model/Cars.java
Outdated
| public String getWinners() { | ||
| int maxLocation = cars.stream() | ||
| .max(Comparator.comparingInt(Car::getCarLocation)) | ||
| .get().getCarLocation(); | ||
|
|
||
| return cars.stream().filter(car -> car.checkLocationEqual(maxLocation)) | ||
| .map(Car::getCarName) | ||
| .collect(Collectors.joining(", ")); |
There was a problem hiding this comment.
public String getWinners() {
int maxLocation = cars.stream()
.max(Comparator.comparingInt(Car::getCarLocation))
.get().getCarLocation();
return cars.stream().filter(car -> car.checkLocationEqual(maxLocation))
.map(Car::getCarName)
.collect(Collectors.joining(", "));
}와 같이 stream 메서드들을 체이닝해서 사용할 때에는 위치가 일정해야 가독성이 좋습니다.
맥을 사용 중이시라면 cmd+opt+L 로 코드 리포맷을 하시거나 문장 끝에서 shift + cmd + enter 를 하시면 리포맷이 가능합니다.
만약 인텔리제이를 이용해 깃을 사용하고 계신다면
우측에 before commit 옵션들을 사용하시면 더 편리하게 이용가능합니다!
There was a problem hiding this comment.
메서드 체이닝 사용 시 위치에 대해 항상 고민이었는데 해결되었습니다!
위치 정렬 설정이 되어 있지 않아 위치를 맞추지 않는 것이 맞는 것이라 생각하고 있었습니다.
자동 정렬이 될 수 있도록 설정한 뒤 수정해 두었습니다.
src/main/java/model/Cars.java
Outdated
| public void moveResult(NumberGenerator randomNumberGenerator) { | ||
| for (Car car : cars) { | ||
| car.moveByRandom(randomNumberGenerator.generateNumber()); | ||
| } | ||
| } |
There was a problem hiding this comment.
moveResult에서 Result 는 어떤 의미로 사용하신건지 의도가 궁금합니다!
There was a problem hiding this comment.
해당 메서드 명에서 Result는 실행 결과에 따른 움직임 결과라는 뜻을 담았습니다.
해당 메서드 명으로 기능에 대한 의미 전달이 불명확할까요?
There was a problem hiding this comment.
result가 붙어서 result에 대한 return을 할 것 같은 네이밍인 것 같습니다. 기본적으로 메서드 네임은 명사가 아니라 동사로 두면 좋습니다!
테코블에 https://tecoble.techcourse.co.kr/post/2020-04-26-Method-Naming/ 이와 관련해서 간단하게 적어둔 글이 있어서 가져와봤습니다~
| public class RandomNumberGenerator implements NumberGenerator { | ||
| @Override | ||
| public int generateNumber() { | ||
| Random random = new Random(); |
There was a problem hiding this comment.
지역 변수가 아닌 객체 변수로 선언되도록 수정했습니다.
| @@ -0,0 +1,19 @@ | |||
| package view; | |||
|
|
|||
| public class MessageView { | |||
There was a problem hiding this comment.
MessageView와 OutputView의 구분은 어떤 생각을 가지고 결정하셨는지 듣고 싶습니다!
There was a problem hiding this comment.
OutputView의 경우 Controller를 통해 파라미터로 값을 전달받아 특정 포맷에 맞추어 출력하는 메서드들을 모아두었습니다.
그리고 OutputView의 경우 전달되는 값 없이 특정 문구를 출력하는 메서드들을 모아두었습니다.
혹시 이렇게 분리하는 것이 크게 의미가 없을까요?
There was a problem hiding this comment.
아니요 제이미가 생각하기에 이 방식이 괜찮다고 생각했다면 괜찮습니다! 정확히 어떤 기준을 가지고 분류했는지 단순히 궁금했습니다~
src/main/java/view/InputView.java
Outdated
|
|
||
| public int inputTryCount() { | ||
| try { | ||
| int tryCount = Integer.parseInt(scanner.nextLine()); |
There was a problem hiding this comment.
아뇨..!
아직 Scanner에 대한 이해가 부족해 nextInt 메서드가 존재하는지 몰랐습니다.
String을 받아 정수로 변환하는 것보다 nextInt()를 통해 int 타입으로 받는 것이 더 좋다고 생각해 수정해 두었습니다.
There was a problem hiding this comment.
앞으로 새롭게 사용할 클래스가 굉장히 많을 것인데요, 특히 실무 들어가면 처음보는 클래스 투성일겁니다. 새로운 클래스를 접하면 일단 .만 딱 쳐서 어떤 메서드가 있는지 쭉 살펴보는 습관을 들이시면 굉장히 도움이 되실겁니다!
Car, Cars, ExeptionMessage, InputView 객체 변수를 final을 통해 불변으로 선언 및 초기화할 수 있도록 수정함
isEmpty와 isBlank를 통해 값이 입력되지 않은 경우를 중복 확인하는 문제에 대해 수정함 isEmpty는 isBlank 메서드를 통해 함께 체크되기 때문
getter 메서드에 객체를 알아보기 위한 접두어를 붙이지 않아도 되므로 수정
maxLocation을 반환하는 메서드와 우승자를 반환하는 메서드로 분리
기존 문자열을 입력받아 int 형으로 변환하는 코드를 nextInt()를 통해 입력 받은 후 바로 int 형으로 리턴될 수 있도록 수정함
자동차 이름을 아무 것도 입력하지 않은 경우에 대한 예외 메시지 추가
|
안녕하세요 파즈! 입력 값 검증 위치에 대해 이번 미션에서 결정한 방법은 다음과 같습니다.
아직 좀 더 생각해 볼거리지만, 일단 이번 미션에는 위와 같이 저만의 규정을 만들어보았습니다. |
| } | ||
| } | ||
|
|
||
| private void repeatMoving(Cars cars, int tryCount) { |
| String carNames = inputView.inputCarNames(); | ||
| validation.validateCarNames(carNames); | ||
| return new Cars(carNames); |
There was a problem hiding this comment.
2단계 하시면서 여유가 생기신다면 아래의 요구사항도 추가로 반영해서 제출해주세요!
pobi,pobi,pobi의 입력이 현재로서는 가능하지만, 동일 이름일 시에는 불가능하게끔pobi, crong, honux입력 시에 앞뒤 공백은 제외하고 이름을 측정할 수 있게끔
자동차 경주 1단계 페어 프로그래밍 PR 제출합니다.
아래는 미션을 수행하며 생긴 의문들에 대해 정리해 보았습니다.
아직 많이 부족하지만, 리뷰 잘 부탁드립니다.
감사합니다!
좀 더 생각해 봐야 할 문제
1. 입력 값 검증의 위치
자동차 이름과 시도 횟수 입력 값에 대한 검증을 도메인 객체와 InputView에서 각각 검증을 진행했습니다.
위와 같이 검증을 수행한 경우 여러 클래스에서 검증 기능을 수행하게 됩니다.
자동차 이름의 경우 객체에서 이름을 저장할 때 검증을 해주는 것이 좋다고 판단해 도메인에서 각각 필요한 검증을 수행해 주었습니다.
그리고 시도 횟수의 경우 숫자로 변환했을 때 0을 초과하는지만 판단하면 되기에 한 가지 검증을 위해 따로 클래스를 생성하는 것은 과하다 생각하여 InputView 내부에서 수행해 주었습니다.
위 내용으로 작업을 진행하면서 InputView에서 검증하는 것이 적합한가에 대한 의문이 들었습니다.
또한, 검증 기능을 수행하는 코드들이 Validation 클래스를 통해 묶는 것이 좋은지,
자동차 이름 등 객체 생성 시 생성자에서 멤버 변수를 초기화해 주며 검증하는 것이 좋은지 궁금합니다.
2. 인터페이스를 사용한 랜덤 값 결과에 따른 Test
이번 미션에선 랜덤 값 결과에 따라 다른 결과를 반환하는 기능에 대한 테스트를 진행했습니다.
테스트를 수행 시 원하는 랜덤 값을 직접 지정하기 위해 NumberGenerator 인터페이스를 사용했습니다.
그리고 Test 클래스 내에, 메서드 오버라이딩을 통해
TestNumberGenerator를 만들어 원하는 값을 랜덤 값으로 넘겨주었습니다.이와 같은 경우 NumberGenerator를 사용하는 것이 어떤 장점이 있는지 궁금합니다.
하드코딩된 값을 전달하는 것보다 NumberGenerator 인터페이스를 생성하는 것이 더 효율적인 작업인지 궁금합니다.
3. 입력 값을 이용한 테스트
InputView 클래스에 대한 테스트 수행 시 콘솔로부터 입력 값을 받아와야 합니다.
InputStream을 통해 입력 문자열을 설정하고
System.setIn()으로 값을 넘겨주었습니다.이때 발생한 문제점은 다음과 같습니다.
테스트를 진행할 때마다 InputView 객체를 생성해주어야 했습니다.
만약 Test 클래스의 인스턴스 변수로 InputView 객체 생성해 사용하게 된다면 아래와 같은 오류를 확인할 수 있었습니다.
이러한 이유로 ParameterizedTest 수행 시에는 반복적으로 지역 변수로 객체를 생성하게 되는데, 이를 해결할 방법이 있는지 궁금합니다.