[로또] 킬리안(박명규) 미션 제출합니다. by kilian-develop · Pull Request #1 · talmood/java-lotto

kilian-develop · 2024-05-19T10:16:12Z

java-lotto

비즈니스 요구 사항

로또 구입 금액을 입력하면 구입 금액에 해당하는 로또를 발급해야 한다.
로또 1장의 가격은 1000원이다.
로또 구입 금액은 0보다 커야 한다.
로또는 1개이상 구매해야한다.
지난 주 당첨 번호는 "1, 2, 3, 4, 5, 6" 같은 형식이어야 한다.
보너스 번호는 당첨 번호에 포함되면 안된다.

구현 기능 목록

- 구입 금액 입력 - 당첨 번호 입력 - 보너스볼 입력

haero77

킬리안 역시 제일 먼저 제출해주셨군요 👍
간단히 리뷰 남겨봤습니다 :D

지난 번과 비슷한 코멘트가 있을 수 있는데, 킬리안의 코드 작성에 대한 근거를 더 명확하게 세울 수 있도록 재차 확인하고 싶은 것으로 이해해주시면 고맙겠습니다.(그 사이에 새로운 근거/의도가 추가되었을지 또 모르죠..!)

haero77 · 2024-05-20T13:17:07Z

src/main/java/controller/LottoSimulator.java

