1단계 미션 제출합니다.#3
Conversation
(cherry picked from commit 0131a41a85f371596998cea5d55d8412f769ef98)
uijin-j
left a comment
uijin-j
left a comment
There was a problem hiding this comment.
메서드명, 오타 등을 제외하곤 크게 수정 요청 드릴게 없네요!👍🏻
리뷰하면서 많이 배웠습니당 🙇🏻♀️ 특히 Stream 활용한 부분들이 인상깊었습니다!
There was a problem hiding this comment.
만약 상속, 인스턴스화를 할 필요가 없는 클래스라면 final 키워드를 추가해서 명시적으로 불필요한 인스턴스 생성, 클래스 확장을 막아도 좋을 것 같습니다!
| try { | ||
| do { | ||
| commend = Commend.of(InputView.inputMenu()); | ||
| final int gameId = baseBallGameController.gameStart(baseBallNumberGenerator); |
There was a problem hiding this comment.
로컬 변수에 final 키워드를 붙이는건 처음봤네요! 불변성도 보장되고 가독성도 더 좋은 것 같습니다!👍🏻
혹시 final 키워드를 쓰신 이유가 따로 있을까요? 저한텐 조금 생소해서 여쭤봅니다!!
There was a problem hiding this comment.
말한대로 불변성 보장 떄문이에요.
코틀린을 사용하면 val, var로 사용하게 되는데 그걸 그대로 자바에도 넣으려고 했습니다
| try { | ||
| boolean isFinished = true; | ||
|
|
||
| while (isFinished) { |
There was a problem hiding this comment.
isFinishied가 true일 때, 계속 추측을 하는게 조금 어색하게 느껴지는 것 같아요!
finish라는 단어가 게임이 끝났는지?(= 추측을 성공했는지?)를 의미하는 것처럼 느껴져서 그런 것 같습니다!
There was a problem hiding this comment.
아 단어를 잘못썻네요. isInprogress라고 해야겠네요 ㅎㅎ
| OutputView.printExitMessage(); | ||
| } | ||
|
|
||
| private static void gameInProgress(final int gameId) { |
There was a problem hiding this comment.
메서드명에 동사가 없고 명사로 되어 있어서 현재 진행 중인 게임을 어떻게 하는건지 바로 느낌이 오지 않는 것 같습니다!
play나 start같은 동사를 추가해도 좋을 것 같아요!
There was a problem hiding this comment.
executeGame 이라는 단어로 해봤어요.
| this.gameRepository = gameRepository; | ||
| } | ||
|
|
||
| public int gameStart(final BaseBallNumberGenerator baseBallNumberGenerator) { |
There was a problem hiding this comment.
별거 아니지만.. 다른 메서드는 동사+명사인데, 얘는 명사+동사라 순서를 바꿔도 좋을 것 같습니당!
| public static Number valueOf(final int value) { | ||
| return numbers.stream() | ||
| .filter(number -> number.sameValue(value)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException("1부터 9까지의 숫자만 입력 가능합니다.")); | ||
| } |
There was a problem hiding this comment.
얘는 Number 객체에 있어도 되지 않을까요?
There was a problem hiding this comment.
이게 어떤 말인지 이해를 못했습니다.
해당 팩터리는 캐싱용으로 쓰는거라 Factory를 만들지 말고 Number에게 Factory의 책임과 역할을 주는게 좋겠다 라고 하시는걸까요?
There was a problem hiding this comment.
Number객체 안에 정적 팩터리 메서드 형태로 둬도 될 것 같아서 말한 거였습니다!
Number 객체의 생성 책임을 Number에 줄지, 따로 책임을 갖는 객체(BaseBallNumberFactory)를 만들지는 개발자의 선택같긴 하네요!
There was a problem hiding this comment.
아... 근데 그렇게 되면 Number에서 BaserBallNumberFActory의 numbers를 가지고 있어야 해서 애매할거 같아서 ㅎㅎ
Factory가 valueOf를 가지고 잇는게 어색하지 않다고 생각한거 같아요!
| public static Number valueOfIndex(final int index) { | ||
| return numbers.get(index); | ||
| } | ||
|
|
||
| public static List<Number> getNumbers() { | ||
| return numbers; | ||
| } |
There was a problem hiding this comment.
BaseBallNumberFactory라는 클래스명만 보면 야구 숫자를 만들어 주는 것 같은데, valueOfIndex(), getNumbers()라는 메서드명만 보면 index가 왜 나오는 건지, 그리고 getNumbers()의 반환값이 야구 숫자로 허용가능한 숫자들의 List인지 예상하기 조금 어려운 것 같아요!
Number 클래스와 역할이 비슷한 것 같기도 하고, 해당 로직이 BaseBallNumberShuffleGenerator를 위해 필요하다면 해당 클래스 안으로 옮겨도 괜찮을 것 같아요!
물론 저의 개인적인 생각이라 참고만 해주시면 좋을 것 같습니당!!
There was a problem hiding this comment.
BaseBallNumberShuffleGenerator에 대해서 필요한것도 있지만 실제 캐싱 역할을 하는것도 있어요.
getter이다 보니 어색하지 않다고 생각했는데... 이건 좀더 생각해봐야겠네요.
There was a problem hiding this comment.
확실히 고민이 되는 부분이긴 하네요..
BaseBallNumberShuffleGenerator에 private static으로 두는것도 방법 중 하나일 것 같습니다!
그런데 추후 해당 메서드가 다른 클래스에서도 필요할수도 있으니 이대로 두는 것도 괜찮을 것 같습니다!
There was a problem hiding this comment.
이대로 두는게 더 좋을거 같아서 이대로 두고 다음 단계에서 볼께요 ㅋㅋㅋ
| final int gameId = baseBallGameController.gameStart(() -> List.of( | ||
| new Number(1), new Number(2), new Number(3))); |
There was a problem hiding this comment.
Mokito 없이 어떻게 할까 고민했었는데, 이 방식 넘 좋은데요?? 👍🏻👍🏻
|
|
||
| assertAll( | ||
| () -> assertThat(baseBallNumbers).hasSize(3), | ||
| () -> assertThat(baseBallNumbers).doesNotHaveDuplicates() |
There was a problem hiding this comment.
doesNotHaveDuplicates() 라는게 있다니.. 배워갑니당!!🙇🏻♀️
2 participants
No description provided.