-
Notifications
You must be signed in to change notification settings - Fork 452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[1단계 - 자동차 경주 구현] 프린(남기범) 미션 제출합니다. #668
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 프린 :) 리뷰어 러너덕입니다.
명료하고 정갈하게 작성된 코드네요 💯 테스트까지 꼼꼼하게 잘 작성해주셨어요.
다음 미션 진행해주시면 될것같습니다.
읽으면서 궁금했던 부분들 간단히 리뷰 남겨두었는데, 천천히 확인해주시면 될것같아요~
@@ -5,3 +5,34 @@ | |||
## 우아한테크코스 코드리뷰 | |||
|
|||
- [온라인 코드 리뷰 과정](https://github.com/woowacourse/woowacourse-docs/blob/master/maincourse/README.md) | |||
|
|||
## 요구사항 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
기능별로 깔끔하게 잘 정리되었네요
}); | ||
} | ||
|
||
public List<String> findWinners(Cars cars) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cars.findMaxPosition 기능은 일급컬렉션인 cars 가 맡도록 하셨군요 :)
findWinners 메서드도 마찬가지로 cars가 맡도록 하면 어떤가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cars가 맡고 있다가 우승자는 서비스에서 판단하는게 맞지 않나? 싶어서 다시 바꾸게 되었는데 cars가 하는게 더 좋을 것 같네요 행위를 한 곳에서 관리할 수 있는 일급 컬렉션의 장점도 살릴 수 있구요! 감사합니다😁
return Arrays.stream(carNames) | ||
.map(Name::new) | ||
.map(Car::new) | ||
.collect(collectingAndThen(toList(), Cars::new)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 이렇게도 묶을 수 있군요 ㅎㅎ 👍
} | ||
|
||
public void moveCars(Cars cars) { | ||
cars.getCars() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8칸 indent를 쓰시는군요?!
4칸 indent 설정에 대해서 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 ㅎㅎ 우테코 코드 스타일이었군요
잘 적용된 것 같고, 그대로 사용해주시는게 좋을것같아요.
제 개인적인 선호를 잘못 말씀드렸네요 :)
@@ -0,0 +1,27 @@ | |||
package enums; | |||
|
|||
public enum Delimiter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enum을 유용한 방식으로 사용하셨네요 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰가 달리기 전에 PR 코멘트 작성을 끝내려고 했는데 조금 늦었네요,,ㅎㅎㅎ 한 번 읽어봐주시면 정말 감사하겠습니다!
} | ||
|
||
public void moveCars(Cars cars) { | ||
cars.getCars() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}); | ||
} | ||
|
||
public List<String> findWinners(Cars cars) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cars가 맡고 있다가 우승자는 서비스에서 판단하는게 맞지 않나? 싶어서 다시 바꾸게 되었는데 cars가 하는게 더 좋을 것 같네요 행위를 한 곳에서 관리할 수 있는 일급 컬렉션의 장점도 살릴 수 있구요! 감사합니다😁
내용을 꼼꼼하게 정리해주셨네요 👍 잘 읽어보았습니다.
현재 문제 상황에서 택한 방식도 자연스러운 방식이고, 이번 과제에서는 충분한 것 같아요.
좋은 토론 과정을 거치셨네요 :)
이것도 전제하는 바에 따라 다르지 않을까 싶네요. 지금 구현도 어색하지 않아요. |
의견 남겨주셔서 감사합니다 😊 러너덕덕분에 가려운 부분이 많이 해소되었어요
어떤 방식이 테스트하기에 더 적합할까? 이거에 초점을 두고 얘기를 나눴는데 두 방식 모두 괜찮다고 생각해서 얘기가 길어졌었어요 |
안녕하세요! 우테코 6기 프린입니다 코드 리뷰 받는 것만큼 설레는 일이 없는데 귀한 시간 저에게 써주셔서 감사합니다 😊
페어와 규칙 정하기
저와 @마크 둘 다 페어 프로그래밍이 처음이라서 구현하기 전에 몇 가지 규칙을 세웠어요
1. commit message convention
기본적인 컨벤션은 title, body, footer로 나뉘지만 body와 footer는 제외했습니다
대신 커밋의 크기를 작게 가져가서 type과 subject로도 구별이 되도록 커밋을 작성하기로 했어요
2. 테스트
테스트 메서드명을 한글로 작성하는 것과
@DisplayName
어노테이션을 사용하는 것 중 호불호가 많이 갈리더라구요저희 둘 다 한글 설명과 영어로된 메서드명 두개를 고민하는 것이 비효율적이라고 생각해서 메서드명을 한글로 작성하게 되었습니다
메서드명은 테스트 코드를 모두 포함할 수 있도록 상세하게, 그리고 동사로 작성했습니다
또한 given, when, then 기반으로 테스트 코드를 작성해서 테스트 대상과 방향을 정리하고 가독성을 향상시키고자 했어요
3. MVC 패턴 적용하기
제리의 MVC 패턴을 보고 이번 페어 프로그래밍에 적용해보기로 했어요
5가지를 지키면서 구현하려고 노력했습니다
고민한 점
1. 입력 값은 어디서 검증해야 할까?
이번 미션에서는 자동차 이름과 시도 횟수를 입력받고 있어요
자동차 이름은 5자 이내, 시도 횟수는 0보다 커야한다는 도메인 유효성은 도메인 객체를 설계해서 도메인 내에서 유효성을 검증하도록 했어요
하지만 입력받은 자동차 이름이
,
로 구분되어 있지 않다던지, 시도 횟수가 숫자가 아니라던지 등의 입력값에 대한 검증은 어디서 해야 좋을지 고민이 되었어요-> 객체를 올바르게 생성하는지 ui 로직에 대한 테스트가 필요할 것 같고, ui 계층에서 객체를 생성하는 것은 적절하지 않는 책임으로 보여짐
-> 입력값에 대한 검증은 inputView에서 처리하고 값을 넘겨주는 것이 적절한 책임이라고 생각
따라서 제가 선택한 것은 inputView에서 검증 후 도메인 객체를 생성하기 전 단계 (ex. String -> int)까지만 수행하고 controller로 값을 리턴하도록 구현했어요
다른 좋은 방법이 있을 지 러너덕의 코멘트를 듣고 싶습니다!
2. 공통된 입력값 유효성과 각각의 입력값에 대한 유효성 검증
입력값이 비어있거나, 공백이 포함되어 있다는 공통된 유효성과 자동차 이름은
,
로 구분되고 시도 횟수는 숫자여야 하는 개별적인 유효성 검증이 필요했어요이 부분은 빈약?하지만 탬플릿 메서드 패턴을 적용해서 해결해 보았습니다
3. 랜덤 숫자를 생성하는 로직은 어떻게 구현할까?
랜덤 클래스는 테스트할 때 핸들링할 수 없어서 테스트가 용이하기 위해 어떻게 구현할지 고민이 되었어요
페어인 마크는 위처럼 람다식을 활용하자고 의견을 내었고
저는 인터페이스를 구현해서 테스트할 때 테스트용 구현체를 만드는게 어떨지 얘기했어요
마크의 람다식 방법은 간결하고 가독성이 좋지만 재사용이 불가능하고
저의 인터페이스 구현 방법은 재사용할 수 있지만 관리할 클래스가 많아진다는 두 방법 사이의 trade off가 있었어요
긴 토론 끝에 저의 방법으로 구현했지만 어떤 것이 더 좋은 선택일까하는 의문은 아직 조금 남은 상태에요
구현이 다 끝나고 생각했을 때 이번 미션의 요구사항이 작기 때문에 마크의 방식이 더 적절하지 않았을까?하는 고민이 들었어요
러너덕의 생각은 어떠신가요??
4. 조건에 따라서 자동차를 움직이게 하는 것은 누가 책임저야 할까?
자동차가 앞으로 전진하는 것은 자동차의 책임이라고 생각했기 때문에 자동차에게 메시지를 전달하도록 구현했어요
요구사항 중에서 숫자가 4 이상일 때 자동차가 전진한다는 조건이 있는데 이러한 조건도 자동차가 알아야할까? 자동차는 단순히 앞으로 가기만 하는 책임만 있으면 되지 않을까? 하는 의문이 들었어요
그래서 조건을 충족했을 때 자동차에게 전진하라는 메시지를 전달하는 책임은
RaceService
가 하도록 구현했지만 제가 너무 과하게 책임을 분리하는 것은 아닐까하는 고민이 생겼어요러너덕이 보셨을 때는 어떤지 궁금합니다!