[2단계 - 로또(자동)] 리뷰 요청 드립니다! by woo-yu · Pull Request #1312 · next-step/java-lotto

woo-yu · 2021-03-24T07:45:17Z

안녕하세요 리뷰어님!

궁금한 점을 test/lotto/domain/AutoLottoStrategyTest 에 작성해보았는데,
리뷰어님꼐서 생각하시는 의견을 여쭐 수 있을까요?

이번 리뷰도 잘 부탁드리겠습니다. 감사합니다 :)

-Delimiter splitWithDelimiter 濡黝� �대��¦� �� -Operands int Arr -> List<Operand>瑜� �대�� 庖¦깆� 異�

-Lotto: 6媛黝� 濡蕤�踰��몃� 吏¢�� 대�� lotto(), equals(), hashcode()

-Lotto contains(), 踰��몃━�ㅽ�몄� �쇱���� 媛黝�� 諛�

-Lottos: n媛黝� 濡蕤� 由ъ�ㅽ�몃� 吏¢�� 濡蕤� 吏 �대��

-Winning: �뱀꺼踰��몄� �뱀꺼�� 吏¢�� 대��

-Winning matches(), �쇱���� � ��λ��¦ �대�� 쇱��� �\� 利��鬻\⑤��

-Lotto, Lottos toString()

-Winning getWinningNumbers(), �뱀꺼踰��몃� �鯛�린�¦�� unmodifiableList濡� 媛�몄¦� 諛��蕤�� -Lottos checkWinning(), �뱀꺼踰��몃� �鯛� �뱀꺼�� 媛黝��� matches瑜� �몄��蕤��

-Price: �뱀꺼湲��≪� 怨¦�고�� 퓩�� 대�� calculate() , �뱀꺼�� 諛髀� �뱀꺼 湲��≪� 珥炮�¦ 諛��蕤��. -Winning yield(), �뱀꺼湲��/援щℓ湲�� 笊瑜풰� 怨¦�고��.

-LottoFactory: 濡蕤� �庖¦깆� 愿¢�퓩�� ⑺�퐤━ �대�� lotto(), lottos(), winning() -LottoStrategy, AutoLottoStrategy: 濡蕤�� 蕤�ㅼ‾庖¦� �¦�듭� �‾¢�대�� 대�� makeLotto()

-InputView: �� UI瑜� 愿¢由ы�� inputBy(), inputWinning(), closeReader() -ResultView: 異蕤윶 UI瑜� 愿¢由ы�� printLottos()

-Price: change PRICE constant, method return type -Winning: change yield return type

-ResultView printResult(), printYield()

-Price: calcualte(), Winning: yield() fix refactor: constant �━

changgunyee

안녕하세요 HJ님 😄

본격적인 로또 미션의 시작이군요! 🔥
지난번 피드백 반영과 더불어 요구사항을 잘 반영해주신 것 같습니다.
특히 전략패턴을 이용한 테스트는 잘 수행해주신 것 같습니다.

그러나 몇 가지 아쉬운 점이 있네요. 😢

원시값에 대해서 Wrapping하여 클래스로 만들어보세요!(불변 객체로 구현이 가능하다면 시도해보셔도 좋습니다.)
enum을 사용해보세요

