[Tany / Ader] 로또 3단계 - 수동구매 기능 추가 #54

ak2j38 · 2022-02-25T09:41:52Z

3단계 PR입니다.

안녕하세요 리뷰어님! tany와 ader입니다.
항상 정성스러운 코드리뷰 감사드립니다.

구현사항

Lotto 클래스를 추상클래스로 변경해 AutoLotto, ManualLotto, WinningLotto가 각각 상속받도록 구성했습니다.
AutoLotto, ManualLotto, WinningLotto는 각각 추상메소드인 createLotto를 자신의 로직대로 구현합니다.
위를 통해 AutoLotto, ManualLotto, WinningLotto를 각각 생성할 수 있었고 하나의 LottoPaper안에 List<Lotto>로 관리할 수 있었습니다.
모든 예외처리를 구현하지는 못했지만 기본적인 예외처리와 유효성검사를 구현했습니다.
테스트 코드를 시간상 추가하지 못한점이 아쉽습니다.

고민사항

현재 InputView와 OutputView는 많은 static으로 관리되고 있는데 리뷰어님께서는 static대신 InputView의 객체를 만들어서 사용하는 것과 static으로 사용하는 것 중 어느것을 추천하시는 지 궁금합니다!
현재 Lotto클래스를 추상클래스로 만들어 본인을 생성하는 메소드만 추상메소드로 지정해 각각 하위클래스에서 자신의 생성방식을 구현하는 것은 미션에서 의도한 방향일까요?
하나의 메소드 내에 여러개의 지역변수가 있는 경우도 있는데 이 경우 가독성을 위해 지역변수 사용을 의도했는데 지양해야하는 방법일까요?
전체적인 코드에서 매개변수로 컬렉션(리스트)이 파라미터로 전달되는 경우가 많은데 이때 너무 자주 등장하는 매개변수(컬렉션)은 클래스의 멤버변수로 선언해두고 사용해도 괜찮은 방법인가요...?(제 생각은 클래스 어디서든 접근할 수 있게 되어 매개변수로만 사용했습니다.)

감사합니다!! 😊

1. 수동으로 몇 장 구매할지 입력 받는 기능 1. 수동 번호 입력 받는 기능 end

1. Lotto클래스에서 발생한 충돌 해결 end

1. 열거형 클래스가 자신의 형태를 반환하는 메소드를 가짐으로서 책임 명확히 수정 end

1. 입력받는 메소드에서 예외처리 할 수 있도록 수정 2. 출력형식 수정 end

1. 입력받는 메소드에서 예외처리 할 수 있도록 유효성검사 메소드 추가 2. 숫자입력인지 유효성검사 3. 최소 1000원 이상 인지 유효성검사 4. 보너스번호와 당첨번호가 중복되는지 유효성검사 end

1. 수동로또에서 무의미한 상수 제거 2. 로또를 상속한 당첨로또를 추가 end

1. 불필요 메소드 제거 2. 당첨판단 메소드 추가 end

1. 당첨판단 테스트 삭제(시간이 부족해 새 테스트는 추가하지 못했습니다) end

1. 매칭된 숫자와 보너스보유 여부를 가진 클래스 추가 end

1. 수동로또의 모든 갯수를 전달받기 위해 map으로 변경 end

1. 당첨판단의 책임을 LottoPaper로 위임 2. 불필요한 지역변수 정리 end

1. 불필요 메소드 제거 2. 매칭판단 메소드 추가 end

1. 미션3 내용 추가 end

wheejuni

수고 많으셨습니다. 💯
추상 클래스를 적절하게 활용한 부분은 정말 인상적입니다. 잘 하셨어요.
질문하신 것에 대한 답변은 별도 코멘트로 드릴게요~

wheejuni · 2022-02-26T09:25:29Z

src/main/java/lotto/controller/LottoGame.java

