Open
Conversation
day024
reviewed
May 7, 2024
There was a problem hiding this comment.
controller에선 int price같은 변수 선언을 아예 하지 않는 게 좋은 지에 대해서 질문 주셨는데요.
저도 이 부분을 고민하였지만, 기능 구현에 한계가 있어 결국 작성할 때는 변수 선언을 진행 하였습니다ㅠㅠ.
저의 생각으론 최대한 지양하는 것이 좋을 것 같다고 생각합니다! 컨트롤러는 최대한 가볍게 작성하는 것이 좋다고 생각하기 때문이에요.
아래 MVC 패턴에 관한 글인데 한번 보시면 좋을 것 같아요. 규원님도 이부분을 어떻게 생각하지는지도 궁금합니다!
There was a problem hiding this comment.
constant class를 통해 상수를 정리하셨는데 이 덕분에 코드가 깔끔해진 것 같아요 👍
Comment on lines
+6
to
+20
| public LottoMoney(String inputAmount) { | ||
| try { | ||
| int amount = Integer.parseInt(inputAmount); | ||
| validateAmount(amount); | ||
| this.amount = amount; | ||
| } catch (NumberFormatException e) { | ||
| throw new IllegalArgumentException(Constant.PRICE_VALUE_EXCEPTION, e); | ||
| } | ||
| } | ||
|
|
||
| private void validateAmount(int amount) { | ||
| if (amount % Constant.LOTTO_PRICE != 0 || amount < Constant.LOTTO_PRICE) { | ||
| throw new IllegalArgumentException(Constant.LOTTO_PRICE_EXCEPTION); | ||
| } | ||
| } |
src/main/java/model/BuyLotto.java
Outdated
Comment on lines
39
to
57
| public List<Lotto> generateLotto() { | ||
| List<Lotto> lottos = convertManualLottoList(manualLottoList); | ||
|
|
||
| for (int i = 0; i < lottoCount; i++) { | ||
| lottos.add(new Lotto(generateRandomNumbers())); | ||
| } | ||
|
|
||
| return lottos; | ||
| } | ||
|
|
||
| private List<LottoNumber> generateRandomNumbers() { | ||
| Collections.shuffle(numberList); | ||
| return numberList.subList(Constant.ZERO_COUNT, Constant.LOTTO_SIZE) | ||
| .stream() | ||
| .map(n -> new LottoNumber(String.valueOf(n))) | ||
| .sorted(Comparator.comparingInt(LottoNumber::getNumber)) // 정렬 | ||
| .collect(Collectors.toList()); | ||
| } | ||
| } |
There was a problem hiding this comment.
BuyLotto에서 로또 구매와 생성이 모두 이루어지고 있는데요 두가지를 분리해보는것은 어떨까요?
Comment on lines
+41
to
+62
| public void updateWinningStates(List<Lotto> userLottos) { | ||
| userLottos.forEach(userLotto -> { | ||
| int matchedNumbers = compareHowManyNumberSame(userLotto); | ||
| boolean containsBonus = userLotto.getNumbers().stream().anyMatch(number -> number.getNumber() == bonusBall.getNumber()); | ||
|
|
||
| if (matchedNumbers == Constant.SIX_COUNT) { | ||
| updateWinningState(LottoPrize.FIRST); | ||
| } | ||
| if (matchedNumbers == Constant.FIVE_COUNT && containsBonus) { | ||
| updateWinningState(LottoPrize.SECOND); | ||
| } | ||
| if (matchedNumbers == Constant.FIVE_COUNT && !containsBonus) { | ||
| updateWinningState(LottoPrize.THIRD); | ||
| } | ||
| if (matchedNumbers == Constant.FOUR_COUNT) { | ||
| updateWinningState(LottoPrize.FOURTH); | ||
| } | ||
| if (matchedNumbers == Constant.THREE_COUNT) { | ||
| updateWinningState(LottoPrize.FIFTH); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
내부에 코드가 길어지는 것 같은데 더 간단하게 매소드를 쪼개보면 어떨까요?
Author
There was a problem hiding this comment.
쪼갤려고 시도를 해봤는데 들여쓰기 조건이 생각보다 까다로워서 일단 이렇게 뒀습니다ㅠ
한번 더 생각해보겠습니다!
There was a problem hiding this comment.
Constant에 상수를 잘 정리해두셔서 ResultView가 한층더 보기 명확해진 것 같습니다👍👍
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
안녕하세요 리뷰어님.
저는 우선 저번 리뷰 때 MVC 패턴에 대한 이해가 완벽하지 않은 것 같다고 느껴 이번 과제에선 MVC 패턴을 조금 더 신경쓰면서 진행해 보았습니다.
기능 구현을 먼저 하고 그 이후에 원시값과 문자열을 포장하여 조금 난잡할 수도 있겠지만 양해 부탁드립니다!
신경쓴 부분들
궁금한 부분
-controller에선 int price같은 변수선언을 아예 하지 않는게 좋은지
갑사합니다!