[혁] 로또게임 #8

malibinYun · 2020-05-21T17:54:01Z

전체적으로 티켓 발급 / 비교 두 부분으로 기능을 나누어서 코드를 작성하였습니다.
ticket 패키지에는 로또 번호와 티켓 관련코드
compare 패키지에는 당첨번호와 티켓번호를 비교하는 것과 로또 결과관련 코드가 들어있습니다.

덕분에 메인코드가 좀 길어지는데,
이 두 기능으로 분리된 애들을 묶는 클래스를 하나 더 만든 뒤 메인에서 써먹는편이 나을까요?

그나저나.. 로또 번호를랜덤으로 만들어줘야해서 이부분을 인터페이스로 만든뒤, 하위로 RamdomLottoNumberGenerator를 만들었는데, 이 클래스를 테스트하지 않아 테스트 커버리지가 0이여서 통과를 못합니다. 이부분은 어떻게 해야할까요

Class	역할
LottoNumber	로또숫자 VO
LottoNumberConverter	List<Integer> -> List<LottoNumber>
ManualLottoNumbers	수동 로또번호인 List<Integer> 를 가지고있음. toLottoTicket()메소드 보유
LottoTicket	List<LottoNumber> 의 일급컬렉션 다른 티켓과 비교하는 책임 특정 넘버가 있는지 확인하는 일
LottoTickets	List<LottoTicket> 의 일급컬렉션 일급 컬렉션 끼리 합쳐서 새 일급컬렉션 만듬 로또 최종 결과를 만들어줌
RandomLottoTicketGenerator	랜덤 값을 가진 LottoTicket을 만들어줌
LottoTicketDispenser	자동/수동 티켓을 만들어줌 수동 티켓 발급시 돈이 모자란지 검증하는 로직 있음.
LottoLuckyNumbers	기본 당첨번호 6개짜리 티켓 하나, 보너스볼이 들어있는 객체. 로또 티켓 하나와 비교해서 해당 결과를 만듦
LottoResult	로또 상금의 Enum 카운트 수와 보너스 유무로 등수를 알려줌
LottoResults	List<LottoResult> 의 일급 컬렉션 총 상금, 특정 등수의 상금 등을 알려줌
Money	로또 금액 VO

jyami-kim · 2020-05-22T16:12:44Z

Random Test 관련 글
https://www.slipp.net/questions/186?fbclid=IwAR3-RwZPAfhky3h-Jle-1RMeEAoenGFQlsaFQzm6Wt7iRel0VnBYwIDk3x8

jyami-kim · 2020-05-22T16:43:37Z