-        LottoStore lottoStore = new LottoStore();
-        LottoPaper lottoPaper = lottoStore.purchase(purchaseAmount);
+    private LottoPaper purchase(int purchaseAmount, int manualLottoCount, Map<Integer, List<Integer>> numbers) {
+        LottoPaper lottoPaper = new LottoStore().purchase(purchaseAmount, manualLottoCount, numbers);


LottoStore 는 구매요청이 들어올 때마다 새로 만들어져야만 하는 객체인지 궁금합니다.
도메인적인 요구 사항만 봐선 그렇지 않은 것 같은데요. LottoGame 이 인스턴스 변수로 관리해도 충분하다고 보여지는데 어떻게 생각하시는지요?

맞습니다. 프로젝트 구현을 다시 되돌아보니 LottoStore는 하나만 존재하고 여러개의 LottoPaper를 발행할 수 있도록 구현했어야했습니다!
바로 수정하겠습니다 😄

wheejuni · 2022-02-26T09:26:17Z

src/main/java/lotto/controller/LottoGame.java

-    private int getBonusNumber(String inputBonusNumber) {
-        return Integer.parseInt(inputBonusNumber);
-    }
+    private Map<Integer, List<Integer>> getManualNumbers(Map<Integer, String> inputNumbers) {


List 자체가 순차성이 보장되는 컬렉션인데, 굳이 Map 의 키로 순서를 따로 보관할 이유가 있었는지 궁금해요.

Map 자료구조를 사용한 이유는 다음과 같습니다!

여러 개의 수동로또 번호를 입력받은 후(문자열 형태)

LottoGame에서 문자열 리스트(Map의 value) 형태를 정수형 리스트(Map의 value) 형태로 변환하게 됩니다(LottoGame::getManualNumbers).

이 과정에서 각 수동 로또의 번호들은 따로 관리되어야 하는데 하나의 리스트로 합쳐지는 현상을 방지하기 위해 Map을 사용하였습니다.

<추가설명>
2의 과정에서 모든 수동로또들을 <순서, 수동로또번호> 형태로 반환해서 그렇습니다! Map을 사용하지 않는다면 하나의 List에 모든 수동로또번호가 계속 add가 되어 사용하는 쪽에서 6개마다 분리해주는 것보다 Map이 더 알맞다고 판단했습니다 😅

wheejuni · 2022-02-26T09:29:39Z

src/main/java/lotto/domain/AutoLotto.java

+    @Override
+    public List<Integer> createLotto(List<Integer> numbers) {
+        return pickSixNumbers(numbers);
+    }


지금 구현을 보면... 파라메터로는 항상 1부터 45까지의 숫자가 들어오는 건가요?

자동로또 생성 시에는 항상 1부터 45의 숫자가 들어오게 됩니다. 😃

wheejuni · 2022-02-26T09:30:15Z

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


-public class Lotto {
+public abstract class Lotto {


아까 AutoLotto 의 구현을 보며 들었던 의문인데 파라메터로 항상 1부터 45까지의 숫자를 받을거라면...
추상 클래스의 이점을 살려서 여기서 1부터 45까지의 숫자가 초기화된 리스트를 관리하는 건 어떨까 의견 드려 봅니다.

1부터 45의 숫자가 AutoLotto에서만 사용되고 있는데 AutoLotto가 아닌 Lotto에서 관리해아하는 이유가 혹시 따로 있을까요....??
제 생각은 Lotto를 상속받는 다른 클래스인 ManualLotto나 WinningLotto에서는 1~45가 사용되지 않을 것 같아 의견을 여쭙니다!

wheejuni · 2022-02-26T09:31:58Z

src/main/java/lotto/domain/MatchLottoResult.java

@@ -0,0 +1,19 @@
+package lotto.domain;
+
+public class MatchLottoResult {


👍 👍 👍

wheejuni · 2022-02-26T09:32:28Z

src/main/java/lotto/domain/WinningLotto.java

+import java.util.List;
+import java.util.stream.Collectors;
+
+public class WinningLotto extends Lotto {


번호 일치 카운트를 알려주는 메소드도 있으면 좋겠네요!

수정 시 고려하겠습니다 😄

wheejuni · 2022-02-26T09:32:54Z

src/main/java/lotto/domain/WinningStrategy.java

@@ -26,4 +26,29 @@ public int getWinningPrice() {
    public int getCorrectNumber() {
        return this.correctNumber;
    }
+
+    public static WinningStrategy convertMatchNumberToWinningStrategy(int matchNumber, boolean hasBonusNumber) {


헉 어떻게 좀 개선할 수 있는 방법이 없을까요? 전략 패턴을 사용하기에 좋은 케이스도 아닌듯 보입니다...

아 심지어 전략 패턴도 아니군요. 낚이기 좋은 enum 이름입니다...

그룹리뷰때도 한 분이 네이밍만 보고 전략패턴으로 오해하신 부분입니다 ㅠㅠㅠ
가이드 주신대로 네이밍도 변경해보고 로직도 개선해보겠습니다 😅

wheejuni · 2022-02-26T09:33:24Z

src/main/java/lotto/domain/WinningStrategy.java

+        if (matchNumber == WinningStrategy.FIVE_MATCHES.getCorrectNumber()) {
+            return WinningStrategy.FIVE_MATCHES;
+        }
+        return WinningStrategy.SIX_MATCHES;


만약에 matchNumber 로 8, 9, 10 이런 숫자들이 넘어온다면... 그럴 가능성이 아예 없으려나요?
공개된 API라서 그럴 수 있을 것 같다는 생각은 듭니다.

의도한 숫자들 이외의 값이 파라미터로 들어온다면 예외처리를 하는 로직을 추가해야겠군요!
메소드 자체를 공개된 API라고 생각해본 적이 없어서 예외처리가 미숙했던 것 같습니다. 감사합니다 👍

wheejuni · 2022-02-26T09:34:03Z

src/main/java/lotto/domain/WinningStrategy.java

+        if (matchNumber == WinningStrategy.ZERO_MATCHES.getCorrectNumber()) {
+            return WinningStrategy.ZERO_MATCHES;
+        }
+        if (matchNumber == WinningStrategy.ONE_MATCHES.getCorrectNumber()) {
+            return WinningStrategy.ONE_MATCHES;
+        }
+        if (matchNumber == WinningStrategy.TWO_MATCHES.getCorrectNumber()) {
+            return WinningStrategy.TWO_MATCHES;
+        }
+        if (matchNumber == WinningStrategy.THREE_MATCHES.getCorrectNumber()) {
+            return WinningStrategy.THREE_MATCHES;
+        }
+        if (matchNumber == WinningStrategy.FOUR_MATCHES.getCorrectNumber()) {
+            return WinningStrategy.FOUR_MATCHES;
+        }
+        if (matchNumber == WinningStrategy.FIVE_MATCHES.getCorrectNumber() && hasBonusNumber) {
+            return WinningStrategy.FIVE_WITH_BONUS_MATCHES;
+        }
+        if (matchNumber == WinningStrategy.FIVE_MATCHES.getCorrectNumber()) {
+            return WinningStrategy.FIVE_MATCHES;


스트림 API로 깔끔하게 개선할 수 있습니다. 시도해보시죠!

스트림으로 한번 개선해보겠습니다! 키워드 감사합니다 👍

wheejuni · 2022-02-26T14:09:17Z

질문에 답변 드립니다.

InputView, OutputView 는 콘솔 환경에서 어쩔 수 없이 사용하는 측면이 있습니다. 지금 구현 단계에서 너무 그 둘의 상세한 구현 혹은 그곳에서의 클린 코드에 신경쓰지 않았으면 하는 마음이 있네요. (어차피 웹에선 view layer는 자바 프로젝트 바깥으로 분리됨)
의도한 것은 아닌데 참신했습니다. 굳
여러개의 지역변수 가 꼭 필요한지부터 다시 고민해야 합니다. 대체로 메서드 안에 지역변수가 너무 많으면 설계가 산으로 가고 있다는 신호입니다. 가독성을 위해 그렇게 했다는 말이 잘 이해되지 않습니다
파라메터를 멤버 변수로 선언한다는 말이 이해되지 않습니다.. 메서드의 설계는 기본적으로 멱등성 이라는 기본 원칙을 갖고 설계하는 것이 좋고요. 그런 의미에서 보면... 아 그런 의미에서 봐도 파라메터를 멤버 변수로 선언한다는 말씀이 잘 이해되지 않네요;

감사합니다.

ak2j38 · 2022-02-26T15:52:04Z

상세하고 정성스러운 리뷰 감사드립니다! 🙇🙇🙇
남겨주신 리뷰들을 보며 설계가 산으로 가지 않았는지 다시 한번 살펴봐야겠네요 ㅠㅠ
조언해주신 내용들을 바탕으로 수정햏 다시 커밋해보겠습니다.
감사합니다 👍👍👍

ak2j38 added 13 commits February 25, 2022 18:17

feature : 수동 구매를 위한 입력 기능 추가

7ac1b17

1. 수동으로 몇 장 구매할지 입력 받는 기능 1. 수동 번호 입력 받는 기능 end

feature : 충돌해결

e3260d5

1. Lotto클래스에서 발생한 충돌 해결 end

feature : 열거형 클래스에 Enum 변환 메소드 추가

d2c7205

1. 열거형 클래스가 자신의 형태를 반환하는 메소드를 가짐으로서 책임 명확히 수정 end

feature : 입출력 클래스 예외처리 및 메소드 추가

3eee1a0

1. 입력받는 메소드에서 예외처리 할 수 있도록 수정 2. 출력형식 수정 end

feature : 로또 클래스를 상속한 당첨번호로또 클래스추가 및 수동로또 수정

abd182d

1. 수동로또에서 무의미한 상수 제거 2. 로또를 상속한 당첨로또를 추가 end

feature : 로또종이 클래스에서 불필요 메소드 제거 및 당첨판단 메소드 추가

689f51f

1. 불필요 메소드 제거 2. 당첨판단 메소드 추가 end

feature : 메소드 변경으로 인해 테스트 삭제

ed14528

1. 당첨판단 테스트 삭제(시간이 부족해 새 테스트는 추가하지 못했습니다) end

feature : 각 로또의 당첨결과 클래스 추가

59c1681

1. 매칭된 숫자와 보너스보유 여부를 가진 클래스 추가 end

feature : 수동로또를 전달받기 위해 수정

344616b

1. 수동로또의 모든 갯수를 전달받기 위해 map으로 변경 end

feature : LottoGame의 책임을 줄이기 위해 수정

677819d

1. 당첨판단의 책임을 LottoPaper로 위임 2. 불필요한 지역변수 정리 end

feature : 불필요 메소드 제거 및 매칭판단 메소드 추가

b488b0a

1. 불필요 메소드 제거 2. 매칭판단 메소드 추가 end

docs : 미션3 내용 추가

d37b794

1. 미션3 내용 추가 end

ak2j38 added the review-BE Improvements or additions to documentation label Feb 25, 2022

wheejuni self-requested a review February 26, 2022 08:49

wheejuni self-assigned this Feb 26, 2022

wheejuni requested changes Feb 26, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tany / Ader] 로또 3단계 - 수동구매 기능 추가 #54

[Tany / Ader] 로또 3단계 - 수동구매 기능 추가 #54

ak2j38 commented Feb 25, 2022

wheejuni left a comment

wheejuni Feb 26, 2022

ak2j38 Feb 27, 2022 •

edited

Loading

wheejuni Feb 26, 2022

ak2j38 Feb 27, 2022

wheejuni Feb 26, 2022

ak2j38 Feb 27, 2022

wheejuni Feb 26, 2022

ak2j38 Feb 27, 2022 •

edited

Loading

wheejuni Feb 26, 2022

wheejuni Feb 26, 2022

ak2j38 Feb 27, 2022

wheejuni Feb 26, 2022

wheejuni Feb 26, 2022

ak2j38 Feb 27, 2022

wheejuni Feb 26, 2022

ak2j38 Feb 27, 2022

wheejuni Feb 26, 2022

ak2j38 Feb 27, 2022

wheejuni commented Feb 26, 2022

ak2j38 commented Feb 26, 2022

		@@ -0,0 +1,19 @@
		package lotto.domain;

		public class MatchLottoResult {

[Tany / Ader] 로또 3단계 - 수동구매 기능 추가 #54

Are you sure you want to change the base?

[Tany / Ader] 로또 3단계 - 수동구매 기능 추가 #54

Conversation

ak2j38 commented Feb 25, 2022

3단계 PR입니다.

구현사항

고민사항

wheejuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ak2j38 Feb 27, 2022 • edited Loading

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

ak2j38 Feb 27, 2022 • edited Loading

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

wheejuni commented Feb 26, 2022

ak2j38 commented Feb 26, 2022

ak2j38 Feb 27, 2022 •

edited

Loading

ak2j38 Feb 27, 2022 •

edited

Loading