로또(순서보장) by csbsjy · Pull Request #11 · Java-Bom/lotto

csbsjy · 2020-05-26T08:27:30Z

malibinYun · 2020-05-26T13:00:54Z

src/main/java/com/javabom/lotto/domain/result/BonusNumber.java

+    public BonusNumber(int number) {
+        if (isValidNumber(number)) {
+            throw new IllegalArgumentException("보너스번호는 1이상 45이하여야합니다");
+        }
+        this.number = number;
+    }
+
+    private boolean isValidNumber(int number) {
+        return number < NUMBER_BEGIN_BOUND || number > NUMBER_END_BOUND;
+    }


저는 개인적으로 if 문에다가 조건을 전부 쓰고,
if부터 throw까지 메소드로 빼는게 더 깔끔한거같아요!
조건이 읽히기 쉽다고 생각해서 괜찮다고 봅니다

가끔은 if문이 조금 더 판단하기 쉽다고 생각했는데 이 경우는 딱히 그래보이지 않네용 ㅎㅅㅎ 반영했습니다!

malibinYun · 2020-05-26T13:11:20Z

src/main/java/com/javabom/lotto/domain/result/LottoPrize.java

+        return new Money(totalMoney);
+    }
+
+    public static LottoPrize findByMathCount(int matchCount, boolean matchBonus) {


Match의 오타..인가요?
아니라면 의미가 궁금합니다!

오타입니다 !!

malibinYun · 2020-05-26T13:13:17Z

src/main/java/com/javabom/lotto/domain/result/LottoPrize.java

+        if (matchCount == THIRD.hitCount) {
+            return findByBonus(matchBonus);
+        }
+        return Arrays.stream(LottoPrize.values())
+                .filter(lottoPrize -> lottoPrize.hitCount == matchCount)
+                .findAny()
+                .orElse(MISS);
+    }
+
+    private static LottoPrize findByBonus(boolean matchBonus) {
+        if (matchBonus) {
+            return SECOND;
+        }
+        return THIRD;
+    }


아 요런방법이...
저는 2등이면 2등던져주고 2등 제외한 List를 새로 만들어서 했는데 이게더 좋아보이네요 ㅠㅠ

matchBonus 변수 자체가 보너스 번호 유무를 넘겨주는 건데, matchCount == THIRD.hitCount && matchBonus말고 한번 더 findByBonus로 빼신 이유가 있을까요??
개인적으로 메소드 없는게 더 파악이 잘되는 느낌이라서요!

malibinYun · 2020-05-26T13:13:53Z

src/main/java/com/javabom/lotto/domain/result/LottoPrize.java

+    public int getHitCount() {
+        return hitCount;
+    }
+}


이건 안쓰는메소드라 없애도 될거같아요

pci2676 · 2020-05-27T11:20:28Z

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

