-
Notifications
You must be signed in to change notification settings - Fork 3
[기현]로또 구현 #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[기현]로또 구현 #10
Conversation
| public BonusNumber(String strBonusNumber, PrizeNumbers prizeNumbers) { | ||
| int bonusNumber = parseInt(strBonusNumber); | ||
| checkDuplicate(bonusNumber, prizeNumbers); | ||
| checkRange(bonusNumber); | ||
| this.bonusNumber = bonusNumber; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
보너스 숫자생성시 당첨번호를 받아야한다??
일단 이부분이 어색한거같아. 다르게 분리를 해야된다고 생각해.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 그렇네! 다르게 바꿔볼게!
| private boolean isNotRange(int bonusNumber) { | ||
| return bonusNumber < 1 || bonusNumber > 45; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1, 45 같은 정수값들이 다른사람들이 보았을 때 무슨 의미인지 알 수 없으니
상수로 선언을 하여서 의미를 부여하는것이 좋다고 생각해.
또, 범위값이 아니면 boolean을 던지는거보다 그냥 예외처리를 하는게 더 나아보여
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 그러네 바로 예외처리하면되는구나
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| public class Lotto { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 객체만 봤을 때 List를 들고있을 거라고 생각하지 못했어.
그리구 이름이 너무 추상적이라고 생각해. 더 구체적인 네이밍이 있을거야
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좀더 구체적인 네이밍을 찾아볼게!
| public int getLottoCount() { | ||
| return money / LOTTO_PRICE; | ||
| } | ||
|
|
||
| public int getRateOfProfit(int amountOfPrize) { | ||
| int profit = amountOfPrize - money; | ||
| if (profit < 0) { | ||
| return 0; | ||
| } | ||
| return (profit / money) * 100; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Money" 라는애가 티켓 갯수도 알려주고
"Money" 라는애가 수익률도 알려주는게
뭔가 이상하다고 생각해. 이름을 바꾸던지, 역할을 분리하던지 해야할거같아.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
역할을 분리해봐야겠다
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| public class MyLottoBundle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My라는 말은 필요없을거같아
| public void compileStatisticsOfPrize(MatchedCounts matchedCounts) { | ||
| prizeCounts.replaceAll((k, v) -> matchedCounts.getMatchedCount(k)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
축약하면 정말 알아보기 힘든거같아..
| prizeCounts.put(3, 0); | ||
| prizeCounts.put(4, 0); | ||
| prizeCounts.put(5, 0); | ||
| prizeCounts.put(6, 0); | ||
| prizeCounts.put(50, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 숫자들이 무엇을 의미하는지 처음 봤을 땐 잘 모르겠어
| FIRST(6) { | ||
| @Override | ||
| int calculate(int count) { | ||
| return 2000000000 * count; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분은 오버플로나
int는 21억 까지여서 20억 * n을 하게되면 의도치 않은 숫자가 나올꺼야
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 그러네 int형으로 하면 안되겠구나
| for (String strPrizeNumber : strPrizeNumbers) { | ||
| int prizeNumber = parseInt(strPrizeNumber); | ||
| checkRange(parseInt(strPrizeNumber)); | ||
| prizeNumbers.add(prizeNumber); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 메소드로 빼는 편이 나아보여
| void has(int lottoNumber, boolean expected) { | ||
| Lotto lotto = new Lotto( | ||
| Arrays.asList(1, 2, 3, 4, 5, 6)); | ||
| assertEquals(expected, lotto.has(lottoNumber)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aseertJ를 사용하는게 더 가독성이 좋아.
| public class LottoApplication { | ||
| private static final InputView inputView = new InputView(new Scanner(System.in)); | ||
|
|
||
| public static void main(String[] args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 메인 로직을 보면 전체 비지니스 플로우의 흐름을 알기 어렵습니다.
내부로 숨겨야할 구현들이 main에 너무 노출되어있어요
예를들어 MyLottoBundle은 LottoFactory의 createLotto를 호출해 로또를 주입받아 생성되는데,
이걸 LottoFactory안쪽으로 감추면 어떨까요
LottoShop lottoShop = new LottoShop(new RandomMachine());
MyLottoBundle bundle = lottoShop.buyLotto(money);랜덤 로또 머신을 가진 로또가게에 돈을 주면 로또를 살 수 있다.
이 문맥흐름이 더 자연스럽지 않나요?
굳이 money.getLottoCount()가 외부에 노출될 필요가 있을까요?
기본 문맥에 방해되는 세부 구현내용은 안쪽으로 감추는 연습을 하는게 좋을거같아요
밑에 내용들도 좀더 자연스러운 문맥으로 바꿔보세요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 맞습니다. 자연스러운 문맥흐름과 세부 구현내용을 안쪽으로 감춘다에 초점을 맞춰서 바꿔보겠습니다! 감사합니다!
| @@ -0,0 +1,17 @@ | |||
| package com.javabom.lotto.util; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일반적으로 팩토리 클래스는 util패키지에 두지 않습니다.
도메인을 생성하는 중요 클래스니, 도메인 패키지 안쪽으로 넣어서 관리하는게 좋아보이네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 감사합니다! 패키지 구조에 대해 공부해서 관리해보겠습니다!
| FIRST(6) { | ||
| @Override | ||
| int calculate(int count) { | ||
| return 2000000000 * count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
큰 숫자는 이런식으로도 2_000_000_000 표현가능합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 처음알았습니다! 좋은 정보 감사합니다 훨씬 보기 편해졌습니다!
| } | ||
| } | ||
|
|
||
| private void checkCanBuyLotto(int money) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
money라는 객체가 로또를 살수있는 금액인지 검사할 필요가 있을까요?
이 validation은 LottoFactory에 있는게 더 자연스러울거 같네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 감사합니다! Lotto구입에 관한 validation 수정하겠습니다!
| package com.javabom.lotto.domain; | ||
|
|
||
| public class Money { | ||
| private final int LOTTO_PRICE = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로또 가격을 Money객체가 가지고 있는게 이상하네요
LottoFactory가 가지고 있는게 맞을거같아요
| checkCount(strPrizeNumbers.size()); | ||
| List<Integer> prizeNumbers = new ArrayList<>(); | ||
| for (String strPrizeNumber : strPrizeNumbers) { | ||
| int prizeNumber = parseInt(strPrizeNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PrizeNumbers가 문자 하나하나에 대한 valid를 수행해야 할까요?
PrizeNumbers가 Integer가 아닌 PrizeNumber클래스로 래핑해서 parseInt, checkRange, isNotRange에 대한 역할들을 위힘하면 PrizeNumbers 자체는 로또를 맞추는 하나의 역할에만 집중할 수 있을거같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와 감사합니다 한번더 래핑해서 역할을 분리주면 되는군요! 수정해보겠습니다!
| } | ||
|
|
||
| public boolean has(int number) { | ||
| if (prizeNumbers.contains(number)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return prizeNumbers.contains(number) 으로 수정하는게 좋아요
|
|
||
| public final int prizeCount; | ||
|
|
||
| PrizeType(int prizeCount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생성자가 멤버변수보다 먼저 있는게 일반적인 컨벤션입니다
| import java.util.stream.Collectors; | ||
|
|
||
| public class ShuffleLottoNumbers implements PickedLottoNumbers { | ||
| private static final List<Integer> lottoNumbers = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수는 대문자로 사용하는게 자바 컨벤션입니다
|
|
||
| @DisplayName("생성한 로또번호와 값이잘 들어갔는지 확인한다.") | ||
| @Test | ||
| void get() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 테스트는 오히려 유지보수에 방해만 되는 테스트입니다.
위 로또번호를 확인하는 테스트로 충분히 해소되니 이 테스트는 삭제하세요
| } | ||
| } | ||
|
|
||
| private void checkRange(int bonusNumber) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
숫자에 대한 valid로직이 너무 중복된다는 생각이 드는데,
GameNumber라는 valid 검사들을 해주는 역할의 클래스를 만들어서 포함관계로 객체를 만드는건 어떨까요
BonusNumber는 GameNumber하나를 들고있고, PrizeNumbers는 여러개를 들고 있는 구조로 가는게 중복로직도 없애고 객체간 역할도 잘 나눠질것같네요
| return bonusNumber < 1 || bonusNumber > 45; | ||
| } | ||
|
|
||
| public boolean isValid(Lotto lotto, int matchedCount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isValid와 valid의 차이는 뭔가요?
일반적으로 valid는 void타입으로 생각되는데 굳이 함수를 쪼갤필요가 없어보이네요
| return lotto.has(bonusNumber) && matchedCount == 5; | ||
| } | ||
|
|
||
| public int get() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트용 get은 만들지 마세요
| try { | ||
| return Integer.parseInt(strBonusNumber); | ||
| } catch (NumberFormatException e) { | ||
| throw new NumberFormatException("보너스 번호는 숫자만 들어올 수 있습니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NumberFormatException을 NumberFormatException으로 던지는 건 이상하네요
IllegalArgumentException로 바꾸는게 좋아보입니다
| @@ -0,0 +1,7 @@ | |||
| package com.javabom.lotto.util; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로또를 생성하는 중요 비지니스 로직이니 domain패키지로 내리고 domain아래에서 패키지를 분리해보세요
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| public enum PrizeType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enums으로 따로 패키지를 빼신 이유가 있으신가용?? info 패키지 있어도 괜찮을 거 같아요!
| @Override | ||
| int calculate(int count) { | ||
| return 2000000000 * count; | ||
| public long getPrize() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
count 처럼 열거 타입의 상태값으로 가지게 아니라 getPrize() 메소드로 만드신 이유가 있으신가요?? 개인적으로 prize는 상태값으로 가지고 있고, getter로 빼는 것도 좋을 거 같습니다!
| public long getRateOfProfit(Money money) { | ||
| int amount = money.get(); | ||
| long profit = calculatePrize() - amount; | ||
| if (profit < FAIL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이익률 계산이 (상금으로 얻은 금액 /로또 구매한 금액)* 100 아닌가요?? calculatePrize에서 amount을 빼면 기능 요구사항에 적힌 출력 결과도 0%가 나와야 해요! 5000 - 14000 < 0 이니! 확인 부탁드립니당
| this.lottoResult = lottoResult; | ||
| } | ||
|
|
||
| public long getPrize() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FAIL이 PrizeType에 열거 타입으로 가지고 있는 건 어떨까요?! 그러면 if로 null값을 확인하는 부분도 없어질 수 있다고 생각합니다!
| private int gameNumber; | ||
|
|
||
| public GameNumber(String strGameNumber) { | ||
| Number number = new Number(strGameNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
number를 생성해서 문자를 숫자로 변환해서 get만 한다면 차라리 NumberConverter라는 클래스의 static메서드로 변환 메서드를 만드는게 좋아보이네요,
근데 제가생각할때 커스텀한 메시지를 내릴필요가 없다면 자연스럽게 numberFormatException이 터지는 구조가 좋아보여요.
| import java.util.List; | ||
|
|
||
| public class Lotto { | ||
| private List<Integer> lotto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lotto도 GameNumber를 들고있게 하면 좋을거같네요
| } | ||
|
|
||
| public boolean has(GameNumber gameNumber) { | ||
| return lotto.contains(gameNumber.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GameNumber equals가 재정의 되어 있고 lotto가 GameNumber 들고있다면 값을 get할필요없이 lotto.contains(gameNumber);로 바로 될거같네요.
| this.prizeCount = prizeCount; | ||
| } | ||
|
|
||
| public final int prizeCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
당첨 갯수와 상금을 둘다 멤버변수로 들고있는게 좋겠네요
|
|
||
| public final int prizeCount; | ||
|
|
||
| private static final Map<Integer, PrizeType> BY_PRIZE = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum자체가 이미 싱글톤 인스턴스로 메모리상에 존재하여 어플리케이션이 구동됩니다.
이런식으로 맵에 자기 멤버변수와 타입을 가질 필요가 없어요.
enum 사용법이 아직 어색한듯 보이니,
https://woowabros.github.io/tools/2017/07/10/java-enum-uses.html
위 블로그를 꼼꼼히 참고해보세요
|
|
||
| import java.util.List; | ||
|
|
||
| public class PrizeNumberInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PrizeNumberInfo와 PrizeNumbers를 나눌필요가 없어보여요.
오히려 객체분리때문에 결과구하는 로직이 더 지저분해졌어요.
PrizeNumbers 객체에
private final List prizeNumbers;
private final BonusNumber bonusNumber;
위 멤버변수를 가지고 정답을 맞추는 비지니스 로직만 가지도록 하고
validation체크와 GameNumber변환 작업은 당첨번호 생성기라는 클래스를 만들어
PrizeNumbers prizeNumbers =생성기.pickPrizeNumbers(List);의 구조가 훨씬 깔끔할거같네요
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| private void checkDuplicate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
당첨번호 중복검사기능을 하지 않네요
| import java.util.List; | ||
|
|
||
| public class ShuffleLottoNumber implements GenerateLottoNumbers { | ||
| private static final List<Integer> LOTTO_NUMBER = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GameNumber를 불변클래스로 만들고 1~45개짜리의 불변 객체를 싱글톤 객체로 띄워놓고,
로또에 사용한다면 불필요한 인스턴스들을 생성하지 않고 사용할 수 있을거같아요.
이 객체를 인터페이스 상속하지 않고,
GameNumberGenerator로 만들고 45개의 싱글톤 gameNumber를 들고있는 클래스로
Lotto생성과 당첨번호를 찾아주는 클래스로 만드는건 어떨까요.
public GameNumber getNumber(String number);
public GameNumber getNumber(int number);
그리고 랜덤하게 셔플하는 기능은 RandomMachine이라는 클래스에 위임하고,
LottoMachine->RandomLottoMachine
LottoMachine->FixLottoMachine
구조가 좋을듯 보이네요
|
|
||
| import com.javabom.lotto.domain.valid.Number; | ||
|
|
||
| public class Money { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
money.plus(money)
money.calculateRate(money)
기능들을 제공해서 get을 없애보는건 어떨까요
|
|
||
| public LottoResult matchLotto(Lotto lotto) { | ||
| int matchedCount = prizeNumbers.getMatchedCount(lotto); | ||
| if (bonusNumber.isValid(lotto, matchedCount)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2등 당첨구하는 로직이 BONUS_COUNT라는 특이 숫자때문에 너무 가독성이 떨어지네요.
PrizeType에 보너스볼 당첨 여부에 대한 플래그를 넣어놓는건 어떨까요.
public enum PrizeType{
FIRST(매칭숫자,당첨번호 플래그,상금),
public static PrizeType matchPrize(매칭숫자,당첨번호 플래그);
}위 구조면 matchLotto의 비지니스 로직을
int matchedCount = prizeNumbers.getMatchedCount(lotto);
boolean isMachedBonusBall = bonusNumber.matchBonusball(lotto);
PrizeType prizeType=PrizeType.matchPrize(matchedCount,isMachedBonusBall);
...구조로 갈 수 있을거같아요.
당첨이 안된 것도 상태도 이넘에 추가하면 좋을듯 보이네요
| return 5_000; | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
당첨이 안된 상태는 없어도 될까요?
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class GameNumber { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GameNumber, Number valid를 각각 분리하신 이유가 있나요?? LottoNumberValid로 묶어도 좋을 거 같아요! 그리고 GameNumber 경우에는 먼가 valid보다는 info 패키지에 있는게 맞지 않을까라고 개인적으로 생각합니다!
| import java.util.List; | ||
| import java.util.Scanner; | ||
|
|
||
| public class LottoApplication { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private final String THIRD_QUESTION = "보너스 번호를 입력해주세요."; | ||
| private final String DELIMITER = ", "; | ||
|
|
||
| private static final String DELIMITER = ", "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DELIMITER에 공백을 넣지 않고 ","이렇게 두고 입력값을 trim으로 공백을 없애고 입력값을 받는 건 어떨까요??
실행때 당첨번호에 1,2,3,4,5,6 이렇게 입력하니 당첨 번호를 1개 입력했다고 에러가 터져버려서요!
| } | ||
|
|
||
| public void buy(List<LottoTicket> lottoTickets) { | ||
| if (lottoTicketBundle != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private LottoTicketBundle lottoTicketBundle = new LottoTicketBundle(new ArrayList<>());lottoTicketBundle 필드 값을 null로 초기화 하는 것보단, 위와 같이 빈 LottoTicketBundle 클래스를 넣어서 null 검사하지 않고 바로 티켓들을 넣는 것 어떨까요?!
널값 체크하는 코드가 좀 어색한 거 같아서요!
| public class GameNumber { | ||
| public static final int MIN_NUMBER = 1; | ||
| public static final int MAX_NUMBER = 45; | ||
| public static final int COUNT = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GameNumber가 COUNT을 가지고 있는게 맞을까요?!
GameNumber 객체는 로또 게임에 사용되는 "번호"값을 가지고 있는 객체인거 같은데 해당 COUNT 필드는 GameNumber가 아닌 LottoTicket에 가지고 있는게 맞지 않을까 생각합니다!
| LottoResultBundle lottoResultBundle = customer.confirmLottoResult(prizeNumberBundle); | ||
|
|
||
| OutputView.printResults(lottoResultBundle, money); | ||
| OutputView.printResults(lottoResultBundle, amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
구입 금액을 그대로 수익률 계산에서 사용하시는 거 같은데, 투입금액이 6500 이렇게 들어가면 수익률 계산이 올바르게 되지 않습니다!
그래서 LottoResultBundle에서 amount 값을 입력 받지 않고(입력을 받는다면 구매한 금액을 받아야합니다!), 구입한 티켓 개수로 수익률을 계산하면 올바르게 계산이 될 거 같습니다:)
|
|
||
| public LottoResult searchResult(LottoTicket lottoTicket) { | ||
| prizeNumbers.compareTo(lottoTicket); | ||
| boolean bonusStatus = lottoTicket.contains(bonusNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요구사항에 보너스 번호는 마지막 번호로 일치 여부를 확인한다고 했는데, 단순히 포함 여부로 판단하면 요구사항대로 실행이 안됩니다!
| public List<GameNumber> generate() { | ||
| Collections.shuffle(GAME_NUMBERS); | ||
| return GAME_NUMBERS.stream() | ||
| .limit(6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생성 개수 제한도 GameNumber 숫자 범위를 상수로 뺀거 처럼 상수로 처리하면 좋을 거 같습니다!
GameNumberConverter에 있는 오류 검증 로직을 변환 받은 값을 받은 객체가 검증 일치 여부를 확인할 때 index에 의존하지 말고 OrderGameNumber를 통해 일치 여부 확인 Customer클래스가 갖고있던 LottoTicketBundle 분리
LottoShop에 대한 테스트코드가 없었네요! 하하! 추가합니다!!

돈이 부족한지 검증하는 로직이 있음
로또를 구입할 때 수동 로또 번호를 건네줌
생성된 로또 티켓을 받아 자신의 티켓묶음에 더함
고객이 들어오면 LottoMachine을 통해 로또 티켓을 건네줌
수동 로또 티켓과 자동 로또 티켓을 생성해줌
자동으로 로또 번호를 생성해줌
로또 번호들의 중복 검사 로직이 있음
각각의 로또 티켓과 당첨 번호를 비교해서 로또 결과 묶음을 반환해줌
내부 클래스로 GameNumberCache가 있음
갯수 검증 및 중복 검사 로직이 있음
보너스 번호와 당첨 번호의 중복 검사 로직이 있음 로또 결과를 알려줌
일치 갯수와 보너스 유무를 통해 값을 반환해줌
당첨된 총 상금을 알려줌
투자한 금액과 총 상금을 비교해서 수익률을 알려줌
당첨 번호와의 일치 갯수를 알려줌