+    public void run() {
+        InputView inputView = new ConsoleInputView();
+        OutputView outputView = new ConsoleOutputView();
+
+        PurchaseInput purchaseInput = inputView.inputPurchaseAmount();
+        PurchaseAmount purchaseAmount = purchaseInput.toPurchaseAmount();
+
+        ManualPurchaseCountInput manualPurchaseCountInput = inputView.inputManualPurchaseCount();
+        PurchaseCount manualPurchaseCount = manualPurchaseCountInput.toManualPurchaseCount();
+        ManualLottoNumbersInput manualLottoNumbersInput = inputView.inputManualLottoNumbers(manualPurchaseCount.fetchPurchaseCount());
+        PurchaseCountCalculator purchaseCountCalculator = new PurchaseCountCalculator(purchaseAmount);
+        PurchaseCount autoPurchaseCount = purchaseCountCalculator.calculateAutoPurchaseCount(manualPurchaseCount);
+        outputView.viewPurchaseCount(PurchaseCountOutput.of(manualPurchaseCount, autoPurchaseCount));


이 메서드는 너무 길고 가독성이 낮아 무엇을 하는지 파악하기가 매우 어렵습니다.🤯 (버그를 유발하기 쉬운구조입니다.) 적절한 단위로 묶어서 분리하고, 이름을 붙여서 의도를 드러내보면 좋을 것 같아요!

이 부분 분리해봐야겠어요

haero77 · 2024-05-20T13:19:36Z

src/main/java/controller/LottoSimulator.java

+public class LottoSimulator {
+
+    public void run() {
+        InputView inputView = new ConsoleInputView();
+        OutputView outputView = new ConsoleOutputView();


이 클래스에서 inputView, outputView를 필드로 관리하지 않은 이유가 궁금해요!
외부에서 의존성을 주입받지 않고 메서드 내부에서 인스턴스를 생성할 때 어떤 문제가 있을지 같이 고민해보시죠~!

외부에서 주입받는게 더 좋을 것 같아요!

haero77 · 2024-05-20T13:22:43Z

src/main/java/domain/AutoPurchaseCount.java

+    public static AutoPurchaseCount create(int purchaseCount) {
+        return new AutoPurchaseCount(purchaseCount);
+    }


이 팩토리 메서드는 생성자를 대신 호출하는 것 외에 별다른 로직이 없네요.
생성자 대신 팩토리 메서드를 쓴 이유가 궁금합니다. (특별히 팩토리 메서드를 사용한 의도를 알기 어렵습니다!)

도메인객체를 단순히 생성(create)한다는 가독성을 위해 그렇게 했는데 단순 생성자를 사용하는 것으로도 고려해보겠습니다.

haero77 · 2024-05-20T13:28:45Z

src/main/java/domain/BonusNumber.java

+    private BonusNumber(int bonusNumber) {
+        this.bonusNumber = bonusNumber;
+    }
+
+    public static BonusNumber create(int bonusNumber) {
+        return new BonusNumber(bonusNumber);
+    }


어떤 정수도 범위에 상관없이 보너스 번호가 될 수 있는 구조입니다!
(이런 클래스를 미성숙한 클래스라고 하는데, 같이 공부해보시죠~!)

validation하는 걸 놓친 부분이네요!
아니면 LottoNumber 클래스를 만들고 거기서 validation을 한 후에 위 클래스에서 상속받아야 겠어요

haero77 · 2024-05-20T13:35:38Z

src/main/java/domain/LottoWinning.java

+    public static LottoWinning findLottoWinning(long matchWinningNumberCount, long matchBonusNumberCount) {
+        return Arrays.stream(LottoWinning.values())
+                .filter(lottoWinning -> lottoWinning.isMatchLottoWinning(matchWinningNumberCount, matchBonusNumberCount))
+                .findAny()
+                .orElseGet(() -> ELSE_PLACE);
+    }


Optional의 orElseGet과 orElse의 차이를 학습해보시면 좋을 것 같아요!
(orElseGet의 구현이 어떻게 되어있는지 직접 보시면 더 좋습니다!)

haero77 · 2024-05-20T13:37:23Z

src/main/java/domain/LottoWinning.java

+    public static LottoWinning findLottoWinning(long matchWinningNumberCount, long matchBonusNumberCount) {
+        return Arrays.stream(LottoWinning.values())
+                .filter(lottoWinning -> lottoWinning.isMatchLottoWinning(matchWinningNumberCount, matchBonusNumberCount))


findLottoWinning을 호출할 때, 실수로 순서를 바꿔서 넣을 가능성이 있는 구조입니다!
위 같은 실수를 방지하기 위해서는 어떻게 설계해야 좋을지 같이 공부해보시죠!

저 두 값을 위해 class를 생성해야 되나 생각이 들기도 하네요

실수하기 쉬운 구조에서, 실수하기 매우 어려운 구조로 변경해나가는 것의 비용이라고 치면 가성비 괜찮다고 생각해요~! 🌝

객체지향 생활체조 원칙에도 나와있는 내용인데, 같이 공부해보시죠~!

이유와 솔루션으로 정리하는 객체지향 생활체조 원칙

haero77 · 2024-05-20T13:38:42Z

src/main/java/domain/LottoWinning.java

+    public long fetchWinningAmount() {
+        return this.winningAmount;
+    }
+
+    public long fetchMatchWinningNumberCount() {
+        return this.matchWinningNumberCount;
+    }


단순 getter 인데 fetch라는 prefix를 쓰신 이유가 궁금합니다 :)

음.. 이 부분은 어떤 로직이 있다면 fetch를 쓰고 단순 데이터 조회면 getter를 쓰는 방향을 수정해봐야겠네요

haero77 · 2024-05-20T13:44:20Z

src/main/java/domain/Lottos.java

+    public List<Lotto> toList() {
+        return List.copyOf(this.lottos);
+    }


toList() 네이밍을 보고 다소 혼란스러웠습니다.

Lottos를 list화 한다는데, 실제로는 List<Lottos>가 아니라 List<Lotto>를 리턴하고 있어요.

toXXX 네이밍을 보아 뭔가 변환 작업을 이루어질 것으로 기대했는데 실제로는 getter입니다.

제삼자가 봤을 때 많이 혼란스러운 네이밍은 변경해봐도 좋을 것 같습니다 :D

(지난 번에도 비슷한 리뷰를 남긴 것으로 기억합니다!)

Lottos는 Lotto의 일급 컬렉션으로 사용했던거라 일급 컬랙션을 List로 변환한다 라는 느낌에서 썼던건데
혼란이 있다면 네이밍을 고민해봐야겠네요

haero77 · 2024-05-20T13:46:03Z

src/main/java/view/output/dto/LottosOutput.java

+public class LottosOutput {
+
+    private final List<LottoOutput> lottoOutputs;


LottoOutput의 리스트를 포함하는 값 객체이므로 클래스명도 LottoOutputs가 적합해보입니다!

haero77 · 2024-05-20T13:53:46Z

src/main/java/domain/LottosWinningCalculator.java

+    public LottosWinningCalculator(Lottos lottos, WinningNumbers winningNumbers, BonusNumber bonusNumber) {
+        this.validateBonusNumberNotContainWinningNumber(winningNumbers, bonusNumber);
+        this.lottos = lottos;
+        this.winningNumbers = winningNumbers;
+        this.bonusNumber = bonusNumber;
+    }
+
+    private void validateBonusNumberNotContainWinningNumber(WinningNumbers winningNumbers, BonusNumber bonusNumber) {
+        if (winningNumbers.isContainNumber(bonusNumber.fetchBonusNumber())) {
+            throw new DomainValidationException(WINNING_NUMBERS_CONTAIN_BONUS_NUMBER, "보너스 번호가 당첨 번호에 포함되어있으면 안됩니다.");
+        }
+    }


보너스 번호가 당첨 번호에 포함되면 안 된다는 validation이 LottosWinningCalculator 객체를 만드는 시점에서 이루어지도록 하신 이유가 궁금합니다! (요 validation이 LottosWinningCalculator에 있어야 할까요?)

사실 이 validation의 위치가 애매했습니다.
BonusNumber에 있어야 할 validation이지만 그럼 BonusNumber를 생성할때 WinningNumbers를 넘겨줘야 하는 부분이 맘에 안들었어서요.
사용하지 않는데 validation만을 위해 넘겨줘야해서요. 그냥 BonusNumber를 생성할때 넘겨줘도 괜찮았나 생각이드네요

사용하지 않는데 validation만을 위해 넘겨줘야해서요.

성숙한 인스턴스를 만들기 위해 validation을 위한 파라미터라면 '사용'의 범주로 봐도 괜찮을 것 같아요 :)
혹은 당첨 번호와 보너스 번호를 모두 입력받았을 때 그 즉시 validation 하는 것도 한 가지 방법이겠구요.

kilian-develop added 26 commits May 15, 2024 19:35

feat : LottoApplication 생성

af9f84e

feat : 요구 사항 README.md 생성

7cba8fc

feat : InputView 생성

e87a84d

- 구입 금액 입력 - 당첨 번호 입력 - 보너스볼 입력

feat : PurchaseInput.java 생성

1ce5340

feat : Console 유틸 객체 생성

a8479d1

feat : 사용자 구입 금액 입력

22da92c

feat : 로또 시뮬레이터 생성

7f2caba

feat : 구입 금액 도메인 생성

8fdc6c4

refactor : 패키지 이동

5b6eb35

feat : 구입 금액 Output객체 생성

d1e16c6

feat : 구입 개수를 출력한다.

491070d

feat : 로또 객체 생성

15168f2

feat : 로또 생성 구현

f95bdd6

feat : 구입 금액, 구입 갯수 분리

7e4cefa

feat : 로또 목록 출력

15ef237

feat : 도메인 객체 >> dto 변환 수정

681ac2a

feat : 지난 주 당첨 번호 입력받기

0a5e3ed

feat : 도메인 객체 validation 수정

572309c

feat : input validation 수정

781d2f1

feat : Constant 객체 생성

ea9d762

feat : 당첨 결과 출력

22a5d55

feat : 당첨 결과 출력

3f6be4f

feat : 수동 구매 구현

d3ecc04

test : 구매 갯수 계산기 테스트

3c51b34

test : 로또 테스트

9bc70a9

test : 공백 제거

7b5b04a

kilian-develop requested review from haero77 and sc0116 May 19, 2024 10:16

kilian-develop self-assigned this May 19, 2024

haero77 reviewed May 20, 2024

View reviewed changes

		public class LottosOutput {

		private final List<LottoOutput> lottoOutputs;

Conversation

kilian-develop commented May 19, 2024

java-lotto

비즈니스 요구 사항

구현 기능 목록

Uh oh!

haero77 left a comment • 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!

haero77 May 20, 2024 • 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!

haero77 May 20, 2024 • 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!

haero77 May 20, 2024 • 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!

kilian-develop May 21, 2024 • 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!

haero77 May 20, 2024 • 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!

haero77 May 20, 2024 • 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!

haero77 May 21, 2024 • 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!

haero77 May 20, 2024 • 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!

haero77 May 21, 2024 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

haero77 left a comment •

edited

Loading

haero77 May 20, 2024 •

edited

Loading

haero77 May 20, 2024 •

edited

Loading

haero77 May 20, 2024 •

edited

Loading

kilian-develop May 21, 2024 •

edited

Loading

haero77 May 20, 2024 •

edited

Loading

haero77 May 20, 2024 •

edited

Loading

haero77 May 21, 2024 •

edited

Loading

haero77 May 20, 2024 •

edited

Loading

haero77 May 21, 2024 •

edited

Loading