[럿고] 로또 자동 생성 미션 제출합니다. #130
Conversation
Joseph415
| return new WinningBalls(winningBallsInput, bonusBall); | ||
| } catch (RuntimeException e) { | ||
| OutputView.printErrorMessage(e.getMessage()); | ||
| return generateWinningBalls(); |
There was a problem hiding this comment.
에러 발생시 다시 입력을 유도하는데요.
예상치 못한 예외가 터지면 무한 루프로 빠지는 코드로 생각됩니다. 어떻게 생각하시나요?
There was a problem hiding this comment.
Custom Exception만 처리하였습니다. 이걸 맞는 말씀이신지 모르겠네요! 확인부탁드릴께요!
There was a problem hiding this comment.
일단 이렇게 처리하면 최소한 알 수 없는 에러에도 generateWinningBalls()를 수행하지는 않겠네요.
| @CsvSource(value = {"1,2,3,4,5,6 FIRST_RANK","1,2,3,4,5,7 SECOND_RANK" | ||
| ,"1,2,3,4,5,10 THIRD_RANK","1,2,3,4,44,45 FOURTH_RANK" | ||
| ,"1,2,3,43,44,45 FIFTH_RANK", "1,2,40,41,42,43 NO_RANK"}, delimiter = ' ') | ||
| void select_rank_test(String input,WinningRank winningRank) { |
There was a problem hiding this comment.
테스트가 복잡해 보입니다.
랭크당 하나의 테스트 메서드로 나눠보는건 어떨까요?
There was a problem hiding this comment.
CsvSource를 이용하여, 이름을 지정하고 전반적으로 정리를 하였습니다. 제 생각은 CsvSource를 이용하는 것이 훨씬 괜찮다고 생각하는데... 혹시 리뷰어 분의 생각이 저와 다르다면 알려주세요~
| public PurchaseAmount(String purchaseAmount) { | ||
| this.purchaseAmount = InputValidationUtil.returnNumberWithNumberCheck(purchaseAmount); | ||
| InputValidationUtil.isPositiveNumber(this.purchaseAmount); | ||
| underLottoUnit(); |
There was a problem hiding this comment.
validLottoUnit() or checkLottoUnit() 와 같은 네이밍 방식은 어떠신가요?
There was a problem hiding this comment.
validateLottoUnit 으로 변경하였습니다.
| , THIRD_RANK(1_500_000,5) | ||
| , FOURTH_RANK(50_000,4) | ||
| , FIFTH_RANK(5_000,3) | ||
| ,NO_RANK(0,0); |
There was a problem hiding this comment.
컨벤션을 찾아보고 사용했어야 하는데, 놓쳤습니다!ㅠㅠ 수정하였습니다!
| import java.util.Arrays; | ||
|
|
||
| public enum WinningRank { | ||
| FIRST_RANK(2_000_000_000,6) |
There was a problem hiding this comment.
조건을 세번째 인자로 넣어서 처리해 보는건 어떨까요?
생각해 보시고 변경을 원하거나 원하지 않거나 하는 부분에 대해 의견이 있다면 알려주셔도 좋습니다ㅎㅎ
There was a problem hiding this comment.
세번째 인자로 넣어서 처리하니, 메소드 하나가 사라지네요! 스트림으로 처리하였습니다~ 감사합니다!
| .filter(result -> result.winningBallCount == correctNumber) | ||
| .filter(result -> result.existingBonusBall == isBonusNumber) |
| import java.util.stream.Collectors; | ||
|
|
||
| public class WinningBallsUtils { | ||
| private List<LottoBall> winningBalls; |
There was a problem hiding this comment.
정렬때문에 Set을 사용하지 않았는데, 생각해보니, WinningBall은 정렬을 할 필요가 없네요... ㅎㅎ DM으로 보내주신 답변과 같이 변경하였습니다~
| int[] winningBallInputs = {1, 2, 3, 4, 5, 6}; | ||
| int bonus = 7; | ||
|
|
||
| List<LottoBall> lottoTicket = Arrays.stream(lottoTicketNumbers) |
There was a problem hiding this comment.
LottoBalls 라는 객체를 만들어 lottoTicket, winningBallValues를 생성해 보도록 하면 코드가 훨씬 간결해 질 것 같네요.
There was a problem hiding this comment.
미리 다음주 미션을 살짝 한 느낌이네요~ 입력값을 받아서 Set를 생성할 수 있게 객체 생성하였습니다! 이렇게 하니깐, WinningBallUtil도 사라지네요! Util보다 객체가 더 어울려 보여서 둘이 기능을 합쳤습니다!
| WinningBalls winningBalls = new WinningBalls(winningBallValues, LottoBallFactory.findByLottoBall(bonus)); | ||
| int correctCount = winningBalls.hitLottoBalls(new LottoTicket(lottoTicket)); | ||
| boolean correctBonusNumber = winningBalls.hitBonusBall(new LottoTicket(lottoTicket)); |
There was a problem hiding this comment.
이 부분을 실제 메서드가 아닌 따로 변수를 할당 & 계산하며 처리한다면 제대로 된 메서드 기능 테스트로 보여 지지 않습니다. 실제 메서드를 사용하도록 바꿔주세요.
There was a problem hiding this comment.
원하시는것이 되었는지 모르겠습니다. 일단 SelectRank가 아닌, generateWinningRank 이걸로 변경하였습니다. 어차피 generateWinningRank안에서 SelectRank가 실행이 되어서 중복을 피할수 있었다고 생각합니다! 또한 generateWinningRank은 테스트가 되어있지 않더라고요! 그래서 둘이 같이 할 수 있는 방법으로 변경하였습니다~
|
이번에 첫번째 두번쨰 피드백을 보면서 많이 배웠습니다. 아직, 전반적으로 컨벤션에 대한 개념이 부족하고 중복코드와 테스트 코드와 가독성을 신경을 많이 못쓴거 같습니다. 2주차에는 이번에 피드백들을 보고 배운 것들을 적용해보겠습니다! 혹시 다른 피드백 있으시다면 또 남겨주세요! 감사합니다~ |
안녕하세요. 이번 미션 리뷰 잘 부탁드리겠습니다.
궁금한 사항은 따로 DM으로 물어보겠습니다!
감사합니다!