[디디] 로또 미션 제출합니다. #129
[디디] 로또 미션 제출합니다. #129
Conversation
구입 금액이 1000원 단위로 들어오는 경우 예외처리 구입 금액에 문자열을 입력받은 경우 예외처리
| } | ||
|
|
||
| private void checkSize(List<Number> numbers) { | ||
| if (numbers.size() != 6) { |
There was a problem hiding this comment.
매직 넘버는 상수로 선언해 의미를 더 명확히 드러내주시면 좋을 것 같습니다.
| * @param count | ||
| * @return Lottos | ||
| */ | ||
| public static Lottos create(int count) { |
There was a problem hiding this comment.
로또는 만드는 로직이 create() 와 중복됩니다. 중복을 제거해보면 어떨까요?
| try { | ||
| Statistic statistic = winningLotto.isWinningLotto(lotto); | ||
| statistic.count(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
catch부분을 비워두기 보다 예외가 발생하면 예외를 내보내 이 클래스를 사용하는 곳으로 알려줘야 하지 않을까요?
|
|
||
| private final int matchingNumbers; | ||
| private final int prize; | ||
| private int count = 0; |
There was a problem hiding this comment.
enum은 인스턴스가 1개만 생성되기 때문에 멀티쓰레딩 환경에서 동시성 문제가 발생할 여지가 있는 코드입니다.
이 클래스에서는 상금에 대한 상수만 관리하고 통계는 다른 클래스에서 하도록 바꿔보면 어떨까요?
| static { | ||
| for (int i = MIN_LOTTO_NUMBER; i <= MAX_LOTTO_NUMBER; i++) { | ||
| numbers.put(i, new Number(i)); | ||
| } | ||
| } |
There was a problem hiding this comment.
이렇게 생성한 숫자를 사용하는 곳은 LottoFactory 밖에 없는 것 같은데요~ 그럼 해당 클래스에서 숫자를 생성하는게 더 클래스의 응집도를 높일 수 있을 것 같습니다. Number 클래스는 원시값(int)만 래핑하는 클래스로 바꿔보면 어떨까요?
| return new GameResultDto(makeStatistics(), calculateProfit()); | ||
| } | ||
|
|
||
| ; |
| FOUR(4, 50000), | ||
| FIVE(5, 1500000), | ||
| BONUS(5, 3000000), | ||
| SIX(6, 2000000000); |
There was a problem hiding this comment.
숫자 리터럴은 _ 를 통해 자릿수를 구분하는 용도로 사용할 수도 있습니다.
| public Statistic isWinningLotto(Lotto lotto) throws Exception { | ||
| int numberOfMatch = lotto.compare(winningNumbers); | ||
| if (checkSecond(lotto, numberOfMatch)) | ||
| return Statistic.BONUS; |
There was a problem hiding this comment.
현재 보너스가 일치하는지 분기문을 통해 확인하고 있습니다. Statistic 에서 보너스 일치 여부를 관리해서 분기문을 제거해보면 어떨까요?
| return Arrays.stream(values()) | ||
| .filter(statistic -> statistic.matchingNumbers == numberOfMatch) | ||
| .findFirst() | ||
| .orElseThrow(Exception::new); |
| @Test | ||
| @DisplayName("로또 장수 만큼 로또를 반환하는지") | ||
| void Lottos_Test() { | ||
| Lottos lottos = LottoFactory.create(3); |
There was a problem hiding this comment.
이 케이스는 LottoFactory 의 테스트에 위치하는 게 더 적절하지 않을까요?
Lotto 숫자 갯수 상수화 Lottos 생성 과정 중복 제거 (create()) 처리불가능한 에러 제거(Rank의 DEFAULT 추가) Count 클래스 구현을 통해 멀티스레딩환경에서의 enum의 문제점 보완 Number 클래스 내의 NumberPool을 NumberFactory로 이동 세미콜론제거 숫자리터럴 자릿수 구분 Rank 보너스 일치 여부 추가 Lottos 테스트 위치 변경
| this.profit = 0; | ||
| } | ||
|
|
||
| public void calculateProfit(PurchaseMoney money) { |
There was a problem hiding this comment.
상태값을 가지는 클래스보다 입출력이 있는 메서드로 만들면 테스트 및 구현이 한결 쉬워집니다.
여기도 profit을 인스턴스 변수로 저장하기 보다 메서드에서 바로 반환하도록 바꿔보면 좋을 듯 합니다.
| Stream<Rank> rankStream = Arrays.stream(values()) | ||
| .filter(statistic -> statistic.matchingNumbers == numberOfMatch); | ||
|
|
||
| if (numberOfMatch != FIVE_MATCH) { | ||
| return rankStream | ||
| .findFirst() | ||
| .orElse(DEFAULT); | ||
| } | ||
| return rankStream | ||
| .filter(statistic -> statistic.bonusMatching == isBonus) | ||
| .findFirst() | ||
| .orElse(DEFAULT); |
There was a problem hiding this comment.
아래와 같은 메서드를 Rank 클래스에 만들면 중복되는 로직을 개선해볼 수 있을 듯 합니다. (불필요한 클래스 변수도 제거되겠네요)
private boolean checkBonus(boolean isBonus) {
if (this == BONUS) {
return this.bonusMatching == isBonus
}
return true;
}| public boolean isNotDefault() { | ||
| return matchingNumbers != 0; | ||
| } | ||
|
|
||
| public boolean isBonusMatching() { | ||
| return matchingNumbers == FIVE_MATCH && bonusMatching; | ||
| } |
| List<Rank> ranks = Arrays.asList(Rank.values()); | ||
| GameResult gameResult = new GameResult(ranks); |
Profit을 return 하는 메서드 구현 후 필드에서 제거 checkBonus 메서드를 통해 중복 제거 GameResult 디폴트생성자로 변경 enum 메소드 수정
| .filter(statistic -> statistic.bonusMatching == isBonus) | ||
| return Arrays.stream(values()) | ||
| .filter(rank -> rank.matchingNumbers == numberOfMatch) | ||
| .filter(rank->rank.checkBonus(isBonus)) |
| } | ||
|
|
||
| public boolean isBonus(){ | ||
| if(this==BONUS){ |
There was a problem hiding this comment.
띄어쓰기 컨벤션 위반입니다. 그리고 if분기문을 축약해 쓸 수 있겠네요~
No description provided.