+        }
+    }
+
+    public LottoPrize calculateLottoPrize(WinningTicket winningTicket) {


양방향 의존성이 발생해 버렸지 뭐야..!

ㅋㅋㅋㅋㅋㅋㅋ 나 안그래도 이거 질문남겼어 ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 해결하겠읍니다 ,,!

pci2676 · 2020-05-27T11:22:25Z

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

+    private final Set<Integer> lottoNumbers;
+
+    public LottoTicket(Set<Integer> lottoNumbers) {
+        validate(lottoNumbers);
+        this.lottoNumbers = lottoNumbers;
+    }
+
+    private static boolean isValidNumber(Integer number) {
+        return number >= NUMBER_BEGIN_BOUND && number <= NUMBER_END_BOUND;
+    }


LottoTicket이 들고 있는 Integer에 대해 개별적으로 검증해야 할 로직이 있다는건
Integer가 하나의 객체로 분리할 수 있는 신호인거 같아
VO로 분리해보는거 어때?

pci2676 · 2020-05-27T11:23:50Z

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

+    public String toString() {
+        List<Integer> sortedLottoNumbers = new ArrayList<>(lottoNumbers);
+        Collections.sort(sortedLottoNumbers);
+        return String.valueOf(sortedLottoNumbers);


헉 나는 toString의 본래 용도대로 사용되는게 아닌거같아 보여

:( 마쟈요 동의합니다 ,,

pci2676 · 2020-05-27T11:31:11Z

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

+
+    public double getRateOfProfit(final Money spentMoney) {
+        Money totalProfit = LottoPrize.calculateTotalReword(lottoStatistics);
+        return (double) totalProfit.getValue() / spentMoney.getValue() * PERCENTAGE;


이 로직 Money 쪽에서 해줄수 있지않을까?

pci2676 · 2020-05-27T11:35:37Z

src/main/java/com/javabom/lotto/vo/Money.java

+    public Money(long value) {
+        this.value = value;
+    }


0원으로 게임 진행이 가능해 ㅎㅎ
vo 로 쓸수 있는 방어로직이 부족한거 같아

Money는 도메인에 종속된 vo가 아닌데 0원에 대한 방어로직을 Money에서 추가해줘야하나?.? 0원을 넣으면 하나도 못사고 끝나는걸로 괜찮지않을까 생각했어 !!

아하 그러면 마이너스 금액!!

사실 그것도생각해봤는데 -1000원도 뭔가 돈으로 칠라면 칠수있을거같아서 ,,

헉ㅋㅋ
근데 살수 없는 금액인데 당첨번호를 확인하러 가는거 이상하지 않아..??
나는 흐름이 끊겨야 된다고 생각했거든...

마쟈 구럼 차라리 LottoStore에 검증로직을 추가하는건 이상한가 ,,? 0원으로는 살수없습니다 !!

상태와 상태에 관련된 행동이 한곳에 있어야 응집도가 올라간다고 하잖아??
그렇게되면 응집도가 떨어지게되는거같아..!

흠 지금 코드에서 그렇게하면 while 돌릴때 문제가 발생할 수 있겠넹

pci2676 · 2020-05-27T11:38:48Z

src/test/java/com/javabom/lotto/domain/result/LottoPrizeTest.java

+    static Stream<Arguments> lottoPrizes() {
+        return Stream.of(
+                Arguments.of(3, false, LottoPrize.FIFTH),
+                Arguments.of(5, false, LottoPrize.THIRD),
+                Arguments.of(5, true, LottoPrize.SECOND),
+                Arguments.of(6, false, LottoPrize.FIRST),
+                Arguments.of(2, false, LottoPrize.MISS)
+        );
+    }
+
+    @DisplayName("번호일치 갯수에 따른 LottoPrize를 반환한다")
+    @MethodSource("lottoPrizes")
+    @ParameterizedTest
+    void findLottoPrize(int matchCount, boolean matchBonus, LottoPrize expectedLottoPrize) {
+        //when
+        LottoPrize byMathCount = LottoPrize.findByMathCount(matchCount, matchBonus);
+
+        //then
+        assertThat(byMathCount).isEqualTo(expectedLottoPrize);
+    }


Suggested change

static Stream<Arguments> lottoPrizes() {

return Stream.of(

Arguments.of(3, false, LottoPrize.FIFTH),

Arguments.of(5, false, LottoPrize.THIRD),

Arguments.of(5, true, LottoPrize.SECOND),

Arguments.of(6, false, LottoPrize.FIRST),

Arguments.of(2, false, LottoPrize.MISS)

);

}

@DisplayName("번호일치 갯수에 따른 LottoPrize를 반환한다")

@MethodSource("lottoPrizes")

@ParameterizedTest

void findLottoPrize(int matchCount, boolean matchBonus, LottoPrize expectedLottoPrize) {

//when

LottoPrize byMathCount = LottoPrize.findByMathCount(matchCount, matchBonus);

//then

assertThat(byMathCount).isEqualTo(expectedLottoPrize);

}

@DisplayName("번호일치 갯수에 따른 LottoPrize를 반환한다")

@ParameterizedTest

@CsvSource(value = {"3,false,FIFTH","5,false,THIRD","5,true,SECOND"})

void findLottoPrize(int matchCount, boolean matchBonus, LottoPrize expectedLottoPrize) {

//when

LottoPrize byMathCount = LottoPrize.findByMathCount(matchCount, matchBonus);

//then

assertThat(byMathCount).isEqualTo(expectedLottoPrize);

}

이넘 이렇게 넣을수 있엉

# Conflicts: # src/main/java/com/javabom/lotto/LottoApplication.java # src/main/java/com/javabom/lotto/view/InputView.java # src/main/java/com/javabom/lotto/view/OutputView.java # src/test/java/com/javabom/lotto/domain/LottoTicketTest.java # src/test/java/com/javabom/lotto/domain/LottoTicketsTest.java

Gobukgol · 2020-06-04T14:17:06Z

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

+        return Arrays.stream(winningNumbers.split(LOTTO_NUMBER_DELIMITER))
+                .map(winningNumber -> Integer.valueOf(winningNumber.trim()))
+                .map(LottoNumber::of)
+                .collect(Collectors.toSet());


이부분 ManualLottoNumbers 쪽에도 있는데 중복 제거하는 방법이 있지않을까??
당첨 번호랑 , 수동 로또도 LottoTicket으로 관리하니까 이 부분에서 중복 제거할 수 있는 방법이 있을거같아!

Gobukgol · 2020-06-04T14:18:38Z

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

+
+
+    public static void printLottoTicketNumbers(LottoTickets lottoTickets) {
+        System.out.println(lottoTickets.count() + "개를 구매했습니다.");


우리 요구사항은 수동 개수랑 자동 개수도 출력하는 거니까 맞추는게 좋을것같아!

로또티켓에 상태로 수동자동 여부를 넣을지 별도의 컬렉션으로 구분할지 고민중 ㅠㅠ

Gobukgol · 2020-06-04T14:21:11Z

src/main/java/lotto/dto/ManualLottoNumbers.java

+        this.manualLottoNumbers = Arrays.stream(manualLottoNumbers.split(MANUAL_LOTTO_NUMBERS_DELIMITER))
+                .map(number -> Integer.parseInt(number.trim()))
+                .map(LottoNumber::of)
+                .collect(toSet());


toSet하는 과정에서 정렬이 안돼서 LottoNumber에 Comparable 구현하구 treeSet 사용하면 될꺼야!

이민형 천재

Gobukgol · 2020-06-04T14:36:36Z

src/test/java/lotto/domain/LottoTicketTest.java

+        assertThatThrownBy(() -> new LottoTicket(numbers))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessage("로또티켓은 6개의 로또숫자로 이루어져야합니다.");
+    }


로또 번호 중복도 같은 로직 탈것같은데 그래도 명시적으로 써주면 좋을것 같아!

Gobukgol · 2020-06-04T14:45:07Z

src/main/java/lotto/domain/result/LottoPrize.java

+    MISS(0, Money.of(0));
+
+    private final int matchCount;
+    private final Money reward;


Money로 감싸는거 굳인듯!

오우 짱이네요! 하나 배워갑니다.

Gobukgol · 2020-06-11T06:43:19Z

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

+        int matchCount = 0;
+        for (int i = 0; i < COUNT_OF_LOTTO_NUMBER; i++) {
+            if (lottoTicket.lottoNumbers.get(i).equals(this.lottoNumbers.get(i))) {
+                matchCount++;
+            }
+        }
+        return matchCount;


인덴트 규칙! stream 으로 해결할 수 있지않을까?

헉스 나중에고쳐야지~하고 넘겼다 떙쿄 !!

public int getMatchCount(LottoTicket lottoTicket) { return Math.toIntExact(IntStream.range(0, COUNT_OF_LOTTO_NUMBER) .filter(index -> isSameLottoNumber(index, lottoTicket.lottoNumbers.get(index))) .count()); }

지금 이렇게밖에 생각이안나는데 더 좋은방법있을깡? ㅠㅠ

나도 당장은 이런식으로 했어 ㅋㅋㅋ

Gobukgol · 2020-06-11T06:49:15Z

src/test/java/lotto/domain/result/LottoPrizeTest.java

+class LottoPrizeTest {
+
+    @DisplayName("번호일치 갯수에 따른 LottoPrize를 반환한다")
+    @CsvSource({"3,flase,FIFTH", "5, false,THIRD", "5,true,SECOND",


여기 flase 로 오타있어서 제대로 들어오기 보단 false, true 가 둘다 아니여서 false로 들어가는거 같아.

ture 로 해도.

false 로 들어오니까 조심하는게 좋을 것 같아!

미쳐따 떙쿜ㅋㅋ

NDjust · 2020-05-28T08:29:13Z

src/main/java/com/javabom/lotto/domain/result/LottoPrize.java

+        if (matchCount == THIRD.hitCount) {
+            return findByBonus(matchBonus);
+        }
+        return Arrays.stream(LottoPrize.values())
+                .filter(lottoPrize -> lottoPrize.hitCount == matchCount)
+                .findAny()
+                .orElse(MISS);
+    }
+
+    private static LottoPrize findByBonus(boolean matchBonus) {
+        if (matchBonus) {
+            return SECOND;
+        }
+        return THIRD;
+    }


matchBonus 변수 자체가 보너스 번호 유무를 넘겨주는 건데, matchCount == THIRD.hitCount && matchBonus말고 한번 더 findByBonus로 빼신 이유가 있을까요??
개인적으로 메소드 없는게 더 파악이 잘되는 느낌이라서요!

NDjust · 2020-05-28T08:32:23Z

src/main/java/com/javabom/lotto/domain/result/BonusNumber.java

+public class BonusNumber {
+    private final int number;
+
+    public BonusNumber(int number) {


저도 찬인님 말씀대로 BonusNumber와 WinningTicket 모두 number(integer)값에 대한 유효성 검사가 들어가서 한번 더 객체로 감싸서 하면 좋다고 생각합니다! ㅎㅎ

위에꺼는답글이 안달아지내용!

matchBonus 변수 자체가 보너스 번호 유무를 넘겨주는 건데, matchCount == THIRD.hitCount && matchBonus말고 한번 더 findByBonus로 빼신 이유가 있을까요??
개인적으로 메소드 없는게 더 파악이 잘되는 느낌이라서요!

if(matchCount == THIRD.hitCount && matchBonus){ return SECOND; } if(matchCount == THIRD.hitCount && !matchBonus){ return THRID; }

보다는 위의 방식이 더 가독성있어보인다고 생각했어요!

BonusNumber는 지금은 없는 클래스예요! LottoNumber로 대체하고있어용

NDjust · 2020-05-28T08:35:46Z

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

+        return new LottoTickets(lottoTickets);
+    }
+
+    private boolean isEnoughChange(Money currentChange) {


Money에서 isEnougToBuy랑 같은 행위를 하는데, 한번 더 메소드로 빼신 이유가 있으신가요?! 궁금합니다!

이것도 지금 없어짐 메서드일거예여!

NDjust · 2020-06-12T08:09:29Z

src/main/java/lotto/domain/LottoStore.java

+        return manualLottoTickets.combine(autoLottoTickets);
+    }
+
+    private LottoTickets buy(final Money budget) {


buyAutoTickets라고 메소드 네이밍 바꾸는 건 어떨까요?? 처음에 약간 buy안에 buy메소드가 있어서 약간 오잉 재귀? 햇어요!

재귀보다는 오버로딩을 활용한건데 요즘 오버로딩을 자제하려고 노력하고있어요 ㅋㅋㅋ 나단님처럼 재귀로파악할 소지도 있으니까용!

NDjust · 2020-06-12T08:10:35Z

src/main/java/lotto/domain/result/LottoPrize.java

+    MISS(0, Money.of(0));
+
+    private final int matchCount;
+    private final Money reward;


오우 짱이네요! 하나 배워갑니다.

NDjust · 2020-06-12T08:17:53Z

src/main/java/lotto/domain/LottoNumber.java

+    }
+
+    public static LottoNumber of(final int value) {
+        return CACHE.computeIfAbsent(value, LottoNumber::new);


CACHE.computeIfAbsent(value, LottoNumber::new);

그냥 LottoNumber로 변환하는 것보다 이렇게 사용하면 이점이 있나요?? 궁금합니다!

첫번째 파라미터를 키로하는 Value가 Map에 없는경우 Map에 두번째 Functional에서 반환하는 인스턴스를 Map에 추가하고 반환해줘요!
null체크와 put을 동시에할 수 있는 유용한 메서드입니다!

와우 감사합니다!!

pci2676 · 2020-06-12T13:10:35Z

src/main/java/lotto/domain/LottoGameProperty.java

+public class LottoGameProperty {
+    public static final int LOTTO_NUMBER_BEGIN_BOUND = 1;
+    public static final int LOTTO_NUMBER_END_BOUND = 45;
+    public static final int COUNT_OF_LOTTO_NUMBER = 6;


이거 사실 LottoTicket이랑 AutoGenerator에서만 사용하는데 Lottoticket의 공개 상수로만 둬도 문제가 없을 것 같아
어차피 Auto쪽에서 LottoTicket을 알고 있거든

COUNT_OF_LOTTO_NUMBER 이것만!

오키오키! 상수클래스 없애벌여ㅑ지`

csbsjy added 5 commits May 21, 2020 17:16

docs : 기능요구사항 정리

eb48727

feature : InputView 작성, OutputView 작성(도메인로직제외)

ca50eee

feature : 1. 로또넘버 생성기, LottoTicket 일급컬렉션구현

4fd9cd8

feature : 당첨률까지 구현완료

dfee2bd

feature : 일단 완성

816f30f

csbsjy requested review from Gobukgol, NDjust and pci2676 May 26, 2020 08:27

csbsjy added 5 commits May 26, 2020 17:29

update : 자동 구현

ad70aa7

update : 자동 + 보너스 구현

9044919

refactor : 수익률 단위 수정

15e115d

refactor : 수익률 단위 수정 테스트코드수정

bed1357

refactor : 도메인 상수클래스 생성.

51a3697

malibinYun reviewed May 26, 2020

View reviewed changes

pci2676 requested changes May 27, 2020

View reviewed changes

csbsjy added 3 commits June 4, 2020 20:36

feature : 일단 완성

d44524b

refactor : jacoco build.gradle 수정

3b5feca

Gobukgol reviewed Jun 4, 2020

View reviewed changes

csbsjy added 2 commits June 9, 2020 20:20

feature : 순서와 숫자 모두 맞아야 맞추게 수정

6282d30

refactor : 다이어그램추가, 테스트코드에만 쓰이던 메서드 제거

2ef0714

csbsjy changed the title ~~로또(자동)~~ 로또(순서보장) Jun 11, 2020

Gobukgol reviewed Jun 11, 2020

View reviewed changes

refactor : 인덴트 Stream 수정.

efeb456

NDjust reviewed Jun 12, 2020

View reviewed changes

pci2676 approved these changes Jun 12, 2020

View reviewed changes



		public static void printLottoTicketNumbers(LottoTickets lottoTickets) {
		System.out.println(lottoTickets.count() + "개를 구매했습니다.");

Comments

Conversation

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

Uh oh!

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

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

csbsjy commented May 26, 2020 •

edited

Loading