Open
Conversation
dompoo
reviewed
Nov 20, 2024
| public GuessResult match(GuessNumber guessNumber) { | ||
| int strike = checkStrike(guessNumber); | ||
| int temporaryBall = checkTemporaryBall(guessNumber); | ||
| return GuessResult.from(strike, temporaryBall - strike); |
There was a problem hiding this comment.
조그만 부분이지만, temporaryBall이 조금 와닿지 않습니다! 아래 temporaryBall - strike까지 봐야 이해할 수 있었어요. 다른 메서드명과 변수명을 시도해보면 좋을 것 같습니다.
Comment on lines
+41
to
+48
| private Set<Integer> createRandomDistictNumbers() { | ||
| Set<Integer> distinctNumbers = new HashSet<>(); | ||
| while(distinctNumbers.size() < Rules.BASE_NUMBER_COUNT) { | ||
| int randomNumber = Randoms.pickNumberInRange(1, 9); | ||
| distinctNumbers.add(randomNumber); | ||
| } | ||
| return distinctNumbers; | ||
| } |
There was a problem hiding this comment.
이 책임은 다른 객체가 가지면 좋을 것 같습니다! 다름 도메인 로직이라고 생각되어서 컨트롤러가 갖는게 아쉬워용
Comment on lines
+18
to
+27
| @Override | ||
| public String toString() { | ||
| StringBuilder stringBuilder = new StringBuilder(); | ||
| if (nothing(stringBuilder)){ | ||
| return stringBuilder.toString(); | ||
| } | ||
| appendBall(stringBuilder); | ||
| appendStrike(stringBuilder); | ||
| return stringBuilder.toString(); | ||
| } |
There was a problem hiding this comment.
toString을 통해 출력하는 부분이 dto의 책임일까? 라는 고민은 저도 항상 합니다 ㅠㅠ.
제 개인적인 생각으로는 dto는 정말 데이터를 전달만 해야하므로 toString을 오버라이딩하는 것이 올바르지 않다고 생각하고, 말장난같지만 Command나 Domain으로 이 객체를 다룬다면 괜찮지 않을까? 라는 생각이에요.
다만 어떤 방법을 써도 출력하는 역할이 여러 곳으로 분리되는 것은 동일해요.
| } | ||
|
|
||
| private boolean nothing(StringBuilder stringBuilder) { | ||
| if (ball + strike == 0) { |
There was a problem hiding this comment.
조그만 부분인데, 이렇게 검증하는 것이 약간 아이디어성 로직이라고 생각하는데, 오히려 코드가 좀 더 길더라도 ball == 0 && strike == 0으로 검증하는 것이 더 읽기 좋고 안정적인 코드라고 생각합니다 ㅎㅎ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
고민한 부분
객체지향적으로 문제를 해결하고자 했습니다.
스트라이크 / 볼 판별시, 메소드를 10줄로 줄이기 위해 노력했습니다.
아쉬운 부분
컨트롤러에 유효성 검증 로직이 드러나 이 로직을 도메인에 넣으면 좋았겠네요.
MatchResult에 toString 로직이 담겨있는데 이를 출력을 위한 Dto로 둬도 괜찮을지 고민이 드네요.