[로또] 신승철 미션 제출합니다. #5
Conversation
|
|
||
| private static final Scanner SCANNER = new Scanner(System.in); | ||
|
|
||
| public static int inputMoney() { |
There was a problem hiding this comment.
짱구님 안녕하세요!
혹시 모든 inputView 메소드를 static으로 만드셨는데 어떤 의도이신지 궁금합니다.
또 만약 console이 아니라 다른 방식으로 입력을 받게 된다면 어떻게 구현하실 생각이신가요??
| return new LottoResult(results); | ||
| } | ||
|
|
||
| private static Map<LottoRank, Integer> groupBy(final Lottos autoLottos, final WinningLotto winningLotto) { |
There was a problem hiding this comment.
변수명 autoLottos 가 아닌 다른 걸 사용하는게 적절해보입니다!
manualLotto가 들어오는 경우도 있으니까요
|
|
||
| public class LottoResult { | ||
|
|
||
| private final Map<LottoRank, Integer> rankResults; |
There was a problem hiding this comment.
저는 코드읽으면서 이 Map의 Integer값이 무엇을 의미하는지 OutputView를 가서야 이해가 됐어요.
|
|
||
| private static void validateNumberRange(final int value) { | ||
| if (value < MIN_LOTTO_NUMBER || value > MAX_LOTTO_NUMBER) { | ||
| throw new IllegalArgumentException("로또 번호는 1 ~ 45 사이의 숫자여야 합니다."); |
There was a problem hiding this comment.
로또 번호가 1 ~ 45가 아닌 다른 범위로 바뀔때 여러 부분을 변경해야할 수 있을 것 같아요!
| public static LottoRank of(final int hitCount, final boolean hasBonusNumber) { | ||
| return Arrays.stream(LottoRank.values()) | ||
| .filter(lottoRank -> lottoRank.isSameHitCount(hitCount)) | ||
| .filter(lottoRank -> !lottoRank.equals(SECOND) || hasBonusNumber) |
There was a problem hiding this comment.
보너스 넘버를 가져야 하는 rank가 추가될때마다 조건이 계속 추가 될 수 있을 것 같아요
LottoRank에 보너스 번호와 관련된 변수를 하나 추가 하는 건 어떨까요??
| this.lottos = lottos; | ||
| } | ||
|
|
||
| public int countPurchasedLottos() { |
There was a problem hiding this comment.
이 함수명 어떻게 생각하시나요?
저는 굳이 count, Purchased같은 말을 사용하지 않아도 된다고 생각해요.
이렇게 이름을 정하면 다른 곳에서 사용해야 할때 countXXXLottos()라는 함수를 같은 기능인데 생성해야하니까요.
저라면 getLottosSize. fetchLottosSize 처럼 size를 가져온다 라는 의미만 담을 것 같아요
| @@ -0,0 +1,21 @@ | |||
| package lotto; | |||
|
|
|||
| class MoneyValidator { | |||
There was a problem hiding this comment.
이 객체도 new 키워드로 instance를 생성할 수 있을 것 같아요.
|
|
||
| static void validate(final int money) { | ||
| validateNegative(money); | ||
| validateZero(money); |
There was a problem hiding this comment.
짱구님은 validateZero라는 함수명에 대해 어떻게 생각하시나요?
validate는 검증하다 라는 뜻이고 Zero는 0을 의미하는데
0이라는 것을 검증한다? 보다 0이 아니라는 것을 검증한다 라는 의미로
validateNotZero와 같은 이름이 더 괜찮지 않을까? 생각이 들었습니다.
|
|
||
| static void validate(final int money) { | ||
| validateNegative(money); | ||
| validateZero(money); |
There was a problem hiding this comment.
또 이 부분 두 함수로 나누는 것과 0보다 작거나 같은지를 검증하는 함수 하나를 만드는 것 중
어느게 괜찮을까요?
음수라는 것과 0이라는 것을 꼭 나눠서 에러 메세지를 표시하고 싶다면 그렇게 하는게 맞는 것 같고
아니라면 하나로 만들어서 "돈은 0보다 작거나 같으면 안됩니다." 로 표현할 수 있을 것 같아요
| } | ||
|
|
||
| private static void printRankResult(final LottoRank lottoRank, final int rankCount) { | ||
| if (lottoRank == LottoRank.SECOND) { |
There was a problem hiding this comment.
lottoRank 바로 == 으로 비교하는 것과 lottoRank에게 같은지 묻는 것 중에 어떤게 괜찮을까요??
| } | ||
|
|
||
| public Lottos purchaseAutoLottos() { | ||
| final int purchasableCount = money.calculatePurchasableCount(LOTTO_PRICE); |
There was a problem hiding this comment.
짱구님의 의견이 궁금합니다
제 생각에는 purchaseAutoLottos라면 lottos를 생성하고 돈을 minus하는 것만 있어야 할 것같아요
몇개를 구매할지는 내부에서 계산하는 것보다 변수로 받는게 낫지 않을까요?
| } | ||
|
|
||
| public List<LottoNumber> lottoNumbers() { | ||
| return this.lottoNumbers; |
| getLottoRanks().stream() | ||
| .filter(LottoRank::hasReward) | ||
| .forEach(lottoRank -> printRankResult(lottoRank, lottoResult.CountBy(lottoRank))); | ||
| System.out.printf("총 수익률은 %.2f입니다.\n", lottoResult.calculateProfitRate()); |
There was a problem hiding this comment.
수익이 이득인지 손해인지 표현해야하는 요구사항이 구현되어있지 않습니다!
No description provided.