로또 미션 [sjiwon] #1
Conversation
- 로또 범위 검증 - 로또 전체 사이즈 검증 - 로또 중복 요소 검증
- LottoStatistics 테스트 시 random에 의한 의존성 제거
- Lotto -> UserLottos(UserLotto)로 중간 매핑 계층 제거
SangBeom-Hahn
left a comment
SangBeom-Hahn
left a comment
There was a problem hiding this comment.
1차로 리뷰 남깁니다 추가로 궁금한 점 디코 질문방에 올리겠습니다
|
|
||
| // then | ||
| assertAll( | ||
| () -> assertThat(lottoWinningMachine.getWinningLotteryNumbers()).containsExactlyInAnyOrderElementsOf(winningNumbers), |
There was a problem hiding this comment.
lottoWinningMachine.getWinningLotteryNumbers())에서 당첨 금액 List의 순서가 변할 것 같지는 않은데 containsexactlyinanyorderelementsof을 사용하신 이유가 있나요?
There was a problem hiding this comment.
제가 임의로 6, 5, 4, 3, 2, 1을 넣어줬을때 전 LottoMachine 자체도 정렬된 Lotto를 가짐을 보장하기 위해서 order와는 상관없는 validation을 했습니다
| public enum WinningRank { | ||
| FIRST(6, List.of(true, false), 2_000_000_000, "6개 일치"), | ||
| SECOND(5, List.of(true), 30_000_000, "5개 일치, 보너스 볼 일치"), | ||
| THIRD(5, List.of(false), 1_500_000, "5개 일치"), | ||
| FOURTH(4, List.of(true, false), 50_000, "4개 일치"), | ||
| FIFTH(3, List.of(true, false), 5_000, "3개 일치"), | ||
| NONE(0, List.of(), 0, "NONE..."), | ||
| ; |
There was a problem hiding this comment.
hasBonus를 SECOND만 true로 하고 나머지는 false로 하면 되지 않나요?
There was a problem hiding this comment.
이거는 제가 당첨 개수를 보너스 번호까지 합해서로 생각해서 잘못 작성한거같네요. 수정할게요
There was a problem hiding this comment.
public enum WinningRank {
FIRST(6, false, 2_000_000_000, "6개 일치"),
SECOND(5, true, 30_000_000, "5개 일치, 보너스 볼 일치"),
THIRD(5, false, 1_500_000, "5개 일치"),
FOURTH_WITH_BONUS(4, true, 50_000, "4개 일치"),
FOURTH(4, false, 50_000, "4개 일치"),
FIFTH_WITH_BONUS(3, true, 5_000, "3개 일치"),
FIFTH(3, false, 5_000, "3개 일치"),
NONE(0, false, 0, "NONE..."),
;다시 생각해서 케이스를 나눠봤는데 위와 같이 나오네요
- matchCount = 보너스 번호 관련없이 당첨번호에 대한 match
- matchCount 6은 오류
- matchCount 4, 3은 정상
Fix
public enum WinningRank {
FIRST(6, List.of(false), 2_000_000_000, "6개 일치"),
SECOND(5, List.of(true), 30_000_000, "5개 일치, 보너스 볼 일치"),
THIRD(5, List.of(false), 1_500_000, "5개 일치"),
FOURTH(4, List.of(true, false), 50_000, "4개 일치"),
FIFTH(3, List.of(true, false), 5_000, "3개 일치"),
NONE(0, List.of(false), 0, "NONE..."),
;| final Lotto lottoA = new Lotto(Arrays.asList(1, 3, 2, 4, 5, 6)); | ||
| final Lotto lottoB = new Lotto(Arrays.asList(44, 1, 10, 23, 18, 6)); |
There was a problem hiding this comment.
Arrays와 List의 차이는
- 파라미터와의 참조여부
- null 가능 여부
- 요소 추가 가능여부
로 알고 있습니다. Arrays로 바꾼 이유가 무엇인가요?
There was a problem hiding this comment.
Lotto에서 sort가 이루어지는데 List.of는 immutable이기 때문에 UnsupportedOperationException이 발생해서 Arrays.asList로 적용했습니다
| @DisplayName("보너스 번호가 당첨 번호에 중복되면 예외가 발생한다") | ||
| void throwExceptionByBonusNumberIsNotUnique() { | ||
| // given | ||
| final List<Integer> winningNumbers = Arrays.asList(1, 2, 3, 4, 5, 6); |
There was a problem hiding this comment.
역시 Arrays로 바꾼 이유가 궁금합니다.
| @ParameterizedTest | ||
| @MethodSource("invalidRange") | ||
| @DisplayName("로또 번호의 범위가 1..45 이외라면 예외가 발생한다") | ||
| void throwExceptionByBaseballIsNotInRange(List<Integer> baseballs) { |
There was a problem hiding this comment.
baseballs를 사용하셨네요. 의도하신 건가요?
There was a problem hiding this comment.
이전 Baseball이랑 로직 똑같아서 테스트 복붙했는데 잘못 넣었네요 ㅋㅋㅋㅋ
| public static LottoStatistics of( | ||
| final LottoWinningMachine lottoWinningMachine, | ||
| final UserLotto userLotto | ||
| ) { | ||
| return new LottoStatistics(lottoWinningMachine, userLotto); | ||
| } |
There was a problem hiding this comment.
생성자를 private으로 하고 of 메서드로 생성하는 이유가 있나요?
There was a problem hiding this comment.
이름 붙이려고 정적 팩토리 메소드를 쓰긴하는데 LottoStatistics의 of는 네이밍을 수정해야겠네요. 기존 팩토리 관례를 그대로 따랐네요
| THIRD(5, List.of(false), 1_500_000, "5개 일치"), | ||
| FOURTH(4, List.of(true, false), 50_000, "4개 일치"), | ||
| FIFTH(3, List.of(true, false), 5_000, "3개 일치"), | ||
| NONE(0, List.of(), 0, "NONE..."), |
There was a problem hiding this comment.
enum에 none은 따로 요구사항을 추가하신 건가요?
There was a problem hiding this comment.
그냥 당첨안된 케이스들 구분하려고 넣은거긴합니다
No description provided.