3단계 - 로또(2등) by yooncheolkim · Pull Request #4131 · next-step/java-lotto

yooncheolkim · 2025-04-01T16:03:18Z

리뷰 부탁드립니다.

wooobo

3단계 잘 진행해주셨습니다!

몇가지 코멘트 남겨놓았어요~
확인 후 재요청 부탁드립니다!

그럼 화이팅입니다!

wooobo · 2025-04-02T16:16:51Z

+import java.util.List;
+
+public class BonusNumber {
+    private int number;


Suggested change

private int number;

private final int number;

final이면 좋을것 같네요 😄

wooobo · 2025-04-02T16:25:46Z

 import java.util.stream.IntStream;

 public class ShuffleStrategy implements LottoNumberStrategy {
+    final static int MAX_RANGE = 45;


지금 구조도 문제 없다고 생각하지만, 고민해볼만한 포인트가 있다면 Lotto도메인 스스로도 해당 범위를 지키는것이 더 안전하지 않을까 생각이 되긴합니다 🤔
어떻게 생각하시나요?

wooobo · 2025-04-02T16:26:46Z

-    private final List<Integer> numList;
    public static final int PRICE_PER_ONE = 1000;

+    private final List<Integer> numList;


규칙 3: 모든 원시값과 문자열을 포장한다.

Integer->LottoNumber라는 객체가 있다면 어떨까요?

wooobo · 2025-04-02T16:29:17Z

                break;
-            case 6:
-                hit6Count++;
+            case FIRST:


Rank를 잘만들어주셔서 새로운 등수가 추가되더라도 확장에 유리할것 같네요! 😄
그렇다면, LottoStatics가 필요할지 혹은 Rank와 협력관계를 형성할수 있지 않을지 고민해보면 좋을것 같아요~

지금은 Rank가 변경되면 LottoStatics도 함께 추가 되어야하는 이슈가 있는것 같아요 🤔

wooobo · 2025-04-02T16:30:25Z

+        @Override
+        public void print(LottoStatics lottoStatics) {
+            System.out.println(this.getCountOfMatch() + "개 일치"
+                + "(" + this.getWinningMoney() + "원)-"


원,개는 view의 영역이라고 생각이듭니다!

wooobo · 2025-04-02T16:36:44Z

+    }
+
+    @Test
+    void 보너스번호가_Lotto와_일치하면_true() {


이 테스트는 현재BonusNumber에서 isMatch를 검증하고 있는데,
사실상 로또 번호에 특정 숫자가 포함되는지를 확인하는 책임은 Lotto 또는 LottoBundle이 가져가는 방향은 어떨까요?

bonusNumber.isMatch(lotto)보다는
lotto.contains(bonusNumber) 형태가 더 직관적이지 않을까요?

테스트 이름도 lotto에 포함된 숫자 확인 가능 으로 한다면 어떨까요?

wooobo · 2025-04-02T16:38:01Z

        assertThat(highProfitStatics.isProfit()).isTrue();
    }
+
+    private List<Lotto> createLottoListWithRank(List<Integer> winningNums) {


테스트 픽스처 올바르게 사용하기
한번 읽어보셔도 좋을것 같습니다!

yooncheolkim · 2025-04-07T17:08:47Z

@wooobo
안녕하세요.

LottoNumber를 만들어보니, 리뷰 작성해주신 부분들에 대해서, 공감되는 부분들이 있었습니다.
리뷰 반영하여, 재리뷰 요청드립니다.

테스트 픽스처 올바르게 사용하기
이 부분을 읽고 보고 수정하려고하였는데,, 어떤부분이 적절하지 못한지 설명해주실수 있으실까요?
팩토리 메소드를 만들때, 인자를 주어야하는 부분인지,, 궁금합니다.

wooobo

3단계도 잘진행해주셨습니다!

피드백에 대해 고민해주셔서 감사합니다~
몇가지 피드백 남겨 놓았어요~

이만 merge진행하겠습니다!
다음 단계도 화이팅입니다!

wooobo · 2025-04-08T09:30:02Z

+import static domain.LottoGenerator.DRAW_NUMBER_COUNT;
+
+public class WinningNumbers {
+    private List<LottoNumber> winningNumberList;


winningNumberList는 Lotto 타입과 다른걸까요?

-List 자료구조를 이름으로 명시하는게 좋을까요?
Set으로 변경되면 메소드명도 변경되어야할까요?

이럴경우는 일반화된 이름이면 좋지 않을까요?

@wooobo
변수명을 보고, 어느정도 타입을 유추할수 있는 이름이 좋다고 생각하여, 이렇게 네이밍하였습니다.
winningNumbers? 이런 네이밍이 좋을지,, 고민되네요;

winningNumbers 복수개념을 가진다면 말씀하신게 더 좋다고 생각합니다 😄

wooobo · 2025-04-08T09:37:34Z

+        List<Lotto> lottoList = this.createLotto();
+        LottoStatistics lottoStatistics = new LottoStatistics(lottoList);
+
+        assertThat(lottoStatistics.getCountByRank(Rank.FIRST)).isEqualTo(1);
+        assertThat(lottoStatistics.getCountByRank(Rank.SECOND)).isEqualTo(1);
+        assertThat(lottoStatistics.getCountByRank(Rank.THIRD)).isEqualTo(0);
+        assertThat(lottoStatistics.getCountByRank(Rank.FOURTH)).isEqualTo(1);
+        assertThat(lottoStatistics.getCountByRank(Rank.FIFTH)).isEqualTo(1);
+        assertThat(lottoStatistics.getCountByRank(Rank.MISS)).isEqualTo(0);


테스트 픽스처 올바르게 사용하기
이 부분을 읽고 보고 수정하려고하였는데,, 어떤부분이 적절하지 못한지 설명해주실수 있으실까요?
팩토리 메소드를 만들때, 인자를 주어야하는 부분인지,, 궁금합니다.

적절하지 못하여 코멘트 남긴것은 결코아니었습니다 ㅎ

조금 고민되는것은,

반환 객체 상태 불명확

createLotto 메소드가 생성하는 Lotto 객체들이 실제 어떤 상태인지 한눈에 파악하기 어려움.

테스트 의존성 문제

여러 테스트가 createLotto 메소드의 반환 상태에 강하게 의존하고 있음.

만약 createLotto 내부 로직 또는 Lotto 객체의 상태가 변경되면, 일부 테스트는 실패하고 다른 테스트는 성공하는 불일치 현상이 발생할 가능성이 있음.

@wooobo
말씀해주신 방향으로 createLotto 사용자 입장에서 어떤 Lotto를 만드는지 아는 방향이 좋은것 같습니다.

wooobo · 2025-04-08T09:55:53Z

+    }

-        boolean matchBonus = bonusNumber.isMatch(this);
+    public void matchRank(WinningNumbers winningNumbers) {


public Rank matchRank(WinningNumbers winningNumbers) { int countOfMatch = this.getMatchCount(winningNumbers); boolean matchBonus = this.isMatchBonus(winningNumbers.getBonusNumber()); return Rank.valueOf(countOfMatch, matchBonus); }

Rank를 내부상태로 가져가야하는지 의문이들긴해요 🤔
lotto.matchRank(winningNumbers); 처럼 후로직으로 처리할거라면 해당 시점에서 Rank를 받는게 더 분명할것 같다는 생각이 들어서요!

@wooobo
Rank를 받는다는 의미는 외부에서 Rank를 만들어서 넣어주는게 더 좋다는 말씀이신거죠..?(setter를 이용?)

기능 구현, 리팩토링

9e57455

wooobo requested changes Apr 2, 2025

View reviewed changes

리뷰 반영

75c0e89

wooobo approved these changes Apr 8, 2025

View reviewed changes

wooobo merged commit 59296b4 into next-step:yooncheolkim Apr 8, 2025

wooobo mentioned this pull request Apr 10, 2025

4단계 - 로또(수동) #4181

Merged

Conversation

yooncheolkim commented Apr 1, 2025

Uh oh!

wooobo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yooncheolkim commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wooobo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wooobo Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yooncheolkim commented Apr 7, 2025 •

edited

Loading

wooobo Apr 8, 2025 •

edited

Loading