또한 요구사항에는 없지만 맥락상 당첨번호에 대한 제약 사항을 구현해주세요.(금액에 대해서도 해주시면 좋습니다.

당첨 번호는 6개입니다.
로또 숫자는 1~45까지입니다.

그 외에도 몇가지 코드상에서 피드백드렸습니다.
피드백 반영 후 다시 리뷰 신청 부탁드리겠습니다.
파이팅입니다~ ⚡ ⚡ ⚡

changgunyee · 2021-03-25T11:30:07Z

+    }
+
+    @Override
+    public String toString() {


저는 toString이 UI로직이라고 생각해 조금 불편하더라도 ResultView로 빠지는 것이 좋다고 생각합니다.
출력을 변경하고 싶을 때 Lotto.java파일을 건드려야하기 때문이지요. 😄

의견 주셔서 감사합니다!
getter로서 값의 접근보다는 toString()으로 반환형태를 바꾸는 방식을 생각했는데,
생각해보니 출력 형태 자체는 ResultView에 존재하는게 맞다는걸 깨달았습니다 의견 감사합니다 😊

그러나 toString()을 완전히 없애서 값을 getter로 얻어내어 출력하는 방식보다는
default 형태의 toString을 Overriding 하는 형태로 접근하여 ResultView에서 문자열을 파싱하는 형태로 로직을 구성해보았습니다.
혹시 해당 방식이 잘못되었다면 의견 부탁드리겠습니다. 감사합니다!

changgunyee · 2021-03-25T11:30:28Z

+import java.util.List;
+import java.util.Objects;
+
+public class Lottos {


일급 컬렉션 👍

changgunyee · 2021-03-25T11:34:40Z

+
+public class LottoFactory {
+
+    public static Lotto lotto(LottoStrategy lottoStrategy) {


LottoStrategy의 위치를 잘 생각해보시기 바랍니다.
LottoFactory는 로또를 생성해내는 곳이므로 그냥 LottoStrategy를 가지고 있어도 괜찮을 것 같군요 🙏

changgunyee · 2021-03-25T11:35:35Z

+
+public class Lotto {
+
+    private List<Integer> numbers;


로또의 숫자도 클래스로 만들어보면 어떨까요?
1~45까지의 숫자만 가능하다는 규칙들이 해당 클래스에 들어가게 될겁니다. 😄

changgunyee · 2021-03-25T11:36:34Z

+import java.util.ArrayList;
+import java.util.List;
+
+public class LottoFactory {


util성 클래스가 아니여보이는군요......

changgunyee · 2021-03-25T12:03:40Z

+    public static Winning winning(List<Integer> winningNumber) {
+        return new Winning(winningNumber);
+    }
+}


1 ~ 45는 다수 개의 로또를 사더라도 계속해서 사용하게 됩니다.
로또 게임 시작할 때 1~45의 숫자를 만들어두고 반복해서 사용해보는 것은 어떨까요?

changgunyee · 2021-03-25T12:05:08Z

+        List<Integer> numbers = lottoStrategy.makeLotto();
+        System.out.println(numbers);
+    }
+}


전략 패턴 👍

changgunyee · 2021-03-25T12:05:52Z

+
+    @BeforeEach
+    void setUp() { //given
+        numbers = new ArrayList<>();


Arrays.asList를 사용하시면 좀 더 편하실 것 같군요!

아 네네!
해당 로직에서는 ArrayList 선언 후 for loop를 통해 일정 수의 로또를 생성하는 방식으로 만들었는데요,
Arrays.asList에서도 이런 형태의 List Comprehension이 가능한가요?

Java에서도 가능한지 찾아보았으나 복잡한 방식으로 구성이 되어있어서 직관적으로 이해하긴 이쪽이 더 간편해보입니다 😅

changgunyee · 2021-03-25T12:07:21Z

+        testLottos.checkWinning(winning);
+
+        //then
+        assertThat(winning).isEqualTo(predictWinning);


객체간 비교 💯

changgunyee · 2021-03-25T12:08:02Z

+        //then
+        assertThat(winning.getWinningNumbers()).containsAll(winningNumber);
+    }
+}


충분한 단위테스트 💯

-LottoNumber: ��帷� �ъ媓 �대�� isLottoNumber()

-LottoFactory setLottoStrategy(), lotto�¢ lottos�¦蕤�� ¦�� 二쇱� X print UI logic 遺¦由� package 蹂¢寃�

-Price : enum class濡� 蹂¢寃�

Winning -> WinningLotto: name change -LottoFactory, LottoStrategy: �¦�� 댁� �� 濡蕤� 踰�� 庖¦깆� �⑺�퐤━濡� �대�

woo-yu · 2021-03-28T14:37:58Z

안녕하세요! 꼼꼼한 리뷰 남겨주셔서 감사합니다.

코멘트를 바탕으로 수정해보았는데요.
몇가지 반영하지 않은 부분이 있는데 제가 생각하는 의견을 해당 코멘트에 댓글 남겼습니다!
혹시 잘못된 부분이 있다면 말씀 부탁드리겠습니다.

감사합니다! :)

changgunyee

안녕하세요. HJ님 😄

피드백을 잘 반영해주셨어요.
고민을 한 흔적이 보이고 코드 퀄리티가 확실히 좋아진 것 같습니다.

UI와 도메인 분리
저는 개인적으로 욕심을 내서 꼭 분리를 했으면 좋겠습니다.
물론 toString을 두고도 UI로직이 도메인에 침범하지 않게 코딩을 할 수 있습니다.
그러나 toString이 있으면 UI로직이 도메인에 의도치않게 영향을 주는 경우가 많이 있습니다.(특히 다음 미션인 사다리 미션에서 말이죠....)
아예 toString을 없앰으로서 여지를 주지 않는게 좋다고 생각해요~

요구사항
당첨 번호는 중복될 수 없습니다.
확인 부탁드릴께요.

다음 단계로 바로 넘어가도 좋지만 코드나 구조를 잘 잡고가야 나머지 단계들도 편할 것이라고 생각합니다.
피드백 반영 후 다시 리뷰 신청 부탁드리겠습니다.
끝까지 파이팅입니다 🔥 🔥

changgunyee · 2021-03-29T11:10:27Z

+
+    private static LottoStrategy lottoStrategy;
+
+    public static List<LottoNumber> lottoNumbers = new ArrayList<>();


lottoNumbers보다는 재사용하는 기본 lottoNumbers라는 힌트를 위해 Default를 붙이는 등 구분을 해주면 가독성이 좋을 것 같군요 👋

changgunyee · 2021-03-29T11:20:17Z

+    public void checkWinning(WinningLotto winning) {
+        List<LottoNumber> winningNumbers = winning.getWinningNumbers();
+        lottoList.forEach(lotto -> {
+            winning.matches(lotto.contains(winningNumbers));


제 의미는 다음과 같았습니다.

winning.matches(lotto);

winning이 당첨 여부를 판단하는 것이지요. 😄
객체끼리 메세지 패싱 방식으로 구현해보세요~

changgunyee · 2021-03-29T11:23:57Z

+        IntStream.range(0, result.length)
+                .filter(i -> i >= MINIMUM_MATCH)
+                .forEach(i -> {
+                    sb.append(i + "개 일치 (" + Price.winningPrice(i) + "원) - " + result[i] + "개");


람다는 간단하게 유지하는 것이 좋습니다.
함수로 빼보는 것은 어떨까요?

changgunyee · 2021-03-29T11:25:46Z

+        });
+    }
+
+    public int getBuyNum() {


Lottos에서 buyNum을 가져오는 것은 이상하군요.
main함수를 보니 없앨 수 있어보여요! 👍

changgunyee · 2021-03-29T11:30:08Z

+    NO_MATCH(0L);
+
+    public static final int LOTTO_PRICE = 1000;
+    private static Price[] prices = {NO_MATCH, NO_MATCH, NO_MATCH, THREE_MATCH_PRICE, FOUR_MATCH_PRICE, FIVE_MATCH_PRICE, SIX_MATCH_PRICE};


맞은 숫자 개수를 멤버 변수로 가지고 있는게 어떨까요?

그러면 이 부분이 없어질 수 있어요. 🙏

changgunyee · 2021-03-29T11:31:03Z

+    * */
+    public double yield(int buyNum) {
+        Long winningMoney = Price.calculate(match);
+        Long buyMoney = buyNum * 1000L;


매직넘버 상수화 부탁드려요~

changgunyee · 2021-03-29T11:33:10Z

+import java.util.List;
+import java.util.Objects;
+
+public class WinningLotto {


당첨 로또 숫자 관리 + 로또들의 당첨 개수 관리

두 가지의 역할을 담당하고 있네요.
아직까지는 괜찮지만 추후에 로직이 늘어날 가능성이 보여요.
분리하면 좋을 것 같아요. 👍

changgunyee · 2021-03-29T11:34:15Z

+    public static final int MINIMUM_MATCH = 3;
+
+    private List<LottoNumber> winningNumbers;
+    private int[] match = new int[MAX_MATCH_NUM+1];  //당첨 개수를 확인하는 array, matchArr[2]은 2개 당첨개수를 뜻한다.


Map과 같은 자료구조를 써보는 것은 어떨까요? 더 코드가 깔끔해질 것 같네요. 😄

changgunyee · 2021-03-29T11:37:04Z

+    }
+
+    @Override
+    public String toString() {


저는 toString을 아예 안가지고 있는게 좋다고 생각합니다.
UI출력을 편하게 하기 위해 도메인 객체를 역으로 바꾸게 되는 경우가 있습니다.(자신도 모르게 말이죠....)
특히 사다리 미션에서 그런 경우가 많습니다.

ResultView에서의 작업이 많이 번거로울 수 있겠지만 아예 분리 부탁드릴께요 🙏

-LottoFactory: defaultLottoNumbers �대� 蹂¢寃� -InputView: number overlap 泥댄¬� 濡黝�

…cs, checkWinning logic -Price -> Rank: matchNo, winningMoney 蹂댁� -WinningLotto -> WinningNumbers(�뱀꺼 踰�� 蹂댁� 諛� �뱀꺼踰�� 留ㅼ�), WinningStatics(�뱀꺼 媛黝�� 蹂댁� 諛� �笊瑜� 怨¦��) -Lottos, WinningNumbers: checkWinning() method瑜� Message passing�쇰� 蹂¢寃�, getter �帷굅

-WinningStatistics: yield() 濡黝� 援ъ¦� -Rank: prices[] �帷굅

…濡黝� ��

woo-yu · 2021-03-29T15:52:43Z

리뷰 감사합니다!

당첨 로또의 개수, 당첨 로또의 번호 등등... 요구사항에 정확히 명시되어있지 않은 모든 input 값에대한 예외처리를 모두 고려해주라는 말은 없었던것 같은데 추가되는 부분이 많네요 😅 (로또 구매 금액도 예외처리 대상임)
그밖에도 리뷰를 남겨주시면서 점점 추가되는 부분이 늘어나서 단계를 넘어가기가 좀 버거운거 같습니다.
이후에 남겨주시는 리뷰는 이후 단계에서 진행하겠습니다.

changgunyee

안녕하세요. HJ님

피드백은 잘 반영해주신 것 같습니다. 💯
코드의 품질이 확연히 좋아졌군요 👍

로또 번호의 범위나 중복된 당첨 번호 처리는 예외처리를 생각하여 객체의 역할을 생각해보셨으면 했으면 했습니다. 😄
현재 InputView에서 inputWinning에서 이러한 예외들에 대해서 check를 하고 있습니다.
하지만 로또 숫자의 1~ 45는 LottoNumber객체의 생성자 쯤에서 하는 것이 맞습니다.(로또 숫자는 1~ 45이다.)
또한 중복된 당첨 로또는 Lotto객체의 생성자 쯤에서 하는 것이 맞습니다. (로또는 6개의 로또 숫자를 가져야한다)
에러 처리 방법으로는 Lotto객체에서 List로 LottoNumber를 갖는 것이 아닌 Set을 써도 되구요.

바로 정답을 드리기보다는 이러한 예외를 처리하면서 어느 객체에서 담당하면 좋을까를 고민해봤으면 하는 생각에서 화두를 던진 것 입니다.
조금 의도가 모호했다면 언급을 정확하게 하도록 노력하겠습니다.

또한 한번의 리뷰에서 많은 것을 고치려하지 마라라는 리뷰어 수칙이 있어 한번에 많은 피드백을 안드리려하다보니 리뷰를 할 수록 새로운 리뷰가 나왔던 것 같습니다.
이 또한 수정하도록 하겠습니다. 🙏

모든 피드백이 다 반영되어야 다음 단계로 넘어가는 것은 아닙니다.
중요한 것은 테스트 코드와 객체의 역할과 책임에 따라 객체지향적으로 코딩하는 부분이므로 이 부분만 잘 지켜지면 됩니다.
지금 구조가 유지보수하기에 유연하여 앞으로의 단계가 쉬울 것이라고 생각이 드는군요.
끝까지 파이팅입니다! 🔥

changgunyee · 2021-03-30T09:47:56Z

+    public static final int LOTTO_PRICE = 1000;
+    private static Rank[] prices = {NO_MATCH, NO_MATCH, NO_MATCH, THREE_MATCH_PRICE, FOUR_MATCH_PRICE, FIVE_MATCH_PRICE, SIX_MATCH_PRICE};
+
+    private final int matchNo;


피드백 반영!

changgunyee · 2021-03-30T09:48:18Z

+
+public class WinningStatistics {
+
+    private final Map<Rank, Integer> winningStatistics = new HashMap<>();


자료구조 사용 굿!

changgunyee · 2021-03-30T09:50:38Z

+import java.util.List;
+import java.util.Objects;
+
+public class WinningNumbers {


WinningNumbers만 따로 분리하셨군요

changgunyee · 2021-03-30T09:57:31Z

+
+import static lotto.domain.Rank.LOTTO_PRICE;
+
+public class WinningStatistics {


피드백 반영!

changgunyee · 2021-03-30T09:59:48Z

+    public static void printResult(WinningStatistics statistics) {
+        System.out.println(RESULT_MESSAGE);
+        System.out.println(RESULT_MESSAGE_LINE);
+        statistics.winningStatistics().entrySet().stream()


UI를 완전히 분리해주셨군요.

woo-yu added 17 commits March 23, 2021 13:45

refactor: step1 �� 遺¦由� �쇰�諛� ��

4c1813a

-Delimiter splitWithDelimiter 濡黝� �대��¦� �� -Operands int Arr -> List<Operand>瑜� �대�� 庖¦깆� 異�

docs README.md

aee73ae

feat

6665222

-Lotto: 6媛黝� 濡蕤�踰��몃� 吏¢�� 대�� lotto(), equals(), hashcode()

feat

565cf0e

-Lotto contains(), 踰��몃━�ㅽ�몄� �쇱���� 媛黝�� 諛�

feat

0026e2c

-Lottos: n媛黝� 濡蕤� 由ъ�ㅽ�몃� 吏¢�� 濡蕤� 吏 �대��

feat

faa60f0

-Winning: �뱀꺼踰��몄� �뱀꺼�� 吏¢�� 대��

feat

96ce368

-Winning matches(), �쇱���� � ��λ��¦ �대�� 쇱��� �\� 利��鬻\⑤��

feat

0449e44

-Lotto, Lottos toString()

feat

7aa6b8b

-Price: �뱀꺼湲��≪� 怨¦�고�� 퓩�� 대�� calculate() , �뱀꺼�� 諛髀� �뱀꺼 湲��≪� 珥炮�¦ 諛��蕤��. -Winning yield(), �뱀꺼湲��/援щℓ湲�� 笊瑜풰� 怨¦�고��.

feat

4fe8456

-LottoFactory: 濡蕤� �庖¦깆� 愿¢�퓩�� ⑺�퐤━ �대�� lotto(), lottos(), winning() -LottoStrategy, AutoLottoStrategy: 濡蕤�� 蕤�ㅼ‾庖¦� �¦�듭� �‾¢�대�� 대�� makeLotto()

feat: View

7bdd111

-InputView: �� UI瑜� 愿¢由ы�� inputBy(), inputWinning(), closeReader() -ResultView: 異蕤윶 UI瑜� 愿¢由ы�� printLottos()

fix: type misscasting (over int -> change Long)

2ee469e

-Price: change PRICE constant, method return type -Winning: change yield return type

feat: print logic

4f83bb5

-ResultView printResult(), printYield()

refactor package

d1639d8

fix: price calculate logic

896317a

-Price: calcualte(), Winning: yield() fix refactor: constant �━

refactor package

dad188a

changgunyee requested changes Mar 25, 2021

View reviewed changes

woo-yu added 6 commits March 26, 2021 00:09

feat: LottoNumber 異�

61baac7

-LottoNumber: ��帷� �ъ媓 �대�� isLottoNumber()

refactor: LottoFactory

4343e35

-LottoFactory setLottoStrategy(), lotto�¢ lottos�¦蕤�� ¦�� 二쇱� X print UI logic 遺¦由� package 蹂¢寃�

refactor: Price change to enum

612eef8

-Price : enum class濡� 蹂¢寃�

refactor Winning -> WinningLotto, lottoNumbers()

1caea76

Winning -> WinningLotto: name change -LottoFactory, LottoStrategy: �¦�� 댁� �� 濡蕤� 踰�� 庖¦깆� �⑺�퐤━濡� �대�

refactor toString() logic

ddac7ab

refactor check winning number size

b674f8e

changgunyee requested changes Mar 29, 2021

View reviewed changes

woo-yu added 4 commits March 29, 2021 21:21

refactor: method name, number overlap logic

b5dad22

-LottoFactory: defaultLottoNumbers �대� 蹂¢寃� -InputView: number overlap 泥댄¬� 濡黝�

refactor: yield 濡黝� 蹂¢寃�, prices[] �帷굅

4f46bee

-WinningStatistics: yield() 濡黝� 援ъ¦� -Rank: prices[] �帷굅

refactor: unmodifiedCollection�¦ return��� getter 異� 諛� ResultView …

d8414f7

…濡黝� ��

refactor: delete getBuyNum

a5266a2

woo-yu closed this Mar 29, 2021

woo-yu reopened this Mar 29, 2021

changgunyee approved these changes Mar 30, 2021

View reviewed changes

changgunyee merged commit 5f6480c into next-step:hj-woo Mar 30, 2021


		public class LottoFactory {

		public static Lotto lotto(LottoStrategy lottoStrategy) {


		private static LottoStrategy lottoStrategy;

		public static List<LottoNumber> lottoNumbers = new ArrayList<>();


		public class WinningStatistics {

		private final Map<Rank, Integer> winningStatistics = new HashMap<>();


		import static lotto.domain.Rank.LOTTO_PRICE;

		public class WinningStatistics {

Conversation

woo-yu commented Mar 24, 2021

Uh oh!

changgunyee 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!

Choose a reason for hiding this comment

Uh oh!

woo-yu Mar 28, 2021 • 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!

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!

woo-yu commented Mar 28, 2021

Uh oh!

changgunyee left a comment

Choose a reason for hiding this comment

Uh oh!

changgunyee Mar 29, 2021 • 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!

changgunyee Mar 29, 2021 • 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!

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!

woo-yu commented Mar 29, 2021

Uh oh!

changgunyee 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!

Reviewers

Assignees

Labels

Projects

Milestone

changgunyee left a comment •

edited

Loading

woo-yu Mar 28, 2021 •

edited

Loading

changgunyee Mar 29, 2021 •

edited

Loading

changgunyee Mar 29, 2021 •

edited

Loading