[로또 main2] 박찬호 제출합니다. #6

great-park · 2023-04-04T11:42:37Z

안녕하세요!
컨트롤러에서 게임의 진행 상황을 코드로써 표현하기 위해서 노력했습니다. 그리고 최대한 역할을 분리시켰는데, 아직은 미숙한 것 같아요. 많은 피드백 부탁 드립니다!

본래의 설계하기를, Input에서 형식적인 검증을 수행하고, 각 객체에서 의미적인 검증을 수행하려고 했습니다. 현재 코드에서는 검증의 책임이 체계없이 파편화 되어 있어서 오히려 안 좋은 설계가 된 것 같네요. 차라리 모든 검증을 각 객체에게 맡겼으면 더 좋았을 것 같습니다. validation은 누구에게 맡겨야 할 지 각자 의견 달아주시면 감사하겠습니다!

ray-yhc

컨트롤러, 뷰, 서비스로직으로 나누어 구현하신 것이 좋은 구조인 것 같네요! 참고해갑니다!
application에서 LottoMachine과 LottoFactory를 주입하셨네요! 의존성 주입의 장점을 살려서 LottoMachine과 LottoFactory를 인터페이스로 구현하면 더 좋은 구조가 될 것 같아요!
만약 로또 상금 규칙이 변경된다면, Ranking과 Output을 모두 변경해야 해서 번거로울 것 같아요!

수고하셨습니다~~

ray-yhc · 2023-04-06T03:43:38Z

src/main/java/lotto/domain/LottoMachine.java

+        printInsertManualLottoNumbersRequest();
+        for (int i = 0; i < manualLottoCount; i++) {
+            List<Integer> numbers = insertLottoNumbers();
+            Lotto manualLotto = lottoFactory.createManualLotto(numbers);


view 메소드들이 서비스로직 내에 있는 것이 좋지 않은 것 같아요! 컨트롤러에서 로또를 입력하고 리스트를 전달받는 게 어떨까요?

다시 보니 컨트롤러에서 입력받고 생성은 서비스로직에서 해주면 더 좋을 것 같네요 감사합니다!

ray-yhc · 2023-04-06T04:03:18Z

src/main/java/lotto/domain/LottoMachine.java

다른 객체들의 역할들을 LottoMachine이 대신하고 있는 것 같아요

Lotto 생성은 Lotto 혹은 Lottos가,
WinningNumbers 생성은 WinningNumber가
결과 계산은 LottoResult가 직접 하도록 하면 어떨까요?

현실 속의 객체와 소프트웨어 객체 사이의 가장 큰 차이점은 무엇일까? 그것은 현실 속에서는 수동적인 존재가 소프트웨어 객체로 구현될 때는 능동적으로 변한다는 것이다.
...
스스로 판매 금액을 계산해서 종이에 기입하는 현실 속의 상품을 상상해 보라. 여러분이 객체지향 세계를 구축할 때 현실에서 가져온 객체들은 현실 속에서는 할 수 없는 어떤 일이라도 할 수 있는 전지전능한 존재가 된다.
<객체지향의 사실과 오해 p.55>

아 그렇네요
객체가 능동적으로 자신과 관련된 동작을 책임지도록 하는게 더 낫겠네요 감사합니다!

ray-yhc · 2023-04-06T04:13:27Z

src/main/java/lotto/domain/LottoMachine.java

+        totalWinningNumbers.addAll(normalWinningNumbers);
+        totalWinningNumbers.add(bonusWinningNumber);


bonusWinningNumber도 totalWinningNumbers에 포함되어 다음과 같이 에러가 나네요!

NumberType을 만들어서 WinningNumber 필드로 넣긴 했는데, 활용을 잘 못했네요
수정해보겠습니다!

ray-yhc · 2023-04-06T04:14:10Z

src/main/java/lotto/Application.java

+        LottoFactory lottoFactory = new LottoFactory();
+        LottoMachine lottoMachine = new LottoMachine(lottoFactory);
+        LottoController lottoController = new LottoController(lottoMachine);


오 의존성 주입을 구현하신 건가요??!?

ray-yhc · 2023-04-06T04:21:40Z

src/main/java/lotto/domain/enumeration/Ranking.java

+    private static Ranking getSecondOrThird(boolean isBonusContain) {
+        if (isBonusContain){
+            return Ranking.SECOND;
+        }
+        return Ranking.THIRD;
+    }


만약 2,3등의 수상 조건이 변경된다면??

ray-yhc · 2023-04-06T04:22:39Z

src/main/java/lotto/domain/LottoMachine.java

+    }
+
+    public LottoResult computeLottoResult(Lottos lottos, List<WinningNumber> winningNumbers) {
+        Map<Ranking, Integer> winningInfo = new HashMap<>();


key 가 Enum이라면 EnumMap도 추천합니다!

seong-wooo

테스트하기 좋은 구조로 변경하려면 어떻게 해야할까요?
일단 이 부분만 중점으로 해서 리뷰 남기겠습니다!

seong-wooo · 2023-04-06T10:01:33Z

src/main/java/lotto/domain/Lotto.java

+        this.numbers = numbers.stream()
+                .sorted()
+                .collect(Collectors.toList());
+    }