src/main/java/com/javabom/lotto/LottoApplication.java

 public class LottoApplication {
    public static void main(String[] args) {
+        long inputMoney = InputView.getMoneyToBuyTicket();
+        int ticketQuantity = (int) inputMoney / 1000;


엇,, 뭔가 추상화 수준이 안맞는 듯 합니다.
위아래에서는 기능을 분리하여 클래스안에 있는 메서드를 호출해서 기능을 서술적으로 표현하는등 고수준의 추상화를 보여주었는데,
갑자기 추상화가 덜된(저수준) statement가 나와서 놀랐네요

기현님처럼 Money 클래스 만들어도 좋을듯 합니다.

jyami-kim · 2020-05-22T16:45:39Z

src/main/java/com/javabom/lotto/view/InputView.java

+    private static final Scanner scanner = new Scanner(System.in);
+
+    public static long getMoneyToBuyTicket() {
+        printLineOf(NOTICE_INPUT_MONEY);


혹시 안드로이드에서는 이렇게 쓰는게 관습인가요? 그냥 혹시나해서,,

코틀린에서 인라인 함수로
println(), print() 메소드가 있는데, 그걸 흉내내보았습니다 안드로이드도 로그찍는건 따로있어요!

jyami-kim · 2020-05-22T16:48:14Z

src/main/java/com/javabom/lotto/domain/ticket/LottoNumber.java

+
+import java.util.Objects;
+
+public class LottoNumber {


감싼거 아주 좋습니다b

jyami-kim · 2020-05-22T16:51:22Z

src/main/java/com/javabom/lotto/domain/compare/LottoLuckyNumbers.java

+    // LottoNumber 내부에 copy해주는 메소드를 추가하여 복사본을 반환하는게 더 나은가요?
+    public LottoNumber getBonusNumber() {
+        return bonusNumber;
+    }


지금도 괜찮아보입니다. LottoNumber 안에도 변경하는 메서드가 없기도하고 그내부 필드값도 final로 되어있어서 불변입니다.

jyami-kim · 2020-05-22T16:52:55Z

src/main/java/com/javabom/lotto/LottoApplication.java

+        LottoTickets lottoTickets = lottoTicketDispenser.getAutoTickets(ticketQuantity);
+        OutputView.printLottoTickets(lottoTickets);
+
+        LottoLuckyNumbers luckyNumbers = InputView.getLottoLuckyNumbers();


domain에 속하는 로직을 지닌 객체를 util성 성격을 띄고있는 inputView 객체의 메서드에서 반환하는건 조금 앞뒤가 안맞아보입니다.
InputView는 InputView의 역할을 하도록 수정하면 좋을 것 같습니다.

jyami-kim · 2020-05-22T17:02:43Z

src/main/java/com/javabom/lotto/domain/compare/LottoBasicLuckyNumbers.java

+import java.util.Collections;
+import java.util.List;
+
+public class LottoBasicLuckyNumbers {


이건 그냥 갑자기 혁님 코드 보고 떠오른 건데 한번 토론해보세요ㅋㅋ 이 방법은 어떤가 싶어서 코멘트 답니다.

지금 LottoTicketComparator를 보면 luckyNumbers와 LottoTicket모두 LottoNumber 리스트를 꺼내서 카운팅하고있는데,

만약 추상클래스로 LottoNumbers를 파고,
그아래 자식클래스로 이 LottoBasicLuckyNumbers / 그리고 사용자들의 티켓을 받는 자식클래스인 LottoUserNumbers
이렇게 분리하는건 어떤가 싶어요.
왜냐면 둘다 똑같은 List numbers 의 기능을 갖고있어서 겹치는 부분이 있다고 판단하기 때문입니다.
이때 둘이 다른 서브 클래스로 나누는 이유는 validation로직이 다르지 않나 싶어서 입니다..!!

그리고 나서 부모 클래스인 LottoNumbers에, compare method를 두어서 LottoNumbers 클래스끼리 비교하는 메서드를 만드는건 어떤가 싶어요.

그러면 LottoTicektComparator에서 List 레벨까지 꺼내지 않아도, 그저
luckyNumbers.compare(lottoTicket);
으로 끝낼 수 있지 않을까 싶어서 입니다.

같은 성질을 가진 티켓에 대한 비교를 LottoTicketcomparator가 책임을 갖고있는게 아니라,
LottoTicket 자체가 갖고있는거죠.
음 근데 이렇게 하면 혁님 코드에서는 comparator 패키지를 새로 땃으니 조금 이상해 보일수도 있겠네요ㅋㅋ

아 그러네요 둘다 똑같이 List 를 들고있다는점이 공통점이네요.

추상클래스 파서 서브클래스 둘 때 네이밍이 직관적이지 않을까 걱정이 되긴합니다. 근데 의견 동의합니다! 좋은 방법인거같아요

구현하다가 막힌점들이 있습니다.
기본 로또 숫자 6개를 비교하는건 가능하나, 나머지 보너스 숫자를 어떻게 비교해서 결과를 내줘야할지 모르겠습니다.
그리고 티켓이든 BasicLuckyNumbers든 한곳에서만 compare 메소드를 호출할 텐데, 다른 한곳에 열려있어도 괜찮은건지 모르겠습니다.
또, 여러 티켓들을 비교하려면 외부에서 compare를 여러번 호출해주어야하는데 이 때에 결국 다른 클래스가 일을 맡게되지 않나 라는 생각이 들었습니다.
역량이 부족해서 구현하기 어렵습니다 ㅠㅠ

pci2676

전체적으로 객체들의 책임이 분리가 되어있지 않은것 같아
get(), get()한 값들이 연산되고 있다면 책임을 생성해서 객체에게 위임해 보자

pci2676 · 2020-05-28T04:58:51Z

src/main/java/com/javabom/lotto/domain/ticket/Money.java

@@ -0,0 +1,14 @@
+package com.javabom.lotto.domain.ticket;
+
+public class Money {


이 클래스 하는 일이 게터밖에 없어서 의미가 없어보여 단순 dto야??

모든 원시값을 포장해야되서 돈을 원시값으로 막 왓다갓다 안하려구 감싼건데..
어떻게하면 더 의미를 줄수있을지 잘모르겠어

pci2676 · 2020-05-28T05:00:06Z

src/main/java/com/javabom/lotto/domain/ticket/LottoTicketDispenser.java

+
+    public LottoTickets getAutoTickets(Money inputMoney) {
+        int quantity = (int) inputMoney.get() / LOTTO_TICKET_PRICE;
+        ArrayList<LottoTicket> tickets = new ArrayList<>();


구현체에 의존하지 말고 인터페이스에 의존하자!

pci2676 · 2020-05-28T05:05:10Z

src/main/java/com/javabom/lotto/domain/ticket/LottoTicketDispenser.java

+    }
+
+    public LottoTickets getAutoTickets(Money inputMoney) {
+        int quantity = (int) inputMoney.get() / LOTTO_TICKET_PRICE;


Money가 해줄수 있는 일

뭔가 돈이 개수를 반환하는게 이상하다고 생각해서 여기다가 나누었었어

pci2676 · 2020-05-28T05:13:20Z

src/main/java/com/javabom/lotto/domain/compare/LottoBasicLuckyNumbers.java

+    // 로또 당첨번호가 로또 번호를 변환하는 코드를 가진다? 뭔가 이상해보입니다.
+    // 차라리 외부에서 LottoNumberConverter 라는 클래스를 만든뒤, 거기에서 List<LottoNumber>를 반환받는게


둘다 나쁘지 않아보임
만약 도메인 객체를 만드는 로직이 복잡해지면 후자의 방법이 좋을 것 같아보임

pci2676 · 2020-05-28T05:15:20Z

src/main/java/com/javabom/lotto/domain/ticket/LottoNumber.java

+
+    private void validateNumber(int number) {
+        if (number < MIN_LOTTO_NUM || number > MAX_LOTTO_NUM) {
+            throw new IllegalArgumentException("로또 번호는 1~45 입니다.");


아이템 75. 예외의 상세 메시지에 실패 관련 정보를 담으라 이펙티브 자바 참고바람

pci2676 · 2020-05-28T05:47:18Z

src/test/java/com/javabom/lotto/domain/compare/LottoLuckyNumbersTest.java

+    // LottoBasicLuckyNumbers 의 get()을 테스트하지 않았는데,
+    // 그 상위인 LottoLuckyNumbers에서 위 메소드를 부르니 테스트를 또 할 필요가 없다고 판단하였습니다.
+    // 오히려 반대로 LottoBasicLuckyNumbers에서 테스트하고, 상위인 LottoLuckyNumbers에서 테스트를 하지 말아야하나요?


나는 기본적으로 단위 테스트가 가장 우선시 되어야 한다고 생각해

pci2676 · 2020-05-28T05:47:56Z

src/test/java/com/javabom/lotto/domain/compare/LottoLuckyNumbersTest.java

+        List<LottoNumber> expectedLottoNumbers = stubLottoNumbers.stream()
+                .map(LottoNumber::new)
+                .collect(Collectors.toList());


테스트에서 이렇게 구체적인 구현로직이 들어가있는건 지양하는게 좋아

pci2676 · 2020-05-28T05:49:12Z

src/test/java/com/javabom/lotto/domain/compare/LottoResultsTest.java

+        List<LottoResult> expectedLottoResultList = lottoResultList.stream()
+                .filter(result -> result == FOURTH_PRIZE)
+                .collect(Collectors.toList());


구현로직 제거하고 다른 방식으로 테스트를 해보자

pci2676 · 2020-05-28T05:51:22Z

src/test/java/com/javabom/lotto/domain/compare/LottoTicketComparatorTest.java

+    // 여기서 각 등수별 테스트를 하려고 했는데 파라미터가 매우 많아지는 현상이 생깁니다.
+    // 이 클래스를 잘못짰기 때문에 테스트가 힘든구조가 되는걸까요?
+    // 여기서 테스트를 어떤식으로 해야할지 잘 모르겠습니다.


난 등수별 테스트 해야된다고 생각해
테스트가 어려운거는 그 구조가 테스트 하기 어려운 구조라고 생각하는게 맞는거같아
즉 구조를 수정해야하는가 의심해볼수 있는 포인트라고 생각해

생각해보니 등수별 테스트는 LottoResult EnumClass 에서 find 함수를 테스트하는걸로
(맞는 숫자 개수, 보너스 볼 별 테스트) 이미 테스트를 완료했어.
그러면 여기서는 굳이 등수별 테스트를 한번 더 할 필요는 없는거같네

pci2676 · 2020-05-28T05:54:31Z

src/test/java/com/javabom/lotto/domain/ticket/SortedLottoNumbersFactoryTest.java

+    // 얘는 어떻게 테스트를 하면 좋을지 몰라서 (for문을 쓰는 것은 지양하라 하여서)
+    // size 로 대체했으나, 정렬된것은 어떻게 테스트를 해야하는지 모르겠습니다 (for문없이)


assertThat(lottoNumbers).containsExactly(.........);
이렇게 하면 컬렉션에 들어간 객체들의 순서까지 정확히 판단할수있어

흠 나는 굳이 SortedLottoNumbersFactory 까지 생성해서 나눌 필요가 있나라는 생각이 들어
생성전략이 이미 인터페이스로 분리가 되있는 시점에서 충분히 분리가 되어있다고 생각을 했거든...🤔

Gobukgol · 2020-06-04T12:43:40Z

src/main/java/com/javabom/lotto/view/OutputView.java

+        return new StringBuilder()
+                .append("[")
+                .append(lottoNumbers)
+                .append("]")
+                .toString();


요 부분

String lottoNumbers = lottoTicket.getNumbers().stream() .map(String::valueOf) .collect("[",Collectors.joining(", "),"]");

저번에 얘기했던거!

Gobukgol · 2020-06-04T13:25:12Z

src/main/java/com/javabom/lotto/domain/ticket/LottoTicket.java

+    }
+
+    @Override
+    public boolean equals(Object o) {


이 LottoTicket의 equals는 어디에서 사용하는 거야?? 일단 실제 코드에는 사용하지않는 것 같고 사용한다면 테스트 코드에서 사용할 것 같은데 내 생각은 equals는 조심해서 사용해야 한다고 생각해.

LottoTicket이 같은 경우가 코드대로라면은 티켓이 들고있는 LottoNumber들이 같을 때 같다고 하게 되는데, 그렇게 되면 똑같은 번호로 산 로또 2개는 같은 Ticket 이라는 의미가 될것같아.

Set은 중복을 허용하지 않는데, Ticket을 Set에 넣는다고 가정했을때 같은 번호를 가지고있는 Ticket 2개를 넣게되면 Set에는 1개만 들어가게 될것같아.

내생각에는 같은 번호의 로또 티켓이라면 같은 티켓이 맞다고 봐.
Set은 형말대로 "중복"으로 같은 숫자들을 가진 티켓들을 제거 하려고 쓰일텐데, 그렇다면 오히려 Equals를 상속받아서 중복을 제거하는게 맞다고봐. 그러니 같은 숫자들을 가지고있는 티켓을 2개 넣는다면 1개가 들어가는게 맞는거지.
같은 번호들을 갖는 티켓을 서로 다르게 취급할거면 애초에 List로 들고있는게 맞다고봐.

Gobukgol · 2020-06-04T13:28:05Z

src/main/java/com/javabom/lotto/domain/results/LottoBasicLuckyNumbers.java

+import java.util.stream.Collectors;
+
+public class LottoBasicLuckyNumbers extends LottoNumbers {
+


당첨 번호 중복 검사도 필요할것같아!

맞어 이거 안햇엇어..

Gobukgol · 2020-06-04T13:28:59Z

src/main/java/com/javabom/lotto/domain/results/LottoLuckyNumbers.java

+    public LottoLuckyNumbers(LottoBasicLuckyNumbers basicLuckyNumbers, LottoNumber bonusNumber) {
+        this.basicLuckyNumbers = basicLuckyNumbers;
+        this.bonusNumber = bonusNumber;
+    }


당첨 번호와 보너스 번호가 중복되는지 유효성 검사 필요해보여!

또 BasicLuckNumbers나 LottoTicket 같이 LottoNumbers 추상클래스를 상속하는 객체들을 생성하는 새로운 객체를 하나 만들어서 거기에 validation 로직을 두는건 어떨까??? 지금 로또 번호 개수 상수가 여러군데에서 중복되서 쓰이고 있어서 이런식으로 하면 어느정도 해결할 수 있을것으로 보여!

밑에 말이 무슨말인지 잘 모르겠어.
일단은 이 상속관계에 대해서 좋지않다는 리뷰를 많이 들어서 상속을 걷어낼생각이구 BasicLuckyNumbers내부가 LottoTicket과 너무 유사해서 그냥 LottoTicket으로 통일해버릴 생각이야

Gobukgol · 2020-06-04T13:33:54Z

src/main/java/com/javabom/lotto/domain/LottoNumbers.java

+                .map(this.numbers::contains)
+                .filter(isContain -> isContain)


.filter(this.numbers::contains)

하나로 할 수 있어보여!

Gobukgol · 2020-06-04T13:42:51Z

src/main/java/com/javabom/lotto/domain/results/LottoResult.java

+        if (matchCount == SECOND_PRIZE.matchCount && isBonusMatched) {
+            return SECOND_PRIZE;
+        }
+        return valuesWithoutSecond().stream()


Second 빼고 List만들어주는 과정없이 해결할 수 있지않을까?

Gobukgol · 2020-06-04T13:45:22Z

src/main/java/com/javabom/lotto/domain/results/LottoResults.java

+
+    public Money getTotalPrizeMoney() {
+        long sum = lottoResults.stream()
+                .mapToLong(LottoResult::getPrice)


나도 실수했던건데 Prize랑 Price!

?? 나도 지금 실수하고있는거야??
저기에 잇는 price는 오타인거같네 첨부터 prize를 생각하고쓴거였는데

오타 나도 했었거든

Gobukgol · 2020-06-11T06:08:20Z

src/main/java/com/javabom/lotto/domain/ticket/RandomLottoTicketGenerator.java

+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+public class RandomLottoTicketGenerator implements LottoTicketGenerator {


음 RandomLottoTicketGenerator 테스트 같은 경우는 간단하게 실제로 6개를 반환하는지랑, 내부에 있는 LottoNumber들이 실제로 1 ~ 45 값사이인지 bound 테스트 할 수 있을것 같아

오 이건 처음 피드백에서 고쳤었음

Gobukgol · 2020-06-11T06:23:55Z

src/main/java/com/javabom/lotto/domain/ticket/LottoTicket.java

+    public int countMatchingNumbers(LottoTicket lottoTicket) {
+        int count = 0;
+        for (int i = 0; i < LOTTO_NUM_PICK_SIZE; i++) {
+            if (this.numbers.get(i).equals(lottoTicket.numbers.get(i))) count++;


인덴트 규칙! stream 을 써서 해결하는 방법은 없을까??

내머리로는 스트림으로 도저히 안돼던데... 대체 어케하는거야..?

Gobukgol · 2020-06-11T06:39:00Z

src/main/java/com/javabom/lotto/domain/ticket/LottoTicketDispenser.java

+    }
+    // 여기서 그냥 inputMoney.spend(totalPrice); 한줄만 때리려했는데, (spend 내부에 금액 검사 로직이 있어서)
+    // 다른사람이 볼 때 이게뭐야? 할거같고, inputMoney내부의 상태값이 바뀌는듯한 인상을 주어
+    // 이렇게 바꿨는데.. 어떻게 생각하시나요?


내 개인적인 생각인데 Money의

private void validateCanSpendMoney(Money money)

메소드를 public 으로 바꿔서 사용하는건 어떨까?? 검증로직이 곂치니까 public으로 열어두고 사용해도 괜찮을거 같아서

흠 이거 public으로 해도 괜찮은거야? 뭔가 validation 로직을 private으로밖에 안만들어봐서
이생각을 하긴햇는데 뭔가 석연치 않아서 private으로 놔둔거였거든...

malibinYun added 4 commits May 22, 2020 02:44

[add] 티켓 발급 부분 완료

c7c6eff

[add] 티켓 비교 부분 완료

f61cab6

[test] 티켓 쪽 테스트 코드 작성

339f14c

[test] 티켓 비교 부분 테스트 코드 작성

41e7fa3

jyami-kim reviewed May 22, 2020

View reviewed changes

malibinYun added 3 commits May 26, 2020 21:10

추상클래스 제외 피드백 반영

dd38b73

추상클래스 하다가 막힘..

6eee120

테스트코드 수정

bfb1a47

pci2676 requested changes May 28, 2020

View reviewed changes

피드백 반영 (LottoNumbers 추상클래스 리팩토링 적용)

40b2369

Gobukgol reviewed Jun 4, 2020

View reviewed changes

malibinYun added 3 commits June 5, 2020 03:30

1차 피드백 반영. 상속제거 및 BasicLuckyNumbers삭제

efe0fa1

2단계 미션 완료

97bf6f1

테스트 케이스 추가

1cd3d44

malibinYun force-pushed the yh branch from 037977f to 1cd3d44 Compare June 4, 2020 20:31

malibinYun added 2 commits June 10, 2020 21:52

피드백 반영 및 새 요구사항 반영

9fb8981

테스트케이스 추가

a8c5835

Gobukgol reviewed Jun 11, 2020

View reviewed changes

		@@ -0,0 +1,14 @@
		package com.javabom.lotto.domain.ticket;

		public class Money {

		// 로또 당첨번호가 로또 번호를 변환하는 코드를 가진다? 뭔가 이상해보입니다.
		// 차라리 외부에서 LottoNumberConverter 라는 클래스를 만든뒤, 거기에서 List<LottoNumber>를 반환받는게

		// 얘는 어떻게 테스트를 하면 좋을지 몰라서 (for문을 쓰는 것은 지양하라 하여서)
		// size 로 대체했으나, 정렬된것은 어떻게 테스트를 해야하는지 모르겠습니다 (for문없이)

		import java.util.stream.Collectors;

		public class LottoBasicLuckyNumbers extends LottoNumbers {

[혁] 로또게임 #8

Are you sure you want to change the base?

[혁] 로또게임 #8

Uh oh!

Conversation

malibinYun commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyami-kim commented May 22, 2020

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!

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

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!

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

malibinYun commented May 21, 2020 •

edited

Loading

malibinYun Jun 11, 2020 •

edited

Loading