[1단계 - 자동차 경주] 페드로(류형욱) 미션 제출합니다. #700
Conversation
- 이름의 길이 초과 시 예외 발생 - 이름들 중 중복 발생 시 예외 발생 - 이름 중 영어가 아닌 문자 포함 시 예외 발생
- 이동한 자동차가 없을 경우 출력 메서드 추가
- ValidatorTest 에 안쓰는 임포트문 제거
- 이동 내역 출력 시 줄바꿈 수정
- 이름 목록을 String[] 에서 List<String> 으로 변경
| } | ||
| } | ||
|
|
||
| private boolean getCarNames() throws IOException { |
There was a problem hiding this comment.
메서드 이름은 getCarNames() 인데 boolean 타입을 반환하는게 어색하네요.
get~는 Java에서 값을 가져오는 메서드의 컨벤션으로 사용하고 있어서,
메서드의 기능을 바꾸거나 반환 값을 변경하는 것이 적절해보입니다.
There was a problem hiding this comment.
get~ 이 게터의 get과 혼동될 여지가 있어서 어색한 것인지, 아니면 메서드의 주된 역할이 이름을 입력받는 것인데, 상태 값을 반환하는 것이 어색한 것인지 궁금합니다.
입력 받는 함수에서 성공 여부를 반환하고 있기에 메서드의 역할이 두 가지 (read + check) 인 것 같아서 조금 찜찜하긴 했어요😅
C의 scanf 같은 함수들이 readBytes 값을 반환하는 것과 유사하게, C스타일의 컨벤션인가 싶기도 하고요..
만약 후자라면, 재 입력을 구현할 때 플래그 값을 반환하지 않고 어떻게 구현할 수 있을까요? 잘 떠오르지 않아 질문 남깁니다.
There was a problem hiding this comment.
둘다입니다 하나씩 볼까요?
전자: getter와 혼동될 수 있다.
후자: 메서드의 역할이 이름 입력을 받는 것인데, 상태값을 반환한다.
-
getter와 혼동될 수 있다.
Java 진영에서 getter는getXXX처럼 사용하는 것이 컨벤션입니다.
누군가 페드로가 만든 getCarNames 메서드를 썼는데 carNames를 반환해주지 않고 boolean을 반환한다면 내부 구현을 까봐야하는 번거로움이 생기겠죠. -
메서드의 역할을 드러내지 못한다.
저는 메서드 이름 짓는 것을 가장 중요하게 생각합니다. 작성해주신 메서드 이름을 직역하면
getCarNames :차 이름을 가져온다.
메서드를 실행하는 사람은 차 이름을 가져오는 행위를 기대할텐데, boolean 값이 나오고 있습니다.
현실로 비유하자면, 이름이 뭐에요? 물어봤는데예,아니요답을 하는것과 마찬가지입니다.
그렇다면 1번과 마찬가지로 내부구현을 까봐야합니다.
결국 질문을 두가지로 나눠서 하셨지만 컨벤션을 왜 지켜야 하는지, 메서드의 이름을 왜 명확하게 가져가야하는지 이유는 하나입니다.
개발을 혼자하지 않기 때문에 다같이 읽을 수 있는 규격에 맞춰서 개발하는 것이 중요합니다.
그렇기 때문에 각 언어에 맞는 convention을 알고 따르는 것이 유리합니다.
플래그 값이 왜 필요했는지 한번 고민해보시면 좋을 것 같아요.
혹시 메서드가 두가지 기능을 하고 있지는 않나요?
현재 단순히 이름을 읽어오는 역할만 하고 있나요?
|
|
||
| private boolean getCarNames() throws IOException { | ||
| try { | ||
| OutputView.printlnInputName(); |
There was a problem hiding this comment.
페드로는 OutputView 와 InputView 를 어떤 기준으로 분리하셨나요?
There was a problem hiding this comment.
단순히 콘솔에 출력하는 부분은 모두 OutputView에서, 사용자의 입력을 받아야 하는 부분은 InputView에서 담당하는 것으로 생각했습니다.
| public static void printPosition(CarGroup carGroup) { | ||
| System.out.println(carGroup.toString()); | ||
| } |
There was a problem hiding this comment.
toString()은 어떻게 사용하는 메서드일까요?
앗 직접 호출한 것은 실수였습니다..ㅎㅎ
There was a problem hiding this comment.
출력을 담당하는 클래스가 도메인 객체를 알고 있어야 할까요?
PR 본문에서 질문드렸던 것 처럼, 도메인 지식을 뷰가 모르고 있는 것이 좋다고 생각합니다.
그렇다면 출력 클래스에서는 String 혹은 List<String> 내지는 Map 등과 같이 출력에 필요한 정보만을 받게 될 텐데,
- 이러한 정보들을 추출하여 적절한 자료구조로 전달하는 추가 소요가 발생하는 것은 불가피한 것인지
- 컨트롤러 단에서 이를 처리해야 할 지, 아니면 각 객체의 메서드에서 처리하는 것이 좋을지
에 대한 의문이 남습니다.
질문이 조금 두서없는 것 같아 예를 들자면,
현재 CarGroup은 Car를 래핑한 일급 컬렉션으로 List<Car> findWinners() 메서드를 가집니다.
코멘트 주신 코드 부분과 유사하게, OutputView.printWinnerList()는 List<Car>를 전달받고 있어 도메인 지식(Car)를 알게 됩니다.
이를 수정하기 위한 하나의 예시로 printWinnerList(List<String> winnerNames) 형태로의 수정을 고려해 볼 수 있을 것 같습니다.
winnerNames 을 구축하여 전달하기 위해 컨트롤러에서 CarGroup.findWinners()를 호출하여 반환받은 List<Car>를 다시 순회하며 우승자 이름 리스트를 만들거나, CarGroup 객체 내에 우승자들의 이름 리스트를 반환하는 메서드를 추가로 구현해야 하는 소요가 생기는데, 두 방법 모두 그다지 깔끔해 보이지 않습니다..
There was a problem hiding this comment.
2단계 리팩터링을 하면서 컨트롤러의 역할이 무엇인지 한번 고민해보시면 1,2번에 대한 고민이 해결될 수 있을 것 같아요.
어떤 부분이 깔끔해보이지 않았을까요?
| private int findMaxPosition() { | ||
| return cars.stream() | ||
| .mapToInt(Car::getPosition) | ||
| .max() | ||
| .orElse(0); |
| public List<Car> findWinners() { | ||
| int maxPosition = findMaxPosition(); | ||
| return cars.stream() | ||
| .filter((car -> car.getPosition() > 0 && car.getPosition() == maxPosition)) |
There was a problem hiding this comment.
모두 0만큼 움직이면 승자가 없는 구조인가요?ㅋㅋㅋㅋ
해당 메서드를 Car 에서 만들어주면 어떨까요?
There was a problem hiding this comment.
모두 0만큼 움직이면 승자가 없는 구조인가요?ㅋㅋㅋㅋ
맞습니다 적어도 한 놈은 움직여야 게임 진행이 됐다라고 생각하고 구현했어요ㅎㅎ
우승자 판별 메서드를 Car에서 만들어준다는 건 사실 정확히 어떤 의미로 말씀하신 건지 파악하지 못했습니다.
만약 Car 내의 코드를 통해 우승자 판별 기능을 구현하는 것이라면 커스텀 정렬 등을 활용해야 할 듯 한데, 정렬 기준이 Car 내부에 명시되어 있으면 나중에 '우승자'를 판별하는 기준이 변경되었을 때 Car 자체를 수정해야 한다는 단점이 있을 것 같습니다. 이 이유로 그룹 내의 우승자를 판별하는 것은 CarGroup 의 책임이라고 생각했는데 이 부분은 어떻게 생각하시나요?
There was a problem hiding this comment.
제가 리뷰를 남길때 정확한 코드라인을 못찝어드렸네요ㅎㅎ
아래 메서드를 말한 것이었습니다.
.filter((car -> car.getPosition() > 0 && car.getPosition() == maxPosition))
페드로가 생각하는 우승자 찾는 로직인데 검증하는 위치가 CarGroup에서 이뤄지고 있어요.
우승자 판별은 CarGroup의 몫이지만 Car이 우승자인지 확인하기 위해서 getter로 값을 가져오고 있네요.
꼭 필요한 getter였을까요?
There was a problem hiding this comment.
6481876 에서 getter를 사용하지 않고 Car 에게 질의하도록 수정하였습니다!
| @Override | ||
| public String toString() { | ||
| return this.name + " : " + "-".repeat(this.position); | ||
| } |
There was a problem hiding this comment.
출력에 대한 로직이 도메인까지 스며들어있네요.
출력 형태를 -가 아니라 +로 변경해달라는 요구사항이 생겼을 때 저는 view 패키지를 먼저 찾아볼 것 같아요.
| if (winners.isEmpty()) { | ||
| OutputView.printNoWinner(); | ||
| return; | ||
| } |
페드로가 이해하고 있는 MVC 패턴에서의 컨트롤러의 역할이 맞습니다.
저는 페어와 토론을 나눴던 것처럼 정답이 없는 문제라고 생각합니다.
입력 재시도 로직이 코드를 지저분하게 만드는 요소이죠...ㅎㅎ |
- 반복 구조는 `read~()`, 실제 입력은 `doRead~()` 로 네이밍
|
안녕하세요 아서, 꼼꼼히 리뷰해 주셔서 감사합니다! 다만 재 입력 로직에서 원래 구현은 이러한 형태였는데요, private void readCarNames() throws IOException {
boolean isValid = false;
while (!isValid) {
isValid = doReadCarNames();
}
}
private boolean doReadCarNames() throws IOException {
try {
names = NameParser.parse(InputView.inputNames());
// 검증 로직()
} catch (IllegalArgumentException e) {
return false;
}
return true;
}
private List<String> readCarNames() throws IOException {
List<String> validatedNames = new ArrayList<>();
while (validatedNames.isEmpty()) { // 1
validatedNames = doReadCarNames();
}
return validatedNames;
}
private List<String> doReadCarNames() throws IOException {
List<String> names = new ArrayList<>();
try {
names = NameParser.parse(InputView.inputNames());
// 검증 로직()
} catch (IllegalArgumentException e) {
return new ArrayList<>(); // 2
}
return names;
}유효성 검증을 통과했는지를 판단하기 위한 |
| while (!isValid) { | ||
| isValid = doReadMoveCount(); | ||
| } |
There was a problem hiding this comment.
이 부분이 좋은 힌트가 될 수 있을 것 같아요!
isValid는 어떤걸 의미하나요?
doReadMoveCount는 해당 값을 충족시켜주나요?
| private void readMoveCount() throws IOException { | ||
| boolean isValid = false; | ||
| while (!isValid) { | ||
| isValid = doReadMoveCount(); | ||
| } |
There was a problem hiding this comment.
readMoveCount가 아무 값을 반환해주지 않으니
moveCount가 어디 저장되는지 확인하려면 내부 구현을 살펴봐야겠네요.
안녕하세요 아서🙂 우테코 BE 6기 페드로입니다.
아직 많이 부족하지만 리뷰 잘 부탁드리겠습니다!
코드 리뷰에 더해, 미션 진행 중 생겼던 의문점들에 대한 질문들을 정리해 보았습니다. 시간 되실 때 조언 주시면 감사하겠습니다.
View에서 인자로 Model을 전달받아도 괜찮은 것인지
OutputView를 보시면 출력을 위해 모델 객체를 인자로 전달받고 있습니다. 컨트롤러의 역할이 모델과 뷰를 분리한 후, 이를 이어주는 다리 역할을 하는 것으로 알고 있는데, 이러한 구조에서 뷰가 모델에 대해(인자의 형태로라도) 알고 있어도 괜찮은 것인지 잘 모르겠습니다.View의 역할과 책임
View의 역할과 책임의 범위에 대해 의문이 들었습니다. 예를 들어, 이번 미션에서 이름을 입력(
woo,te,co) 받고 구분자(,) 를 기준으로 분리하는 책임은 어느 객체에게 있는 것이 자연스러울까요?저는 View 객체를 사용자가 볼 수 있는 UI로서, html의
<form>과 같은 태그와 유사하게 사용자로부터 입력을 받고, 이를 로직에서 사용할 수 있도록 적절한 구조로 가공하는 책임 역시 뷰에 있다고 생각했습니다.반면 페어는
InputView에서는 "입력"에 집중하고, 파싱 관련 로직은 별도의NameParser객체에서 수행하는 것이 낫겠다고 판단하였습니다. 프리코스 진행 당시 두 접근 모두 타당한 면이 있다고 생각하였고, 매 미션마다 다르게 구현했던 기억이 있습니다. 이 주제에 대해 아서의 의견을 듣고 싶습니다.사용자 입력 오류 발생 시 재 시도 로직 구현에 관해
사용자의 입력에 오류가 있을 경우, 다시 시도하도록 구현하였습니다.
이는 프리코스 진행 중에도 들었던 의문인데, 재귀함수로 구현하게 되면 잘못된 입력이 계속 반복될 시 함수의 콜스택이 무한정 쌓이는 문제가 있고 반복문으로 구현 시 현재 구현과 같이 입력받는 메소드를
while문으로 감싸고 있는 다른 메소드를 작성하게 되어 중복 코드가 발생합니다.입력 재 시도 로직을 깔끔하게 구현하기 위한 팁이 있으신지 궁금합니다.
감사합니다!