[서브웨이] 3주차 -로또 코드리뷰 요청입니다. #146
Conversation
| @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 = ':') |
There was a problem hiding this comment.
리팩토링하면서 enum상수값을 잘못 바꿔주었네요,, 테스트를 돌려본줄 알았는데 죄송합니다 ㅠ
다음에는 다시한번확인하겠습니다.!
| WinningBalls winningBalls = new WinningBalls(winningBallValues, bonus); | ||
| int correctCount = winningBalls.hitLottoBalls(new LottoTicket(lottoTicket)); | ||
| boolean correctBonusNumber = winningBalls.hitBonusBall(new LottoTicket(lottoTicket)); | ||
| Assertions.assertThat(winningRank).isEqualTo(WinningRank.determineRank(correctCount, correctBonusNumber)); |
There was a problem hiding this comment.
예상값을 뒤에 넣어주셔야 Junit테스트 실패시
Expected가 winningRank값
Actual이 WinningRank.determineRank(correctCount, correctBonusNumber) 값이 나옵니다 :)
Assertions.assertThat(WinningRank.determineRank(correctCount, correctBonusNumber)).isEqualTo(winningRank);
There was a problem hiding this comment.
아 사소한것에 실수가 있었네요 assertj 다시 한번 공부하겠습니다 :)
|
|
||
| public static WinningRank determineRank(int correctNumber, boolean isBonusNumber) { | ||
| return Arrays.stream(values()) | ||
| .filter(result -> result.winningBallCount == correctNumber && result.isBonusBall == isBonusNumber) |
There was a problem hiding this comment.
filter 내부가 명시적으로 보기 힘들어요 😭
메소드 분리해 어떤 판별을 하는건지 명시적으로 보이게 하면 좋을 것 같아요.
| import java.util.List; | ||
|
|
||
| public class WinningBalls { | ||
| private final List<LottoBall> winningBalls; |
There was a problem hiding this comment.
LottoTicket과 winningBalls의 다른점은 무엇일까요? 🤔
There was a problem hiding this comment.
LottoTicket은 티켓을 만들어 주는 역할을 위해 만들었고
WinningBalls는 지난주 당첨번호티켓를 담기위해 만든 클래스였습니다..혹시 두 클래스를 합처야 하나요?
우선은 의미를 명확하게 하기 위해 WinningTicket 이라고 클래스명을 바꾸겠습니다!
There was a problem hiding this comment.
혹시 일급컬랙션에 인스턴스 변수가 final로 선언이 안되었던것이 다른점이였나요? 수정했습니다!
-> 본질적으로 같다는 의미
|
|
||
| private void validateWinningBallsWithDuplicatedBonusBall() { | ||
| if (winningBalls.contains(this.BonusBall)) { | ||
| throw new DuplicationException("보너스 볼이 중복입니다. 당첨 번호를 다시 입력해주세요."); |
There was a problem hiding this comment.
CustomException으로 추가적으로 해주는일이 없다면 표준 예외를 사용하는 것이 어떨까요?
아래 글 중 Item 72. 표준 예외를 사용하라를 읽어보세요!
https://badcandy.github.io/2019/01/01/effective-java-3rd-chapter10/
| try { | ||
| return Integer.parseInt(inputValue); | ||
| } catch (NumberFormatException e) { | ||
| throw new NumberFormatException("숫자가 아닙니다. 재입력 해주세요."); |
| private List<LottoBall> winningBalls; | ||
|
|
||
| public WinningBallsUtils(String input) { | ||
| String[] winningBalls = input.split(","); |
There was a problem hiding this comment.
하드코딩은 피해주세요! ","
그리고 관련된 코드별로 공백을주면 가독성이 더 좋아집니다 :)
public WinningBallsUtils(String input) {
String[] winningBalls = input.split(",");
validateWinningBallsNumber(winningBalls);
validateWinningBallsLength(winningBalls);
validateDuplicatedWinningBalls(winningBalls);
this.winningBalls = Arrays.stream(winningBalls)
.map(Integer::parseInt)
.map(LottoBallFactory::findByLottoBall)
.collect(Collectors.toList());
Collections.sort(this.winningBalls);
}
| } | ||
|
|
||
| private void validateDuplicatedWinningBalls(String[] winningBalls) { | ||
| Set<String> DuplicatedValidation = new HashSet<>(Arrays.asList(winningBalls)); |
| import java.util.*; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class WinningBallsUtils { |
There was a problem hiding this comment.
이 클래스는 유틸이라기 보단 winningBalls의 도메인 로직이라고 보이네요 🤔
There was a problem hiding this comment.
도메인로직이 있는 부분을 따로 WinningTicket으로 분리하였습니다.
| } | ||
|
|
||
| public int countRankPeople(List<WinningRank> winningRanks, WinningRank winningRank) { | ||
| return (int) winningRanks.stream().filter(Rank -> winningRank == Rank).count(); |
There was a problem hiding this comment.
람다식에서 파라미터 부분은 변수이기 때문에 앞글자를 소문자로 해주세요.
filter(rank -> winningRank == rank)
There was a problem hiding this comment.
이전 미션부터 사소한 것에서 실수가 많이 나오네요,, 이런 실수를 줄이는게 가장중요한것 같네요ㅠㅠ 꼭 다음 번에는 꼼꼼히 확인하겠습니다!!😀
WinningRank 테스트의 예상값과 실제값의 인자 위치를 변경 WinningRank의 determineRank 메소드의 filter 내부를 명시적으로 표현하기위해 메소드로 분리 LottoTicket과 WinningBalls 클래스의 의미를 명확하게 하기 위해 WinningTicket 이라고 클래스명을 변경 DuplicationException을 표준예외로 변경 매직넘버 상수화 리팩토링후 테스트 확인 String Format으로 사용 WinningBallUtils 가 인스턴스 변수를 갖고 비지니스 로직을 갖는 부분이 있어서 WinningTicketFactory 클래스를 만들어 검증유틸 부분과 비지니스 로직을 나눴고 테스트를 추가 에러메시지 상수화 전체적인 리팩토링
custom exception을 모두 표준예외로 변경 LottoBallFactory에서 인스턴스 참조값을 주었던 메소드가 외부에서 변경가능성이 있어서 필요한 부분만 unmidifiableList를 통해 리턴해주도록 변경 LottoTicket과 WinningBallFactory의 본질적 기능이 같아 합침
No description provided.