-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[로또] 염규영 미션 제출합니다. #1344
base: main
Are you sure you want to change the base?
[로또] 염규영 미션 제출합니다. #1344
Conversation
…th winning numbers
public static void main(String[] args) { | ||
RandomNumberGenerator randomNumberGenerator = new RandomNumberGenerator(); | ||
LottoNumberSorter lottoNumberSorter = new LottoNumberSorter(); | ||
LottoTicketStore lottoTicketStore = new LottoTicketStore(randomNumberGenerator, lottoNumberSorter); | ||
|
||
NumberValidator numberValidator = new NumberValidator(); | ||
DuplicateValidator duplicateValidator = new DuplicateValidator(); | ||
WinningNumberSeparator winningNumberSeparator = new WinningNumberSeparator(); | ||
PurchaseAmountValidator purchaseAmountValidator = new PurchaseAmountValidator(numberValidator); | ||
|
||
OutputView outputView = new OutputView(); | ||
InputView inputView = new InputView(outputView, purchaseAmountValidator, winningNumberSeparator, | ||
duplicateValidator); | ||
|
||
LottoPurchaseService lottoPurchaseService = new LottoPurchaseService(inputView, outputView, lottoTicketStore); | ||
LottoGameService lottoGameService = new LottoGameService(inputView); | ||
LottoResultCalculator lottoResultCalculator = new LottoResultCalculator(); | ||
LottoStatisticsService lottoStatisticsService = new LottoStatisticsService(lottoResultCalculator, outputView); | ||
LottoController lottoController = new LottoController(lottoGameService, lottoPurchaseService, | ||
lottoStatisticsService); | ||
|
||
lottoController.play(); | ||
} |
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.
리팩토링이 필요해 보인다. 15줄을 넘기기도 했다.
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 void play() { | ||
int purchaseAmount = lottoPurchaseService.purchaseForLottos(); | ||
List<Lotto> lottos = lottoPurchaseService.buyLottos(purchaseAmount); | ||
WinningNumber winningNumber = lottoGameService.setWinningNumber(); | ||
LottoResult lottoResult = lottoGameService.playLottoGame(lottos, winningNumber); | ||
lottoStatisticsService.summarizeStatistics(purchaseAmount, lottos, winningNumber); | ||
} |
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.
개인적으로 맘에 드는 부분이다... service단을 잘 나누었다고 생각한다 😅
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.
service단을 객체가 아닌 기능별로 나누신 이유가 있을까요?
private boolean canDivideWithPrice(int purchaseAmount) { | ||
return purchaseAmount % LottoConstantNumbers.LOTTO_PRICE.getValue() == 0; | ||
} | ||
|
||
private boolean isExceedMaxPurchaseAmount(int purchaseAmount) { | ||
return purchaseAmount > LottoConstantNumbers.MAX_PURCHASE_AMOUNT.getValue(); | ||
} |
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.
boolean으로 검증하고 싶었는데, 그냥 각 메서드별로 throw하는 게 나았나 싶다
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.
각 boolean값을 지역변수로 가져와 한번에 throw하는 방안을 고려할 수 있을것 같습니다.
MATCH_3("3개 일치 (5,000원) - %d개"), | ||
MATCH_4("4개 일치 (50,000원) - %d개"), | ||
MATCH_5("5개 일치 (1,500,000원) - %d개"), | ||
MATCH_5_BONUS("5개 일치, 보너스 볼 일치 (30,000,000원) - %d개"), | ||
MATCH_6("6개 일치 (2,000,000,000원) - %d개"), |
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 getPurchaseAmount() { | ||
while (true) { | ||
outputView.printPurchaseAmountMessage(); | ||
String input = readLine(); | ||
|
||
try { | ||
return purchaseAmountValidator.validateInput(input); | ||
} catch (InvalidPurchaseAmountException e) { | ||
outputView.printInvalidPurchaseAmountMessage(); | ||
} catch (NegativePurchaseAmountException e) { | ||
outputView.printInvalidPurchaseAmountMessage(); | ||
} catch (NotDivisibleByLottoPriceException e) { | ||
outputView.printNotDivisibleByLottoPriceMessage(); | ||
} catch (MaxPurchaseExceedException e) { | ||
outputView.printMaxPurchaseExceedMessage(); | ||
} | ||
} | ||
} | ||
|
||
public List<Integer> getWinningNumbers() { | ||
while (true) { | ||
outputView.printToGetWinningNumbers(); | ||
String input = readLine(); | ||
List<Integer> numbers = winningNumberSeparator.splitByComma(input); | ||
try { | ||
return duplicateValidator.validateNoDuplicates(numbers); | ||
} catch (NumberFormatException e) { | ||
outputView.printOutOfRangeNumberMessage(); | ||
} catch (LottoNumberSizeException e) { | ||
outputView.printInvalidLottoNumberSizeMessage(); | ||
} catch (OutOfRangeNumberException e) { | ||
outputView.printOutOfRangeNumberMessage(); | ||
} catch (DuplicateException e) { | ||
outputView.printDuplicateNumberMessage(); | ||
} | ||
} | ||
} | ||
|
||
public int getBonusNumber(List<Integer> winningNumbers) { | ||
while (true) { | ||
outputView.printToGetBonusNumbers(); | ||
String input = readLine(); | ||
|
||
try { | ||
int bonusNumber = Integer.parseInt(input); | ||
duplicateValidator.validateNoOverlapWithWinningNumbers(bonusNumber, winningNumbers); | ||
return bonusNumber; | ||
} catch (NumberFormatException e) { | ||
outputView.printOutOfRangeNumberMessage(); | ||
} catch (OutOfRangeNumberException e) { | ||
outputView.printOutOfRangeNumberMessage(); | ||
} catch (DuplicateException e) { | ||
outputView.printDuplicateNumberMessage(); | ||
} | ||
} | ||
} |
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.
나눠주세요.
public void printPrizeStatistics(Map<Prize, Integer> prizeCounts) { | ||
StringBuilder statisticBuilder = new StringBuilder(); | ||
statisticBuilder.append(WINNING_STATISTICS.getMessage()) | ||
.append('\n') | ||
.append(DIVIDER.getMessage()) | ||
.append('\n') | ||
.append(String.format(ConsoleMessage.MATCH_3.getMessage(), prizeCounts.get(Prize.FIFTH))) | ||
.append('\n') | ||
.append(String.format(ConsoleMessage.MATCH_4.getMessage(), prizeCounts.get(Prize.FOURTH))) | ||
.append('\n') | ||
.append(String.format(ConsoleMessage.MATCH_5.getMessage(), prizeCounts.get(Prize.THIRD))) | ||
.append('\n') | ||
.append(String.format(ConsoleMessage.MATCH_5_BONUS.getMessage(), prizeCounts.get(Prize.SECOND))) | ||
.append('\n') | ||
.append(String.format(ConsoleMessage.MATCH_6.getMessage(), prizeCounts.get(Prize.FIRST))) | ||
.append('\n'); | ||
System.out.print(statisticBuilder); | ||
} |
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.
print메서드를 좀더 제네릭하게 만들면 어떨까요?
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.
아쉽습니다. 이번 주는 충분한 시간을 갖길 바랍니다.
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.
3주차도 고생하셨습니다.
50인의 코드리뷰는 많이 무섭네요..
그럼에도 불구하고 좋은 퀄리티의 코드를 작성하신거 같습니다.
4주차도 화이팅 입니다.
public static void main(String[] args) { | ||
RandomNumberGenerator randomNumberGenerator = new RandomNumberGenerator(); | ||
LottoNumberSorter lottoNumberSorter = new LottoNumberSorter(); | ||
LottoTicketStore lottoTicketStore = new LottoTicketStore(randomNumberGenerator, lottoNumberSorter); | ||
|
||
NumberValidator numberValidator = new NumberValidator(); | ||
DuplicateValidator duplicateValidator = new DuplicateValidator(); | ||
WinningNumberSeparator winningNumberSeparator = new WinningNumberSeparator(); | ||
PurchaseAmountValidator purchaseAmountValidator = new PurchaseAmountValidator(numberValidator); | ||
|
||
OutputView outputView = new OutputView(); | ||
InputView inputView = new InputView(outputView, purchaseAmountValidator, winningNumberSeparator, | ||
duplicateValidator); | ||
|
||
LottoPurchaseService lottoPurchaseService = new LottoPurchaseService(inputView, outputView, lottoTicketStore); | ||
LottoGameService lottoGameService = new LottoGameService(inputView); | ||
LottoResultCalculator lottoResultCalculator = new LottoResultCalculator(); | ||
LottoStatisticsService lottoStatisticsService = new LottoStatisticsService(lottoResultCalculator, outputView); | ||
LottoController lottoController = new LottoController(lottoGameService, lottoPurchaseService, | ||
lottoStatisticsService); | ||
|
||
lottoController.play(); | ||
} |
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_PRICE(1_000), | ||
MAX_PURCHASE_AMOUNT(100_000), | ||
LOTTO_NUMBERS_COUNT(6), | ||
MIN_LOTTO_NUMBER(1), | ||
MAX_LOTTO_NUMBER(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.
숫자 가독성을 위해 '_'를 붙인 점 좋네요!
public void play() { | ||
int purchaseAmount = lottoPurchaseService.purchaseForLottos(); | ||
List<Lotto> lottos = lottoPurchaseService.buyLottos(purchaseAmount); | ||
WinningNumber winningNumber = lottoGameService.setWinningNumber(); | ||
LottoResult lottoResult = lottoGameService.playLottoGame(lottos, winningNumber); | ||
lottoStatisticsService.summarizeStatistics(purchaseAmount, lottos, winningNumber); | ||
} |
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.
service단을 객체가 아닌 기능별로 나누신 이유가 있을까요?
|
||
public static Prize findPrize(int matchCount, boolean matchBonus) { | ||
return Arrays.stream(values()) | ||
.filter(prize -> prize.matchCount == matchCount && prize.matchBonus == matchBonus) |
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 canDivideWithPrice(int purchaseAmount) { | ||
return purchaseAmount % LottoConstantNumbers.LOTTO_PRICE.getValue() == 0; | ||
} | ||
|
||
private boolean isExceedMaxPurchaseAmount(int purchaseAmount) { | ||
return purchaseAmount > LottoConstantNumbers.MAX_PURCHASE_AMOUNT.getValue(); | ||
} |
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.
각 boolean값을 지역변수로 가져와 한번에 throw하는 방안을 고려할 수 있을것 같습니다.
public int getPurchaseAmount() { | ||
while (true) { | ||
outputView.printPurchaseAmountMessage(); | ||
String input = readLine(); | ||
|
||
try { | ||
return purchaseAmountValidator.validateInput(input); | ||
} catch (InvalidPurchaseAmountException e) { | ||
outputView.printInvalidPurchaseAmountMessage(); | ||
} catch (NegativePurchaseAmountException e) { | ||
outputView.printInvalidPurchaseAmountMessage(); | ||
} catch (NotDivisibleByLottoPriceException e) { | ||
outputView.printNotDivisibleByLottoPriceMessage(); | ||
} catch (MaxPurchaseExceedException e) { | ||
outputView.printMaxPurchaseExceedMessage(); | ||
} | ||
} | ||
} | ||
|
||
public List<Integer> getWinningNumbers() { | ||
while (true) { | ||
outputView.printToGetWinningNumbers(); | ||
String input = readLine(); | ||
List<Integer> numbers = winningNumberSeparator.splitByComma(input); | ||
try { | ||
return duplicateValidator.validateNoDuplicates(numbers); | ||
} catch (NumberFormatException e) { | ||
outputView.printOutOfRangeNumberMessage(); | ||
} catch (LottoNumberSizeException e) { | ||
outputView.printInvalidLottoNumberSizeMessage(); | ||
} catch (OutOfRangeNumberException e) { | ||
outputView.printOutOfRangeNumberMessage(); | ||
} catch (DuplicateException e) { | ||
outputView.printDuplicateNumberMessage(); | ||
} | ||
} | ||
} | ||
|
||
public int getBonusNumber(List<Integer> winningNumbers) { | ||
while (true) { | ||
outputView.printToGetBonusNumbers(); | ||
String input = readLine(); | ||
|
||
try { | ||
int bonusNumber = Integer.parseInt(input); | ||
duplicateValidator.validateNoOverlapWithWinningNumbers(bonusNumber, winningNumbers); | ||
return bonusNumber; | ||
} catch (NumberFormatException e) { | ||
outputView.printOutOfRangeNumberMessage(); | ||
} catch (OutOfRangeNumberException e) { | ||
outputView.printOutOfRangeNumberMessage(); | ||
} catch (DuplicateException e) { | ||
outputView.printDuplicateNumberMessage(); | ||
} | ||
} | ||
} |
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 void printPrizeStatistics(Map<Prize, Integer> prizeCounts) { | ||
StringBuilder statisticBuilder = new StringBuilder(); | ||
statisticBuilder.append(WINNING_STATISTICS.getMessage()) | ||
.append('\n') | ||
.append(DIVIDER.getMessage()) | ||
.append('\n') | ||
.append(String.format(ConsoleMessage.MATCH_3.getMessage(), prizeCounts.get(Prize.FIFTH))) | ||
.append('\n') | ||
.append(String.format(ConsoleMessage.MATCH_4.getMessage(), prizeCounts.get(Prize.FOURTH))) | ||
.append('\n') | ||
.append(String.format(ConsoleMessage.MATCH_5.getMessage(), prizeCounts.get(Prize.THIRD))) | ||
.append('\n') | ||
.append(String.format(ConsoleMessage.MATCH_5_BONUS.getMessage(), prizeCounts.get(Prize.SECOND))) | ||
.append('\n') | ||
.append(String.format(ConsoleMessage.MATCH_6.getMessage(), prizeCounts.get(Prize.FIRST))) | ||
.append('\n'); | ||
System.out.print(statisticBuilder); | ||
} |
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.
print메서드를 좀더 제네릭하게 만들면 어떨까요?
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.
안녕하세요 코드 리뷰 반사하고 갑니다!😁 급하게 하신 코드라고 하기엔 잘 하신 것 같아요! 너무 고생 많으셨고 이번주도 화이팅입니다!!👍
} | ||
|
||
public void play() { | ||
int purchaseAmount = lottoPurchaseService.purchaseForLottos(); |
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.
purchaseForLottos
라는 메서드명이 아래의 buyLottos
와 혼동될 수 있을 것 같아요! 반환되는 amount
가 포함된 메서드명으로 네이밍해보는
것은 어떠실까요?
|
||
public enum ErrorConstants { | ||
ERROR_MESSAGE_PREFIX("[ERROR]"), | ||
SPACE(" "); |
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.
PREFIX에 공백을 함께 넣지 않고 따로 빼신 이유가 있으실까요?!
@@ -0,0 +1,37 @@ | |||
package lotto.service; | |||
|
|||
import static lotto.constants.LottoConstantNumbers.*; |
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시 와일드 카드 사용은 지양하는 것이 좋다고 합니다!
prizeCountMap.put(prize, 0); | ||
} | ||
return prizeCountMap; | ||
} |
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.
LottoResult
도메인 객체를 생성할 때와 로직이
겹치는 것 같은데 Service
로 분리하신 이유가 있으실까요?
import java.util.List; | ||
|
||
public class LottoNumberSorter { | ||
public List<Integer> sortInAscendingOrder(List<Integer> numbers) { |
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.
LottoNumberSorter
는 로또 번호를 정렬하는 일만 하는데 객체로 만드신 이유가 있으신지 궁금합니다! 저는 개인적으로 util
클래스는 도메인의 로직과 연관되면 안 된다고 생각해서 클래스명을 NumberSorter
정도로 하면 더 좋지 않았을까 의견 드려봐요!
if (numberValidator.isNegativeNumber(parsedInput)) { | ||
throw new InvalidPurchaseAmountException(); | ||
} | ||
if (!canDivideWithPrice(parsedInput)) { |
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.
if문의 조건 부분에 부정형은 가독성이 떨어지는 듯 한데 메서드명을 canNotDivideWithPrice
로 해주고 내부 로직을 수정해주시는 것도 좋을 것 같아요!
public String getMessage() { | ||
return message; | ||
} | ||
} |
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으로 이렇게 관리하는 방법도 좋은 것 같네요!
} | ||
} | ||
} | ||
} |
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.
중복되는 while
~ try
~ catch
부분만 메서드로 나눠주셔도 조금 더 좋아질 것 같아요!
😥 자기 반성
핑계겠지만.. 평일 일정과 약 50인의 코드 리뷰 진행 때문에 바쁘다고 구현을 월요일 오후 9시에 시작했다.
구현도 오전 2시에 마쳤을 뿐더러 테스트 코드, 소감문은 당연히 적지 못했다.
심지어 기본 제공된 테스트도 하나 통과하지 못한다.
구현한 코드 마저 깔끔하게 해내지 못한 것이 너무나도 훤히 보인다 ...
이번 주는 코드 리뷰 비중을 줄이고, 쉬는 시간도 4주차 과제에 투자해서 성공적으로 구현하도록 하자.
+추후 리팩토링 예정
📂 패키지 구조
📌 계획