numbers 내부에 존재하는 값들이 모두 1 ~ 45라는 룰을 만족하나요?

seong-wooo · 2023-04-06T10:19:02Z

src/main/java/lotto/view/Input.java

+    private static int validateNumber(String input) {
+        return checkRange(checkNumber(input));
+    }
+
+    private static int checkNumber(String input) {
+        if (!input.chars().allMatch(Character::isDigit)) {
+            throw new IllegalArgumentException(INPUT_IS_NOT_NUMBER);
+        }
+
+        return parseInt(input);
+    }
+
+    private static int checkRange(int input) {
+        if (input < LOTTO_MIN_NUMBER || input > LOTTO_MAX_NUMBER) {
+            throw new IllegalArgumentException(LOTTO_NUMBER_OUT_OF_RANGE);
+        }
+        return input;
+    }


상당히 복잡한데요?

checkNumber나 checkRange는 검증의 역할만 하는 것이 좋을 것 같아요.

seong-wooo · 2023-04-06T10:24:13Z

src/main/java/lotto/domain/Lotto.java

+    public Lotto() {
+        this.numbers = Randoms.pickUniqueNumbersInRange(MINIMUM_LOTTO_NUMBER, MAXIMUM_LOTTO_NUMBER, LOTTO_TOTAL_COUNT)
+                .stream()
+                .sorted()
+                .collect(Collectors.toList());
+    }


Lotto lotto = new Lotto()를 실행했다고 생각해볼게요.
lotto 내부에 어떤 값이 들어있는지 테스트할 수 있나요?
Random으로 값을 생성하는 로직을 직접적으로 의존하고 있어서 테스트할 수 없습니다.

며칠 전에 이동욱 개발자님이 하신 말씀이 생각나네요.
제어할 수 없는 영역에는 의존하지말라!

jeongyuneo · 2023-04-11T06:13:09Z

src/main/java/lotto/domain/Lottos.java

+        List<Lotto> lottos = new ArrayList<>();
+        lottos.addAll(manualLottos);
+        lottos.addAll(autoLottos);


List<Lotto> lottos = Stream.concat(manualLottos.stream(), autoLottos.stream()) .collect(Collectors.toList());

이런 방식으로도 두 개의 리스트를 합칠 수 있습니다!

jeongyuneo · 2023-04-11T06:15:02Z

src/main/java/lotto/domain/Lottos.java

+    private Integer manualLottoCount;
+    private Integer autoLottoCount;


자료형으로 int가 아닌 Integer를 사용하신 이유가 있으신가요?

jeongyuneo · 2023-04-11T06:28:35Z

src/main/java/lotto/view/Input.java

+    private static final int LOTTO_MIN_NUMBER = 1;
+    private static final int LOTTO_MAX_NUMBER = 45;


야구 게임 미션 당시 성우님이 달아주셨던 코멘트인데, 찬호님 코드에도 해당되는 내용 같아서 첨부해드립니다.
로또 숫자의 최솟값과 최댓값에 대한 캡슐화가 필요해보입니다!

