[로또] 임규리 3주차 미션 제출합니다. #644
Conversation
fix: validate 함수 내 validateDuplicate 추가
…expecting code to raise a throwable 오류로 인함
…rror: expecting code to raise a throwable로 인함
|
|
||
| private final int money; | ||
| private final int matchLottoCount; | ||
| private final BiPredicate<Integer, Boolean> isMatch; |
There was a problem hiding this comment.
ENUM 에서 함수형 인터페이스를 사용할 수 있는지 몰랐네요..! 2등과 3등 당첨을 구분하는게 조금 어려웠는데 이런 방법도 있구나 알아갑니다.
There was a problem hiding this comment.
로직의 흐름은 전체적으로 잘 이해됩니다!
다만 요구사항중 에러발생한 부분에서 입력을 다시받는건 구현이 안된 것 같네요, 4주차엔 한번 고민해보시고 다른 코드도 리뷰하면서 적용해보시면 좋을 것 같습니다!
| public static void printResult(PrizeResult prizeResult, Rate rate) { | ||
| System.out.println(OUTPUT_FIRST_MESSAGE); | ||
| Arrays.stream(Prize.values()) | ||
| .filter(prize -> !prize.equals(Prize.EMPTY)) | ||
| .forEach(prize -> System.out.println(getPrintResultPrize(prize, prizeResult))); | ||
| System.out.printf((OUTPUT_RETURN_RATE) + NEWLINE, rate.getRate()); | ||
| } | ||
|
|
There was a problem hiding this comment.
저는 계속 고민하고 있는 부분인데, View 에서 Domain의 getter 함수를 사용하는게 좋은 방법은 아니라고 생각이 듭니다.
4주차엔 한번 다른 방법을 고민해보고 적용해보시는건 어떨까요?
| package lotto.view; | ||
|
|
||
| import camp.nextstep.edu.missionutils.Console; | ||
|
|
||
| public class InputView { | ||
| protected String inputValue() { | ||
| return Console.readLine(); | ||
| } | ||
| } |
There was a problem hiding this comment.
혹시 InputView 를 따로 만든다음 다른 InputView 에서 extends 한 이유가 따로 있을까요? 따로 이유가 생각나지 않아서 질문드려요..!
| public int getTicket() { | ||
| return money / 1000; | ||
| } | ||
| } |
| private static final int LOTTO_SIZE = 6; | ||
| private static final int MIN_RANGE = 1; | ||
| private static final int MAX_RANGE = 45; |
There was a problem hiding this comment.
로또 숫자 범위나 사이즈의 경우 generator 등 따른 클래스에서도 사용되는 것 같은데 enum으로 처리해두는 게 어떨까요?
There was a problem hiding this comment.
enum을 접근제한자를 통해 사용 패키지를 제한하면 Enum 처리가 괜찮을 것 같습니다
There was a problem hiding this comment.
클래스 이름 관련해서, MainController는 너무 일반적이고 포괄적인 이름인 것 같습니다. 도메인 이름을 포함하면 좀 더 구체적일 것 같습니다. LottoController처럼요.
| private Money getLottoMoney() { | ||
| InputBuyLottoView inputBuyLottoView = new InputBuyLottoView(); | ||
| int money = inputBuyLottoView.getValue(); | ||
| return new Money(money); | ||
| } | ||
|
|
||
| private Lottos getLottos(Money money) { | ||
| LottoGenerator lottoGenerator = new LottoGenerator(); | ||
| return new Lottos(lottoGenerator.generateLottoGroup(money.getTicket())); | ||
| } | ||
|
|
||
| private WinningLotto getWinningLotto() { | ||
| InputWinningLottoView inputWinningLottoView = new InputWinningLottoView(); | ||
| InputBonusNumberView inputBonusNumberView = new InputBonusNumberView(); | ||
| List<Integer> winningNumbers = inputWinningLottoView.getValue(); | ||
| Integer bonusNumber = inputBonusNumberView.getValue(); | ||
| return new WinningLotto(winningNumbers, bonusNumber); | ||
| } |
There was a problem hiding this comment.
지금 메서드 추춣하신 것들을 보면 거의 다 객체를 생성하는 로직들을 담고 있는 것으로 보입니다. 저는 객체 생성은 컨트롤러보다는 객체 자신에게 있다고 보는데요. 정적 팩토리 메서드를 이용해서 매개변수들을 받고 바로 객체를 생성하면 불필요한 코드들도 줄고 책임이 명확해 질 것 같습니다.
| return new WinningLotto(winningNumbers, bonusNumber); | ||
| } | ||
|
|
||
| private void calcLottoResult(PrizeResult prizeResult, WinningLotto winningLotto, Lottos lottos) { |
There was a problem hiding this comment.
변수 ,클래스, 함수 이름을 지을 때 코드를 보는 사람 모두가 알고 있지 않다고 생각되면 줄임말을 사용하지 않는 것이 좋습니다. 여기서는 calc 보다는 그대로 calculate를 사용하는 것이 더 적절해보여요. 이름이 길어진다고 해도 줄임말을 사용해 의미가 불분명해지는 것보다는 나을 것 같습니다.
| } | ||
|
|
||
| private void validateRange(Integer number) { | ||
| if (!(MIN_RANGE <= number && number <= MAX_RANGE)) { |
There was a problem hiding this comment.
| if (!(MIN_RANGE <= number && number <= MAX_RANGE)) { | |
| if (number < MIN_RANGE || number > MAX_RANGE) { |
if문은 긍정 조건문인 것이 의도를 파악하기 더 쉬운 것 같아요
| } | ||
|
|
||
| public Lotto generateLotto() { | ||
| LottoNumbersGenerator lottoNumbersGenerator = new LottoNumbersGenerator(); |
| public int getMatchLottoCount() { | ||
| return matchLottoCount; | ||
| } | ||
| } |
| private final double rate; | ||
|
|
||
| public Rate(Money money, PrizeResult prizeResult) { | ||
| this.rate = (getAllMoney(prizeResult) * 0.1) / money.getTicket(); |
|
|
||
| public class WinningLotto { | ||
| private static final Integer MIN_RANGE = 1; | ||
| private static final Integer MAX_RANGE = 45; |
| validateDuplicate(inputValue); | ||
| } | ||
|
|
||
| private void validateFormat(String inputValue) { |
| return String.format(OUTPUT_RESULT_MESSAGE_1 | ||
| , prize.getMatchLottoCount() | ||
| , String.format("%,d", prize.getMoney()) | ||
| , prizeResult.getPrizeCount(prize)); |
| } | ||
|
|
||
| private void updatePrizeCount(Prize prize) { | ||
| prizeResult.put(prize, prizeResult.get(prize) + 1); |
| } | ||
|
|
||
| private void validateFormat(String inputValue) { | ||
| if (!PATTERN.matcher(inputValue).matches()) { |
There was a problem hiding this comment.
| if (!PATTERN.matcher(inputValue).matches()) { | |
| if (!inputValue.matches(PATTERN)) { |
PATTERN을 String으로 선언하고 위처럼 사용하면 더 간결하게 처리할 수 있습니다!
| } | ||
| } | ||
|
|
||
| private void validateDuplicate(String inputValue) { |
There was a problem hiding this comment.
view에 너무 많은 책임이 있는 것은 아닐까?하는 고민을 해보면 좋겠습니당
| package lotto.exception; | ||
|
|
||
| public class BuyLottoMoneyFormatException extends IllegalArgumentException { | ||
| private static final String ERROR_MESSAGE = "[ERROR] 1,000원 단위의 금액을 입력해야합니다."; |
There was a problem hiding this comment.
IllegalArgumentException을 상속한 각각의 커스텀 예외들은 모두 메시지로 "[ERROR]"를 포함하고 있는데요.
이 경우 해당 메시지의 세부 사항이 변경되었을 때 일일이 수정을 해 주어야 하는 단점이 있을 것 같아요.
해당 값을 별도의 상수 클래스로 분리해 사용하면 좋을 것 같습니다.
| private static final String ERROR_MESSAGE = "[ERROR] 1,000원 단위의 금액을 입력해야합니다."; | |
| private static final String ERROR_MESSAGE = LottoConst.ERROR_PREFIX + "1,000원 단위의 금액을 입력해야합니다."; |
| package lotto.exception; | ||
|
|
||
| public class InvalidRangeLottoNumberException extends IllegalArgumentException { | ||
| private static final String ERROR_MESSAGE = "[ERROR] 로또 번호가 1 ~ 45 사이의 수가 아닙니다."; |
There was a problem hiding this comment.
이런 부분들 역시 요구사항의 변경에 유연하게 대처하기 위해선 상수를 활용해 줄 수 있을 것 같아요.
| private static final String ERROR_MESSAGE = "[ERROR] 로또 번호가 1 ~ 45 사이의 수가 아닙니다."; | |
| private static final String ERROR_MESSAGE = String.format("[ERROR] 로또 번호가 %d ~ %d 사이의 수가 아닙니다.", LottoConst.LOTTO_RANGE_START, LottoConst.LOTTO_RANGE_END); |
| } | ||
|
|
||
| public Lotto generateLotto() { | ||
| LottoNumbersGenerator lottoNumbersGenerator = new LottoNumbersGenerator(); |
There was a problem hiding this comment.
해당 클래스는 내부적으로 Randoms 클래스를 사용하는 클래스인데요.
이 클래스의 경우 다음의 두 가지 이유로 의존성을 낮추는 것을 추천드립니다!
- 제어할 수 없는 값을 사용한다.
- 외부 API를 사용한다.
자세한 이유에 대해서는 제 설명보다, 우아한 테크 세미나 영상을 하나 공유 드릴게요.
[우아한테크세미나] 테크 리더 3인이 말하는 "개발자 원칙"
첫 번째 세션인 "제어할 수 없는 것에 의존하지 않기" 입니다.
비즈니스 로직이 사용하는 부분 중 제어할 수 없는 부분은 추상화를 제공하여 테스트를 진행할 때는 적절한 다른 클래스로 갈아끼울 수 있습니다!
| private void validateRange(List<Integer> numbers) { | ||
| numbers.forEach(this::validateRange); | ||
| } | ||
|
|
||
| private void validateRange(Integer number) { | ||
| if (!(MIN_RANGE <= number && number <= MAX_RANGE)) { | ||
| throw new InvalidRangeLottoNumberException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
메서드 명이 중복되고 있는데요, 같은 메서드를 오버로딩 하는 것 역시 하나의 방법이지만 메서드의 역할을 더 잘 나타내기 위해선 디테일한 부분을 나타낼 수있도록 서로 다른 메서드명을 주는 것도 좋을 것 같아요.
| private void validateRange(List<Integer> numbers) { | |
| numbers.forEach(this::validateRange); | |
| } | |
| private void validateRange(Integer number) { | |
| if (!(MIN_RANGE <= number && number <= MAX_RANGE)) { | |
| throw new InvalidRangeLottoNumberException(); | |
| } | |
| } | |
| private void validateNumberList(List<Integer> numbers) { | |
| numbers.forEach(this::validateNumberInRange); | |
| } | |
| private void validateNumberInRange(Integer number) { | |
| if (!(MIN_RANGE <= number && number <= MAX_RANGE)) { | |
| throw new InvalidRangeLottoNumberException(); | |
| } | |
| } |
| } | ||
|
|
||
| public List<Lotto> getLottos() { | ||
| return lottos; |
There was a problem hiding this comment.
저는 getter 사용시에는 외부에서 해당 객체의 변경을 일으킬 수 없도록 조심하는 편인데요.
이 경우 역시 getLottos()를 통해 가져온 List에 값을 추가하는 등의 변경이 가능할 것 같습니다.
다음과 같이 불변 리스트를 반환해 보는 건 어떨까요?
| return lottos; | |
| return Collections.unmodifiableList(lottos); |
또는, 수정을 하더라도 해당 객체는 영향 받지 않도록 새로운 객체를 생성하여 반환해 줄 수도 있을 것 같아요.
|
|
||
| public static Prize getPrize(final int matchLottoCount, final boolean containBonusNumber) { | ||
| return Arrays.stream(Prize.values()) | ||
| .filter(prize -> prize.isMatch.test(matchLottoCount, containBonusNumber)) |
There was a problem hiding this comment.
enum의 필드로 BiPredicate를 사용하고, 해당 필드를 사용해 각각의 결과를 판별하는 것은 정말 마음에 드네요!! 👍
| private final Map<Prize, Integer> prizeResult; | ||
|
|
||
| public PrizeResult() { | ||
| prizeResult = new EnumMap<Prize, Integer>(Prize.class); |
| private void updatePrizeCount(Prize prize) { | ||
| prizeResult.put(prize, prizeResult.get(prize) + 1); | ||
| } | ||
|
|
||
| public void calcPrizeResult(WinningLotto winningLotto, Lottos lottos) { | ||
| for (Lotto lotto : lottos.getLottos()) { | ||
| Prize prize = Prize.getPrize(lotto.getMatchLottoCount(winningLotto), | ||
| lotto.isContain(winningLotto.getBonusNumber())); | ||
| updatePrizeCount(prize); | ||
| } | ||
| } |
There was a problem hiding this comment.
정-말 사소한 이야기지만, private 메서드인 updatePrizeCount가 public 메서드인 calcPrizeResult에게만 딱 한번 호출이 된다면, updatePrizeCount 메서드가 calcPrizeResult 메서드 바로 아래에 위치하면 좀 더 일기 편해진다고 생각해요!
There was a problem hiding this comment.
2등과 3등을 비교하는 메서드 중에 가장 깔끔한 코드인 것 같습니다..!
| int result = 0; | ||
|
|
||
| for (Prize prize : Prize.values()) { | ||
| result += prizeResult.getPrizeCount(prize) * prize.getMoney(); |
There was a problem hiding this comment.
result가 int형으로 정의되어 있는데요.
1등의 상금이 20억임을 고려하면 1등이 2번 이상 당첨되었을 경우 오버플로우의 위험이 존재할 것 같아요!
제가 놓치고 있는 부분이 있을까요?
|
|
||
| import camp.nextstep.edu.missionutils.Console; | ||
|
|
||
| public class InputView { |
There was a problem hiding this comment.
InputView를 상속하는 모든 클래스가 공통적으로 getValue()와 같은 동일한 일부 메서드를 구현하고 있는데요.
이 경우 차라리 InputView를 인터페이스나 추상 클래스로 만들어서 getValue()의 구현을 강제시키는 것이 더 좋을 것 같다고 생각합니다!
| private static final int LOTTO_SIZE = 6; | ||
| private static final int MIN_RANGE = 1; | ||
| private static final int MAX_RANGE = 45; |
There was a problem hiding this comment.
enum을 접근제한자를 통해 사용 패키지를 제한하면 Enum 처리가 괜찮을 것 같습니다
| private final Map<Prize, Integer> prizeResult; | ||
|
|
||
| public PrizeResult() { | ||
| prizeResult = new EnumMap<Prize, Integer>(Prize.class); |
|
|
||
| private final int money; | ||
| private final int matchLottoCount; | ||
| private final BiPredicate<Integer, Boolean> isMatch; |
| private void updatePrizeCount(Prize prize) { | ||
| prizeResult.put(prize, prizeResult.get(prize) + 1); | ||
| } | ||
|
|
||
| public void calcPrizeResult(WinningLotto winningLotto, Lottos lottos) { | ||
| for (Lotto lotto : lottos.getLottos()) { | ||
| Prize prize = Prize.getPrize(lotto.getMatchLottoCount(winningLotto), | ||
| lotto.isContain(winningLotto.getBonusNumber())); | ||
| updatePrizeCount(prize); | ||
| } | ||
| } |
There was a problem hiding this comment.
2등과 3등을 비교하는 메서드 중에 가장 깔끔한 코드인 것 같습니다..!
| numbers = sorted(numbers); | ||
| this.numbers = numbers; |
There was a problem hiding this comment.
정말 사소하지만 this.numbers = sorted(numbers); 처럼 한번에 작성해도 될 것 같습니다!
| import java.util.regex.Pattern; | ||
| import lotto.exception.SingleNumberFormatException; | ||
|
|
||
| public class InputBonusNumberView extends InputView { |
There was a problem hiding this comment.
입력 기능마다 클래스를 분리해서 각각 유효성검사를 진행하는 방법은 좋은 방법이라고 생각이되는데 생성해야되는 객체가 많이지는 것에 고민이 되네요.. 어떻게 생각하시나요??
| return Stream.of(result.split(",")) | ||
| .map(Integer::parseInt) | ||
| .toList(); |
There was a problem hiding this comment.
입력에서 변환하지 해주는 부분은 비즈니스 로직이 아닐까 생각되는 것 같습니다!
| private static final String OUTPUT_RESULT_MESSAGE_2 = "%d개 일치, 보너스 볼 일치 (%s원) - %d개"; | ||
| private static final String OUTPUT_RETURN_RATE = "총 수익률은 %.1f%%입니다."; | ||
|
|
||
| public static void printResult(PrizeResult prizeResult, Rate rate) { |
There was a problem hiding this comment.
해당 클래스에서 메서드가 모두 static으로 사용하고 계시는데 이유가 있나요..??
이번 미션을 진행하면서 제일 고민되었던 점은 '과연 이 코드가 남들에게도 잘 읽힐까?'였습니다. 저는 제 코드라 익숙하지만 다른 분들이 보시기에는 어떨지 궁금해요! 함수명, 클래스명, 코드 흐름 등이 괜찮은지 여쭙고 싶습니다 :)
3주차도 다들 고생많으셨습니다!