[Step03] 로또(2등) PR by pbg0205 · Pull Request #2307 · next-step/java-lotto

pbg0205 · 2022-04-21T14:21:10Z

안녕하세요! TDD 14기 박병기 입니다.
로또 2등 추가 미션 기능 완료하여 리뷰 요청 드립니다!
이번 미션에서 고민했던 부분은 BonusNumber 객체의 선언할지에 관한 고민이 있었습니다
WinningTicket과 같이 Wrapper class로 선언하면 객체의 행위를 조금 더 명확히 할 수 있지만
이후 미션에 적용해보면서 코드의 차이를 �확인해보고 싶어 LottoNumber에 보너스 번호 확인 로직만 추가한 상태입니다 🤔

�미션을 진행하면서 적당한 책임을 나누는 기준을 갖는 것이 �익숙치 않네요. 이전에 말씀해주셨던 Wallet을 선언하는 것이 저도 적합해보이지만 오버 엔지니어링에 관한 피드백을 듣고 프로젝트 규모의 적당한 책임을 부여하는 것이 아직까지는 �어려운 것 같습니다ㅎㅎ

혹시나 부족하거나 개선히야할 부분있다면 아낌없는 피드백 부탁드립니다!!
감사합니다. 😄

- InvalidMoneyUnitException 생성

- Money: getValue() - lottoTicketList -> lottoTickets

- Rank 규칙 변경에 따른 테스트 데이터 변경 - Rank enum : 2, 3등 구분 규칙 추가 - 보너스 번호 추가 로직 구현

aiden-sim

앞에 미션에서 부터 일급컬렉션을 잘 사용해 주셨고, 역할에 대한 고민도 많이 하신 상태라서
크게 리뷰할 부분은 없습니다. 👍
설계에는 정답이 없다고 생각합니다. 지금 처럼 고민 하시면서 객체에 대한 역할을 찾아가는 모습이 보기 좋습니다.

그래도 너무 빠르게 넘어가면 심심하실까봐
앞에서 놓친 부분 몇가지를 같이 언급합니다.

LottoTicketGenerator 의 생성자에서 구체적인 전략인 RandomGenerationStrategy 를 구현하고 있는데 앞단에서 주입받아 사용하면 좀 더 유연한 구조가 되지 않을까 생각됩니다.
예를들어 RandomGenerationStrategy 대신 다른 전략이 사용된다면 LottoTicketGenerator 의 코드는 수정이 발생되고, 이는 런타임에 전략을 교체할 수 있는 장점이 없어 집니다.
현재 RandomNumberUtil 은 RandomGenerationStrategy 에서만 사용되기 때문에 (전략의 일부분) 따로 유틸로 분리 안해도 되지 않을까 생각됩니다.
오히려 호출 단계가 많아지는 느낌인데 요 부분은 리뷰이님 의견도 궁금합니다 😄

generateLottoTickets -> generateLottoNumbers -> generateNumbers -> RandomNumberUtil.generateNumbers

그 외에 몇가지 리뷰 드렸으니 다음 미션 진행하시면서 같이 고민해 주시기 바랍니다.
벌써 로또 미션이 끝나가고 있네요.
다음 미션도 파이팅 하시기 바랍니다. ❗❗❗

aiden-sim · 2022-04-21T17:10:05Z

            Money money = inputView.readMoney();
            LottoGame lottoGame = generateLottoGame(money);
-            RankResults rankResults = lottoGame.start();
+            RankResults rankResults = lottoGame.start(inputView.readBonusNumber());


당첨 번호와 보너스 번호는 어떻게 보면 사용 되는 시점이 동일하다 봅니다.
따로 분리하지 말고 WinningTicket에서 같이 가지고 있으면 어떨까요 ?! 🤔

그렇게 되면 validateBonusNumber에 대한 유효성 체크 부분을 외부에 노출시키지 않고
WinningTicket 내부에서 처리할 수 있어서
좀 더 응집도 있는 클래스가 되지 않을까 생각되네요.