jeongyuneo · 2023-04-11T06:52:20Z

src/main/java/lotto/view/Input.java

+    private static int checkNumber(String input) {
+        if (!input.chars().allMatch(Character::isDigit)) {
+            throw new IllegalArgumentException(INPUT_IS_NOT_NUMBER);
+        }
+
+        return parseInt(input);
+    }


검증 로직에서 검증 외에 파싱까지 수행하는게 어색한 것 같습니다!

jeongyuneo · 2023-04-11T06:55:54Z

src/main/java/lotto/controller/LottoController.java

+        try {
+            LottoMoney money = new LottoMoney(insertLottoMoney());
+            Lottos lottos = lottoMachine.createLottos(money, insertManualLottoCount());
+            printLottos(lottos);
+            List<WinningNumber> winningNumbers = lottoMachine.createWinningNumbers(insertNormalWinningNumbers(), insertBonusWinningNumber());
+            LottoResult lottoResult = lottoMachine.computeLottoResult(lottos, winningNumbers);
+            printLottoResult(lottoResult, money);
+        } catch (IllegalArgumentException error) {
+            printErrorMessage(error.getMessage());
+        }


사용자가 잘못된 값을 입력할 경우 IllegalArgumentException를 발생시키고, "[ERROR]"로 시작하는 에러 메시지를 출력 후 종료한다.

try-catch 때문에 해당 요구사항을 만족하지 못하는 것 같아요! try-catch는 삭제해도 될 것 같습니다!

jeongyuneo · 2023-04-11T07:38:17Z

src/main/java/lotto/domain/LottoMachine.java

+    private List<Lotto> createManualLottos(int manualLottoCount) {
+        List<Lotto> manualLottos = new ArrayList<>();
+        printInsertManualLottoNumbersRequest();
+        for (int i = 0; i < manualLottoCount; i++) {
+            List<Integer> numbers = insertLottoNumbers();
+            Lotto manualLotto = lottoFactory.createManualLotto(numbers);
+            manualLottos.add(manualLotto);
+        }
+
+        return manualLottos;
+    }


private List<Lotto> createManualLottos(int manualLottoCount) { printInsertManualLottoNumbersRequest(); return Stream.generate(lottoFactory::createAutoLotto) .limit(manualLottoCount) .collect(Collectors.toList()) }

이렇게 Stream을 사용하면 가독성이 훨씬 좋아질 것 같아요!

jeongyuneo · 2023-04-11T08:07:37Z

src/main/java/lotto/domain/enumeration/Ranking.java

+
+    private final Long prizeMoney;
+    private final int equalCount;
+    private final boolean containBonus;


해당 변수는 선언 후 사용되고 있지 않네요!

jeongyuneo · 2023-04-11T09:13:03Z

src/main/java/lotto/domain/LottoMoney.java

+    private final String MONEY_MUST_OVER_1000 = "구매 금액은 1000원 이상을 입력해주세요.";
+    private final String MONEY_UNIT_IS_1000 = "구매 금액은 1000원 단위로 입력해주세요.";
+    private final int LOTTO_PRICE_UNIT = 1000;


Suggested change

private final String MONEY_MUST_OVER_1000 = "구매 금액은 1000원 이상을 입력해주세요.";

private final String MONEY_UNIT_IS_1000 = "구매 금액은 1000원 단위로 입력해주세요.";

private final int LOTTO_PRICE_UNIT = 1000;

private static final String MONEY_MUST_OVER_1000 = "구매 금액은 1000원 이상을 입력해주세요.";

private static final String MONEY_UNIT_IS_1000 = "구매 금액은 1000원 단위로 입력해주세요.";

private static final int LOTTO_PRICE_UNIT = 1000;

상수 선언은 보통 static final을 함께 사용합니다!

jeongyuneo · 2023-04-13T14:04:57Z

src/main/java/lotto/view/Output.java

