Step2 - 로또(자동) #3462
Step2 - 로또(자동) #3462
Conversation
| - 기능 목록 | ||
| * [X] 구입 금액을 입력받는다. | ||
| * [X] 구입 금액을 바탕으로 구매 수량을 판단한다. | ||
| * [X] 중복이 없는 6개의 숫자(1~45)로 구성된 로또를 구매 수량만큼 발행한다. | ||
| * [X] 지난 주 당첨 번호를 입력받는다. | ||
| * [X] 입력받은 당첨 번호를 바탕으로 당첨 통계를 구한다. | ||
| * [X] 구매 금액 대비 수익률을 계산한다. |
| public Lotto(){ | ||
| shuffle(); | ||
| this.lottoNumberList = numList.subList(0, 6); | ||
| sort(); | ||
| } |
There was a problem hiding this comment.
생성자에 로직은 제거하고 필드에 할당할 인자를 받으면 어떨까요?
로직이 필요하다면 정적 팩토리 메서드를 활용해 보시길 추천드립니다
| import java.util.List; | ||
|
|
||
| public class MyLotto { | ||
| private static int LOTTO_PRICE = 1000; |
There was a problem hiding this comment.
공유하는 자원을 변경하면 어떤 이슈가 발생할까요?
static을 사용할 경우 final 도 함께 사용하시길 권장드립니다
| public MyLotto(int money){ | ||
| for(int i=0; i<money/LOTTO_PRICE; i++){ | ||
| lottoList.add(new Lotto()); | ||
| } | ||
| } |
There was a problem hiding this comment.
모든 클래스의 생성자에 로직이 있네요
객체의 필드를 주입받지 않고 내부에서 생성하는 이유가 궁금합니다
| public PrizeResult getResult(List<Integer> winningNumbers){ | ||
| PrizeResult prizeResult = new PrizeResult(); | ||
| for(Lotto lotto : lottoList){ | ||
| prizeResult.addPrizeResult(matchResult(winningNumbers, lotto)); | ||
| } | ||
|
|
||
| return prizeResult; | ||
| } | ||
|
|
||
| private MatchInfo matchResult(List<Integer> winningNumbers, Lotto lotto){ | ||
| return MatchInfo.checkMatch(lotto.getMatchCount(winningNumbers)); | ||
| } |
| public PrizeResult(){ | ||
| for(MatchInfo matchInfo : MatchInfo.values()){ | ||
| prizeResultInfoList.put(matchInfo, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
컨벤션이 지켜지지 않은 코드가 많은 것 같아요
세이브 또는 커밋 전에 ide 의 reformat code 기능을 활용해 보시면 좋을 것 같아요
| public PrizeResult(){ | |
| for(MatchInfo matchInfo : MatchInfo.values()){ | |
| prizeResultInfoList.put(matchInfo, 0); | |
| } | |
| } | |
| public PrizeResult() { | |
| for (MatchInfo matchInfo : MatchInfo.values()) { | |
| prizeResultInfoList.put(matchInfo, 0); | |
| } | |
| } |
| public double getEarnRate(int inputMoney){ | ||
| int income = prizeResultInfoList.entrySet() | ||
| .stream() | ||
| .mapToInt(matchInfoIntegerEntry -> matchInfoIntegerEntry.getKey().getReward() * matchInfoIntegerEntry.getValue()) |
There was a problem hiding this comment.
스트림 내부에서의 변수 명도 의미있게 작성해 보시면 좋을 것 같아요
Map 자료 구조를 사용한다고 해도 IntegerEntry 표현은 없어도 괜찮지 않을까요?
| @DisplayName("로또 1장 가격 1000원 및 금액에 해당하는 로또 발급 여부 테스트") | ||
| @ParameterizedTest() | ||
| @ValueSource(ints = {14000, 8000, 999, 2000}) | ||
| void lottoCountTest(int money){ | ||
| MyLotto myLotto = new MyLotto(money); | ||
| assertThat(myLotto.getLottoList().size()).isEqualTo(money/LOTTO_PRICE); | ||
| } |
There was a problem hiding this comment.
테스트는 클래스별로 만들어 주세요 (MyLottoTest, MatchInfoTest)
테스트를 검증할 때 로직을 사용하지 마시고 명시된 값을 활용하면 어떨까요?
단위 테스트는 프로덕션 코드를 검증하는 것인데 검증부의 로직때문에 실패하면 신뢰할 수 있는 테스트로 보기 어려울 것 같아요
14000원이면 14장, 8000원이면 8장 처럼 누가봐도 명확한 값들로 테스트해 보시면 좋을 것 같아요
| @DisplayName("로또 중복 번호 존재 유무 테스트") | ||
| @RepeatedTest(value = 10) | ||
| void lottoNumberDuplicateTest(){ | ||
| Lotto lotto = new Lotto(); | ||
| assertThat(new HashSet<>(lotto.getLottoNumberList()).size()).isEqualTo(LOTTO_NUMBER_COUNT); | ||
| } |
There was a problem hiding this comment.
테스트 코드 작성자 외에 lotto 객체가 중복되지 않는 6개 숫자를 가진다는 사실을 어떻게 알 수 있을까요?
생성자 내부에 로직으로 숨겨져 있어서 lotto 객체 생성시 중복되지 않는 6개의 번호를 가진다는 사실을 알 수가 없습니다
lotto를 생성할 때 5개, 7개 숫자를 주입한다거나, 중복된 숫자가 포하된 6개 숫자를 주입하면 객체 생성 시 예외가 발생함을 검증하면 어떨까요?
| void lottoEarnRateTest(int inputMoney){ | ||
| List<MatchInfo> matchInfoList = new ArrayList<>(); | ||
| PrizeResult prizeResult = new PrizeResult(); | ||
| for(int i=0; i<inputMoney/1000; i++){ | ||
| int rand = (int) (Math.random()*7); | ||
| MatchInfo matchInfo = MatchInfo.checkMatch(rand); | ||
| matchInfoList.add(matchInfo); | ||
| prizeResult.addPrizeResult(matchInfo); | ||
| } | ||
|
|
||
| int reward = matchInfoList.stream() | ||
| .mapToInt(MatchInfo::getReward) | ||
| .sum(); | ||
|
|
||
| assertThat(prizeResult.getEarnRate(inputMoney)).isEqualTo((double) (reward/inputMoney)); | ||
| } |
There was a problem hiding this comment.
테스트가 실패하고 있어요
커밋 전에 모든 테스트가 성공하는지 확인해 보시면 좋을 것 같아요
No description provided.