[자동차 경주] 이현제 미션 제출합니다. #104
Conversation
구현 기능 목록, 프로그래밍 요구사항, 라이브러리 작성
전진 조건인 무작위 값 4 이상의 조건을 만족시키기 위해 `pickNumberInRange()` 사용
This reverts commit 70006e7. 커밋 메세지 변경을 위한 revert
This reverts commit 26ac6c3. 커밋 컨벤션이 적용되지 않았으므로 다시 revert
- 우승자는 한 명 이상일 수 있음 - 쉼표(,)로 우승자 여러 명 출력
- 자동차 각각의 이름은 5자 이하
- ,기준 문자열 분할 - 공백 제거
carMap의 선언을 함수 실행과 일치시켰으며 인자로 자동차 이름을 처리하는 과정을 새로운 코드로 내보냈습니다.
- moveForwardIfVaild() : 전진 조건에 따라 전진 - printRaceResult() : 각 실행 별 전진 출력 - printAllRaceResults() : 모든 실행 출력
- ilterWinners() : 우승자 선별 - printWinners() : 모든 우승자 출력
기존 Constants에서 Strings로 파일명 교체했습니다.
| } | ||
|
|
||
| fun moveForwardIfValid(carMap: MutableMap<String, Int>, key: String, value: Int) { | ||
| if (isMoveForwardValid()) { |
There was a problem hiding this comment.
isMoveForwardValid()라는 검증 메서드를 호출하고 있지만, 위로 한참 스크롤해야 하는 점에서 응집도가 떨어지는 것 같습니다. 관련성이 높으면 붙어있는 것도 좋은 방법이라고 생각합니다!
| } | ||
| } | ||
|
|
||
| fun moveForwardIfValid(carMap: MutableMap<String, Int>, key: String, value: Int) { |
There was a problem hiding this comment.
함수명을 보면 '유효할 경우 앞으로 이동하는 작업'이라는 점을 알 수 있긴 하지만, '무엇이' 유효한지에 대해서 빠진 게 아쉽다고 생각합니다. 결국 가독성을 떨어뜨릴 수 있다고 합니다..! 만약 Car 클래스의 메서드로 moveForwardIfValid였다면 대충 차를 앞으로 보내겠구나 생각을 하겠지만, 함수로만 구현되었기 때문에 매개변수와 함수내용을 보고 로직으로 판단해야 해서 가독 비용이 높아진 것 같습니다..!
| } | ||
|
|
||
| @Test | ||
| fun `통합 테스트 공백 제거`() { |
There was a problem hiding this comment.
테스트 함수명을 정할 때도 어떤 역할을 하는 함수인지 명확히 해주시면 더 좋다고 알고 있습니다. 어떤 입력인지 적은 건 좋았다고 생각합니다! 하지만 여기에 무엇을 했을 때, 어떤 결과를 기대하는지까지 적는 테스트 네이밍 룰도 있다고 하는데 개발자마다 적절한 테스트 네이밍 룰을 찾아보는 것도 좋을 거 같아요!
| val isCarNameValidLength = { carName: String -> carName.length <= 5 } | ||
| val isMoveForwardValid = { pickNumberInRange(0, 9) >= 4 } | ||
| val isWinnerValid = { carMap: MutableMap<String, Int>, count: Int -> count == carMap.values.maxOrNull() } | ||
|
|
||
| fun main() { |
There was a problem hiding this comment.
리뷰 요청에 클래스 필요성에 대한 요청사항이 있어서 글 남깁니다...!
클래스는 메서드 행위의 주체, 프로퍼티 상태의 주체라서 현제님께서 작성하신 함수들이 클래스로 적절하게 묶였을 때, 그 클래스 명만 보고도 어떤 역할을하는 객체인지 쉽게 예측 가능하다는 장점이 있습니다.
클린 코드 책을 보면 코드를 작성할 때 좋은 글을 쓰려는 것처럼 하라는 말이 있더라구요...! 문장을 잘 써도 문단으로 묶는 게 중요한 것처럼, 클래스로 객체의 행위와 상태를 적절히 묶는 것도 중요하다고 합니다!
|
2주차 고생하셨습니다~
클래스로 구현해야하는 이유는 당연히 없습니다. |
읽고보니 문장이 이상하긴 하네요 🤣 함수 분리를 하면서 입력 안내와 입력 요청의 분리가 필요한가 생각이 들었습니다. 이런 고민들이 코드 작성 내내 존재해서 PR 소개글에 명시했습니다. 코드 리뷰 감사합니다! 👍 |
There was a problem hiding this comment.
함수의 역할 분리도 잘 되어있는 것 같고, 코드에 대해서는 전체적으로 잘 읽히게끔 가독성 좋게 짜신 것 같습니다!
현제님의 리뷰 요청에 대해 제 나름의 답변을 남겨보자면,
-
중간에 이미 push된 커밋 메시지를 변경하기 위해 몇 번 revert 했습니다.
로그가 깔끔해지지 못했는데 다른 방법이 있었을까요?
-> 이미 push된 커밋에 대해서는 --force 옵션을 통해 강제로 원격 저장소의 커밋을 변경하는 것밖에 방법이 없는 것으로 알고 있습니다. 개인 프로젝트라면 위 방식으로 원격 저장소의 커밋을 수정해도 별 무리가 없겠지만, 협업 상황이라면 사용을 최대한 지양해야 하는 것으로 알고 있습니다! -
커밋은 적절한 간격으로 커밋됐나요?
-> 사람들마다 의견이 갈리는 부분이지만, 커밋을 작은 단위로 작성하는 것은 VCS의 존재 의의와 일맥상통한다고 생각합니다. 그 부분에서 커밋 간격은 적당하신 것 같습니다! -
함수로만 구현했는데 클래스를 생성하여 구현해야되는 이유가 있을까요?
-> 지금처럼 작은 규모의 프로젝트라면 클래스를 분리하는 의미가 작을 수 있지만, 역할 분리의 관점에서 확실한 이점이 있다고 봅니다! 이는 코드의 가독성과도 직결될 수 있는 부분이라 생각합니다.
| package racingcar | ||
|
|
||
| object Strings { | ||
| const val MESSAGE_INPUT_CAR_NAME = "경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)" | ||
| const val MESSAGE_INPUT_ATTEMPT_NUMBER = "시도할 횟수는 몇 회인가요?" | ||
| const val MESSAGE_OUTPUT_RESULT = "실행 결과" | ||
| const val MESSAGE_OUTPUT_WINNER = "최종 우승자 : " | ||
| const val MESSAGE_EXCEPTION_CAR_NAME_LENGTH = "자동차 이름은 5자 이하만 가능합니다." | ||
| const val MESSAGE_EXCEPTION_INPUT_CAR_NAMES = "자동차 이름은 값이 없거나 공백이 아니여야 합니다." | ||
| const val MESSAGE_EXCEPTION_INPUT_ATTEMPT_NUMBER = "입력 횟수는 정수값이여야 합니다." | ||
| } |
|
|
||
| fun processCarNames(input: String): List<String> { | ||
| val carList = input.split(",").map { it.trim() } | ||
| require(carList.all { it.isNotBlank() }) { Strings.MESSAGE_EXCEPTION_INPUT_CAR_NAMES } |
PR 스케치 🎨
검사 조건은 람다식으로 가급적 코드 맨 앞으로 배치하였습니다.
Strings파일은 다국어 처리를 염두에 두고 생성하였습니다.명시되지 않은 예외 처리는 단위테스트와 통합 테스트를 하며 추가하였습니다.
코드 가독성보다
함수(또는 메서드)가 한 가지 일만 하도록 최대한 작게 만들어라.라는 프로그래밍 요구 사항에 집중하여 작성했습니다.trim()함수를 통해 자동차 이름 입력 중의 공백을 제거하였습니다.carName등의 제약조건은 함수보다 변수처럼 표현하고 싶어 람다식을 사용했습니다.예외처리는
require()을 이용해 구현했습니다.리뷰 요청
중간에 이미 push된 커밋 메시지를 변경하기 위해 몇 번 revert 했습니다.
로그가 깔끔해지지 못했는데 다른 방법이 있었을까요?
커밋은 적절한 간격으로 커밋됐나요?
커밋 컨벤션과 네이밍은 어떤가요?
변수 및 함수 네이밍은 어떤가요?
함수로만 구현했는데 클래스를 생성하여 구현해야되는 이유가 있을까요?