+        String FIFTH_STATS = String.format("3개 일치 (%s원) - %s개", numberFormat.format(FIFTH.getPrizeMoney()), lottoResult.getWinningInfo().getOrDefault(FIFTH, 0));
+        String FORTH_STATS = String.format("4개 일치 (%s원) - %s개", numberFormat.format(FORTH.getPrizeMoney()), lottoResult.getWinningInfo().getOrDefault(FORTH, 0));
+        String THIRD_STATS = String.format("5개 일치 (%s원) - %s개", numberFormat.format(THIRD.getPrizeMoney()), lottoResult.getWinningInfo().getOrDefault(THIRD, 0));
+        String SECOND_STATS = String.format("5개 일치, 보너스 볼 일치 (%s원) - %s개", numberFormat.format(SECOND.getPrizeMoney()), lottoResult.getWinningInfo().getOrDefault(SECOND, 0));
+        String FIRST_STATS = String.format("6개 일치 (%s원) - %s개", numberFormat.format(FIRST.getPrizeMoney()), lottoResult.getWinningInfo().getOrDefault(FIRST, 0));


로또 결과를 출력 양식으로 반환하는 로직은 LottoResult 안에 있으면 더 좋을 것 같아요!

great-park added 15 commits April 3, 2023 21:00

branch test

1446b8f

feat: 로또 구입 금액과 수동 로또 구입 개수를 입력 받는다.

47f699e

feat: 수동 로또 번호를 입력받는다.

e4a20f7

feat: 수동 로또와 자동 로또를 생성한다.

323ba51

feat: 최종적으로 구매한 로또의 결과를 생성한다.

e5d31f4

feat: 오류 메시지를 출력한다.

f956564

feat: 로또 수량과 각 로또의 숫자를 출력한다.

6f9a540

fix: 수동 로또 번호 담기

a021a53

fix: 총 로또 구입 개수 수정

c25f06e

refactor: 확장성을 위해 네이밍 변경

e64dfd8

feat: 당첨 번호와 보너스 번호를 생성한다.

68faf17

style: enumeration 패키지

f5f4066

feat: 로또 당첨 결과를 계산하고 이를 관리하는 객체를 생성한다.

d35e916

feat: 로또의 결과를 출력한다.

189e45b

fix: 소숫점 연장

f7403aa

great-park requested review from ray-yhc, jeongyuneo, minnseong and seong-wooo April 4, 2023 11:42

great-park added 3 commits April 4, 2023 20:44

refactor: lottoMachine 주입

942c5f2

refactor: lottoFactory 주입

48f5790

style: line 수정

8d0b162

ray-yhc reviewed Apr 6, 2023

View reviewed changes

seong-wooo reviewed Apr 6, 2023

View reviewed changes

jeongyuneo reviewed Apr 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[로또 main2] 박찬호 제출합니다. #6

[로또 main2] 박찬호 제출합니다. #6

great-park commented Apr 4, 2023

ray-yhc left a comment

ray-yhc Apr 6, 2023

great-park Apr 6, 2023

ray-yhc Apr 6, 2023

great-park Apr 6, 2023

ray-yhc Apr 6, 2023

great-park Apr 6, 2023

ray-yhc Apr 6, 2023

ray-yhc Apr 6, 2023

ray-yhc Apr 6, 2023

seong-wooo left a comment

seong-wooo Apr 6, 2023

seong-wooo Apr 6, 2023

seong-wooo Apr 6, 2023

jeongyuneo Apr 11, 2023

jeongyuneo Apr 11, 2023

jeongyuneo Apr 11, 2023

jeongyuneo Apr 11, 2023

jeongyuneo Apr 11, 2023

jeongyuneo Apr 11, 2023

jeongyuneo Apr 11, 2023

jeongyuneo Apr 11, 2023

jeongyuneo Apr 13, 2023

		totalWinningNumbers.addAll(normalWinningNumbers);
		totalWinningNumbers.add(bonusWinningNumber);

		private Integer manualLottoCount;
		private Integer autoLottoCount;

		private static final int LOTTO_MIN_NUMBER = 1;
		private static final int LOTTO_MAX_NUMBER = 45;

[로또 main2] 박찬호 제출합니다. #6

Are you sure you want to change the base?

[로또 main2] 박찬호 제출합니다. #6

Conversation

great-park commented Apr 4, 2023

ray-yhc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seong-wooo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment