[Step4] 로또(2등) #1676
Merged
[Step4] 로또(2등) #1676
Conversation
This file contains 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
|
tag @kyucumber |
kyucumber
requested changes
May 26, 2021
Comment on lines
8
to
9
| private Ticket lastWeekWinningTicket; | ||
| private int bonus; |
There was a problem hiding this comment.
Ticket을 상속 혹은 위임해서 winningTicket에 보너스 번호를 추가하고 로또의 제약이나 기능을 그대로 사용할 수 있지 않을까요?
There was a problem hiding this comment.
생각해보니, 그렇게 할수 있겠네요. 한번 시도해보겠습니다!
| .map(ticket -> ticket.numbers().matchCountWith(lastWeekWinningTicket.numbers())) | ||
| .filter(matchCount -> matchCount >= MATCH_THREE_NUMBER) | ||
| .collect(Collectors.groupingBy(LotteryMatchType::fromInteger, summingInt(a -> 1))))); | ||
| .map(ticket -> ticket.numbers().getMatchTypeWith(lastWeekWinningTicket.numbers(), bonus)) |
There was a problem hiding this comment.
규칙 4: 한 줄에 점을 하나만 찍는다.
위 객체지향 생활 체조 원칙 관점에서 봤을 때 아래와 같이 호출되는건 어떨까요?
Suggested change
| .map(ticket -> ticket.numbers().getMatchTypeWith(lastWeekWinningTicket.numbers(), bonus)) | |
| .map(ticket -> ticket.getMatchTypeWith(lastWeekWinningTicket.numbers(), bonus)) |
Comment on lines
32
to
39
| if (isBonusMatchCount(bonus, matchType)) { | ||
| return LotteryMatchType.FIVE_MATCH_WITH_BONUS; | ||
| } | ||
| if (isNotContainsBonus(bonus, matchType)) { | ||
| return LotteryMatchType.FIVE_MATCH; | ||
| } | ||
| return matchType; | ||
| } |
There was a problem hiding this comment.
규칙 2: else 예약어를 쓰지 않는다.
if를 2개 사용한다고 위 원칙이 지켜진다고 보긴 어려울 것 같습니다.
enum의 values()는 순서를 보장했던 것 같은데 그렇다고 하면 FIVE_MATCH가 우선적으로 반환되어 아래와 같이도 사용할 수 있을 것 같아요.
다만 조금 혼란을 줄 순 있으니 여러 방법들을 한번 고민해보시면 좋을 것 같습니다.
Suggested change
| if (isBonusMatchCount(bonus, matchType)) { | |
| return LotteryMatchType.FIVE_MATCH_WITH_BONUS; | |
| } | |
| if (isNotContainsBonus(bonus, matchType)) { | |
| return LotteryMatchType.FIVE_MATCH; | |
| } | |
| return matchType; | |
| } | |
| if (isBonusMatchCount(bonus, matchType)) { | |
| return LotteryMatchType.FIVE_MATCH_WITH_BONUS; | |
| } | |
| return matchType; | |
| } |
There was a problem hiding this comment.
저도 이부분에 대해서 고민이 많았습니다. values()가 순서를 보장하지만, enum의 순서를 개발자가 조금이라도 변경하면- 사이드 이펙트가 발견될 수 있을 확률이 높은 것 같아, 위와같이 수정했습니다.
조금 더 고민해보겠습니다!
- WinningTicket 티켓에서 당첨 확인이 될 수 있도록 책임 위임 - Ticket.numbers().values() 이 부분을 Ticket.numbers() 수정
kyucumber
approved these changes
May 26, 2021
Comment on lines
7
to
10
| public class WinningTicket { | ||
|
|
||
| private final Ticket ticket; | ||
| private final int bonus; |
|
리뷰 해주셔서 감사합니다 : ) 마지막 단계 도전합니다ㅎㅎ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
안녕하세요. 김규남 리뷰어님!
벌써 3번째이네요.
보내주신 피드백 덕분에 코드를 시야가 넓어졌습니다.
이번에는 커밋의 수는 2개뿐입니다. 조금더 나누서 커밋해야되는건 아니였을까 싶습니다.
이렇게 된 이유는, 2등을 위한 기능 추가 코드는 많지 않았습니다. 그러나, 2등 기능을 추가하게 되면서 넓은 범위의 리펙토링을 수행하게 되었습니다.
특히,
matchCount()와 같이 중복된 수의 갯수로 Map 에서 Type을 가져오는 로직을 조금 더 직관적으로 이해할 수 있도록 시도하니, 코드 변경라인이 많아졌습니다.리뷰 잘부탁드리겠습니다. 🏋️♀️