아 말씀 해주신대로 WinningTicket 내부 필드로 선언하는 것이 적합 하겠네요 😲
다음 미션에 WinningTicket 으로 변경 해보겠습니다!

aiden-sim · 2022-04-21T17:14:39Z

-    public InvalidLottoNumberException() {
-        super(INVALID_LOTTO_NUMBER_MESSAGE);
+    public InvalidLottoNumberException(int number) {
+        super(String.format(INVALID_LOTTO_NUMBER_MESSAGE_FORMAT, number));


에러 메시지만 봐도 어떤 값 때문에 에러가 났는지 알 수 있겠네요 👍

이전에 피드백 주셨던 내용을 적용해보니 원인을 명확하게 확인할 수 있어 좋은거 같습니다!
감사합니다 👍 👍

aiden-sim · 2022-04-21T17:16:23Z

+        winningTicket.validateBonusNumber(bonusNumber);
+        Ranks ranks = new Ranks(lottoTickets.getRanks(winningTicket, bonusNumber));
        List<RankResult> rankResults = ranks.groupBy();
        return new RankResults(rankResults);


Suggested change

return new RankResults(rankResults);

return new RankResults(ranks.groupBy());

현재 임시변수의 값을 사용하는 곳이 없어서 바로 넘겨도 될것 같습니다. 😄

aiden-sim · 2022-04-21T17:21:26Z

    }

-    public List<Rank> getRanks(WinningTicket winningTicket) {
+    public List<Rank> getRanks(WinningTicket winningTicket, LottoNumber bonusNumber) {


public Map<Rank, Integer> getRanks(WinningTicket winningTicket, LottoNumber bonusNumber) { return lottoTickets.stream() .map(lottoTicket -> winningTicket.getRank(lottoTicket, bonusNumber)) .collect(Collectors.toMap(Function.identity(), value -> 1, Integer::sum)); }

stream을 잘 이용하면 코드를 좀 더 간결하게 표현할 수 있습니다.

이런식으로 사용하면 따로 Ranks에서 초기화를 위한 initRankGroupingMap 작업과
groupBy 를 없앨 수 있을것 같네요.

피드백 감사합니다!! 저도 이 부분에 대해 고민이 되더라구요...
제가 했던 고민은 2등 0개, 3등 0개 경우를 어느 객체에 로직을 관리할지에 관한 고민이었습니다.
이 역할을 Ranks에서 처리할지, RankResult에서 처리를 책임 관점에서 고민해보았을 때, Ranks에서 결과 집계를 반환한다는 책임이 생각해 Ranks에서 처리하는 방법이 더욱 적합해 보였습니다 😃

@pbg0205 말씀하신 책임 괜찮네요!
설계는 트레이드오프의 연속이라 생각합니다.

제가 드린 의견은 역할에 대해 조금 모호할 수 있지만 좀 더 간결하게 로직을 작성할 수 있고
리뷰이님이 현재 작성하신 코드는 역할은 좀 더 명확하지만 값을 가지고와서 처리하다 보니 조금 추가적인 로직이 필요한 것 같네요.

장단이 있으니 원하시는 방향으로 작업하시면 될것 같습니다 😄

aiden-sim · 2022-04-21T17:23:33Z

+    }
+
+    public void validateBonusNumber(LottoNumber bonusNumber) {
+        if(!lottoTicket.includeBonusNumber(bonusNumber)) {


이 로직은 보너스볼이 당첨번호에 포함되지 않을 경우 예외가 발생하는 것인가요 ?! 🤔

구현했을 때는 보너스 번호가 당첨번호에 포함되어 있다고 생각했는데 다시 생각해보니까 아니더라구요 😅
당첨번호를 포함하지 않는 경우에 대해 예외를 반환하도록 로직 변경하겠습니다 ㅎㅎ

aiden-sim · 2022-04-21T17:25:45Z

    }

-    public Rank getRank(LottoTicket lottoTicket) {
+    public Rank getRank(LottoTicket lottoTicket, LottoNumber bonusNumber) {


현재 당첨번호와 구입한 로또번호의 변수명이 같다 보니 햇갈리는것 같습니다.
명확하게 구분해 보는것은 어떨까요 ?! 😄

ex) purchaseLottoTicket ?

네이밍이 조금 애매하긴 하네요;
로또를 추첨하다는 의미로 drawLotto는 어떠세요??

@pbg0205 제가 의견 드린 구매로또번호 보다
의견 주신 추첨로또번호가 더 명확한것 같네요. 👍

aiden-sim · 2022-04-21T17:27:30Z

    }

-    public void printLottoTickets(List<LottoTicket> lottoTicketList) {
+    public void printLottoTickets(List<LottoTicket> lottoTickets) {


개인적으로 lottoTickets을 보여줄때 숫자 정렬이 되어 있으면 좀 더 보기 편할것 같습니다.

aiden-sim · 2022-04-21T17:30:44Z

-    public RankResults start() {
-        Ranks ranks = new Ranks(lottoTickets.getRanks(winningTicket));
+    public RankResults start(LottoNumber bonusNumber) {
+        winningTicket.validateBonusNumber(bonusNumber);


LottoGame 에서 validateBonusNumber 체크를 하는게 약간 어색하다고 느껴졌는데
위에서 피드백 드린대로 WinningTicket 내에서 보너스 번호를 가지고 있다면
해당 로직을 제거할 수 있겠네요.

aiden-sim · 2022-04-21T17:36:01Z

-        return new Money(readNumber());
+        long value = readNumber();
+        validateMoneyUnit(value);
+        return new Money(value);


참고만 하세요~ 객체를 생성할때 정적 팩터리 메서드 방식으로 사용할 수 있습니다.
장점과 단점이 있으니 한번 읽어 보시면 좋을것 같습니다 😄

참고) https://honbabzone.com/java/effective-java-static-factory-method/

pbg0205 added 8 commits April 21, 2022 21:26

refacor: InvalidLottoNumberException 예외 원인 번호 반환

0f6d331

test: LottoNumber 동등성 비교 테스트 변경 (값 비교 X)

5065480

feat: 천원 단위 검증 예외 생성

89ac96d

- InvalidMoneyUnitException 생성

refactor: Rank 동일성 비교 변경

e8213e8

refactor: Money 미사용 메서드 제거 및 변수명 변경

7e742c4

- Money: getValue() - lottoTicketList -> lottoTickets

refactor: 수익/손해 반환 메세지 반영

5290bb2

feat: 보너스 번호 로직 추가

1e53e34

- Rank 규칙 변경에 따른 테스트 데이터 변경 - Rank enum : 2, 3등 구분 규칙 추가 - 보너스 번호 추가 로직 구현

feat: 보너스번호 검증 로직 추가

8a47be6

pbg0205 changed the base branch from master to pbg0205 April 21, 2022 14:21

aiden-sim approved these changes Apr 21, 2022

View reviewed changes

aiden-sim merged commit 9a48340 into next-step:pbg0205 Apr 21, 2022

	return new RankResults(rankResults);
	return new RankResults(ranks.groupBy());

Uh oh!

Conversation

pbg0205 commented Apr 21, 2022

Uh oh!

aiden-sim 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!

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!

pbg0205 Apr 22, 2022 • 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!

aiden-sim Apr 22, 2022 • 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!

aiden-sim Apr 21, 2022 • 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!

aiden-sim Apr 21, 2022 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aiden-sim left a comment •

edited

Loading

pbg0205 Apr 22, 2022 •

edited

Loading

aiden-sim Apr 22, 2022 •

edited

Loading

aiden-sim Apr 21, 2022 •

edited

Loading

aiden-sim Apr 21, 2022 •

edited

Loading