코드 리뷰용 pull request입니다. #246
Conversation
Docs: 기능 목록 추가
| state = EXIT | ||
| } | ||
|
|
||
| private fun showExecuteMessage() { |
There was a problem hiding this comment.
출력에 대한 책임을 다른 클래스로 위임하여 Controller의 역할을 분리하는 것이 좋아 보입니다.
함수(또는 메서드)가 한 가지 일만 하도록 최대한 작게 만들어라라는 말이 추가된 프로그래밍 요구 사항에 언급 되어 있기 때문에 이를 지켜서 분리를 해보시면 좋겠습니다.
보통 입력을 받아올 때 출력할 메세지는 InputView, 결과를 출력할 때는 OutputView가 출력을 담당합니다.
View는 웹으로 치면 하나의 웹 페이지라고 볼 수도 있을텐데, 간단하게 UI라고 보면 되겠네요.
고로 InputView는 '입력을 위한 UI를 출력하고 입력을 받아오는 것'까지를 하나의 역할이라고 생각하면 될 것 같습니다.
There was a problem hiding this comment.
고로
InputView는 '입력을 위한 UI를 출력하고 입력을 받아오는 것'까지를 하나의 역할이라고 생각하면 될 것 같습니다.
@zxcev 최대한 분리했다고 생각했는데 이렇게 분리가 가능할 줄은 몰랐습니다. 각 클래스가 세분화되고 보다 명확해짐에 따라 유지보수가 편리해지는 등의 여러 이점들을 가져갈 수 있을것 같습니다. 다른 부분들도 한 가지 일만 할 수는 없는지 점검해보겠습니다. 감사합니다.
| val user = User() | ||
| showFinishMessage() | ||
| val finishNumber = user.getFinishNumber() | ||
| if (finishNumber == 1) restart() |
There was a problem hiding this comment.
매직 넘버 사용을 지양하는 것이 좋겠습니다.
FinishNumber를 좀 더 적절한 이름의 enum으로 만드는 것이 좋아보입니다.
상태를 정수로 표현할 때는 1, 2 등의 단순한 값으로 아무런 의미를 찾을 수 없으나,
enum으로 표현할 경우, FinishNumber.PLAYING, FinishNumber.FINISH 등으로 표현할 수 있기 때문에 보다 직관적이며, 추후 상태가 늘어나더라도 대응이 쉽게 가능하며 전체적인 상태를 enum 파일 내에서 모두 확인 가능하다는 장점이 있습니다.
또한 지금은 1이 '재시작 하라'는 의미이지만, 나중에 '종료하라'로 변경된다면, 모든 1을 찾아서 변경해야 하는데, enum은 그럴 필요가 없어집니다.
There was a problem hiding this comment.
enum으로 표현할 경우,FinishNumber.PLAYING,FinishNumber.FINISH등으로 표현할 수 있기 때문에 보다 직관적이며, 추후 상태가 늘어나더라도 대응이 쉽게 가능하며 전체적인 상태를enum파일 내에서 모두 확인 가능하다는 장점이 있습니다.
@zxcev 생각하지 못했던 부분이었습니다. '매직 넘버', 'enum'을 적절하게 사용해서 가독성을 높이고, 대응 가능한 코드를 작성할 수 있도록 공부하고 적용해보겠습니다. 감사합니다.
| package baseball | ||
|
|
||
| // 유효성 검사(사용자 숫자) | ||
| fun isValidateInputStringForGame(input: String) { |
There was a problem hiding this comment.
InputManager보다 InputValidator라는 클래스명이 좀 더 적절해 보입니다.
There was a problem hiding this comment.
InputManager보다InputValidator라는 클래스명이 좀 더 적절해 보입니다.
@zxcev naming도 많이 고민하는 부분이라서 저에겐 꼭 필요한 리뷰입니다. 감사합니다.
| package baseball | ||
|
|
||
| class GameManager { | ||
| private val computerNumberList = mutableListOf<Int>() |
There was a problem hiding this comment.
ComputerNumber, UserNumber를 일급 컬렉션화 하여 도메인 객체로 만든 뒤, 비즈니스 로직을 직접 실행하도록 계층을 나눠주는 것이 좋아 보입니다.
There was a problem hiding this comment.
ComputerNumber,UserNumber를 일급 컬렉션화 하여 도메인 객체로 만든 뒤, 비즈니스 로직을 직접 실행하도록 계층을 나눠주는 것이 좋아 보입니다.
@zxcev '일급 컬렉션화', '도메인 객체', '비즈니스 로직', '계층 나누기' 모두 공부하고 적용해보도록 하겠습니다. 보다 좋은 코드를 작성할 수 있도록 알려주셔서 감사합니다.
| ## ✏️ 과제 진행 요구 사항 | ||
|
|
||
| - 미션은 [kotlin-baseball](https://github.com/woowacourse-precourse/kotlin-baseball-6) 저장소를 Fork & Clone해 시작한다. | ||
| - **기능을 구현하기 전 `docs/README.md`에 구현할 기능 목록을 정리**해 추가한다. |
There was a problem hiding this comment.
@Iwillbeagood 빠뜨린 요구사항이 있었네요ㅠㅠ 체크해주셔서 감사합니다! 👍
| fun isDuplicated(input: String): Boolean { | ||
| input.forEachIndexed { index, num -> | ||
| var count = 0 | ||
| for (j in index..input.lastIndex) { |
There was a problem hiding this comment.
| for (j in index..input.lastIndex) { | |
| for (j in input.indices) { |
indices라는 메서드를 사용하면 간단해 집니다.
There was a problem hiding this comment.
@Iwillbeagood 말씀해주신대로 하면 보다 간결한 코드를 작성할 수 있을것 같아요. 감사합니다! 👍
No description provided.