[로또 미션] 황승준 미션 제출입니다. #61
Conversation
There was a problem hiding this comment.
로또 미션 1등 제출!! 고생 많으셨어요~
테스트도 꼼꼼하고, 많은 고민과 함께 학습하신 모습이 돋보이는 코드였습니다.
몇가지 코멘트 남겼으니 확인해주세요!
중간중간에 사소한 코멘트는 👀 이 이모지를 남겨두었습니다!
너무 세세하게 객체를 만들다 보니 test code에서 given에 객체를 만들때 시간을 많이 투자가 되는거 같네요. LottoNumbers 객체를 없애버리고 이에 대한 것을 Lotto 객체에 두었다면 훨씬 편했을거 같습니다.
PR에 남겨주신대로 Lotto와 LottoNumbers가 불필요하게 분리되어 있는 것 같다고 느껴졌어요.
적당한 책임 분리 역시 중요한 요소 중에 하나입니다!
그리고 테스트를 위한 데이터 준비(Given절)가 많아 보이긴 했어요. 특히 수익률 계산의 경우 꼼꼼하게 하셨더라고요..? 하지만, 어떤걸 검증하는지에 더 집중해보면 좋을 것 같아요. 모든 케이스를 다루는 것 보단 해당 로직에서 발생하는 경우에 맞게 검증하면 간단하기도 하고, 나중에 확인했을 때 어떤 로직인지 파악하기 쉬워요!
현재 수익률 기대값이 253944.37인 테스트가 구성 되어 있는데, 이거 사람이 코드 보고 예측 가능 할까요..? 어차피 다시 따져가면서 계산기를 두드려야 한다면 개선이 필요한 테스트이지 않을까 싶습니다..!
for loop 대신 stream을 더 많이 사용하고 싶은데 stream의 forEach, map등의 함수에서는 모든 변수들이 final로 정의되어 for loop을 또 많이 사용하게 된거 같습니다. 이것은 조금 더 찾아보면서 리팩토링 해보겠습니다!
stream 역시 자주 활용하려는 시도가 보였습니다! 잘 사용하신 것 같아요. stream으로 개선하는 리뷰도 종종 남겨두었습니다.
하지만, stream이 항상 좋은 건 아니라고 생각해요. stream의 가장 큰 장점은 간결하고 선언적이라 가독성이 좋다는 점인데요.
#61 (comment)
위 코멘트에서 stream으로 변경하신걸 보시면, 더 읽기 좋아졌나요?
무조건 stream이 좋은 것이라 생각하기 보단, 목적에 맞게 적절하게 사용하면 좋겠습니다!
특히, forEach()는 "stream스럽지 않다" 라는 의견도 있고 side-effect (링크에서 Side-effect 찾기 하시면 나와요!)도 보고 된 바가 있으니 잘 확인하시고 활용하세요~!
|
|
||
|
|
There was a problem hiding this comment.
2줄, 4줄씩 띄운 공백의 의미가 있나요?
없다면 일관되게 1개만 사용해주세요
| this.outputView = outputView; | ||
| } | ||
|
|
||
| public void run() { |
There was a problem hiding this comment.
run() 메서드가 너무 길어요.
이루어지는 행위들을 purchaseLottos() 메서드처럼 구분하면 읽기 더 좋아질 것 같네요.
| List<Lotto> lottos = new ArrayList<>(); | ||
| for (int i = 0; i < manualCount; i++) { | ||
| LottoNumbers lottoNumbers = inputView.readManualLottoNumbers(i); | ||
| lottos.add(new Lotto(lottoNumbers)); | ||
| } |
There was a problem hiding this comment.
manualCount 만큼 실행 하는 건 괜찮은데, readManualLottoNumbers(i); 경우에 flag(i)가 들어가는게 어색하네요.
i 라는 인덱스는 InputView의 코드 없이는 현재 객체 내에서 설명이 안되는 값이에요.
flag를 제거할 수 없을까요?
제거한다면, 아래처럼 Stream을 활용할 수도 있을 것 같아요.
| List<Lotto> lottos = new ArrayList<>(); | |
| for (int i = 0; i < manualCount; i++) { | |
| LottoNumbers lottoNumbers = inputView.readManualLottoNumbers(i); | |
| lottos.add(new Lotto(lottoNumbers)); | |
| } | |
| inputView.printReadManualLottoMessage(); | |
| List<Lotto> manualLottos = IntStream.range(0, manualCount) | |
| .mapToObj(i -> inputView.readManualLottoNumbers()) | |
| .map(Lotto::new) | |
| .toList(); |
|
|
||
| public class Lotto { | ||
| public static final int PRICE = 1000; | ||
| private static final LottoNumberGenerator lottoNumberGenerator = new LottoNumberGenerator(); |
src/main/java/domain/Lotto.java
Outdated
|
|
||
|
|
||
|
|
||
| public Lotto(final LottoNumbers lottoNumbers) { |
There was a problem hiding this comment.
파라미터에 붙은 final은 여기에만 있네요.
일관성을 위해 제거하는게 좋아보여요
| .map(LottoNumber::from) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| Collections.sort(lottoNumbers); |
There was a problem hiding this comment.
로또가 정렬 되어 있어야 한다면, input에 의존적으로 구성 되어 있어요.
LottoNumbers에서 정렬이 이루어져야 할 것 같네요!
| System.out.print("["); | ||
| for (int j = 0; j < LottoNumbers.SIZE; j++) { | ||
| System.out.print(flatLottosNumber.get(j).getValue()); | ||
|
|
||
| if (j != LottoNumbers.SIZE - 1) { | ||
| System.out.print(", "); | ||
| } | ||
| } | ||
| System.out.print("]\n"); | ||
| } |
There was a problem hiding this comment.
잘 만들어진 기능이 있었어요!
| System.out.print("["); | |
| for (int j = 0; j < LottoNumbers.SIZE; j++) { | |
| System.out.print(flatLottosNumber.get(j).getValue()); | |
| if (j != LottoNumbers.SIZE - 1) { | |
| System.out.print(", "); | |
| } | |
| } | |
| System.out.print("]\n"); | |
| } | |
| System.out.println(Arrays.toString(flatLottosNumber.toArray())); |
| Assertions.assertDoesNotThrow(() -> { | ||
| LottoNumber lottoNumber = new LottoNumber(value); | ||
| }); |
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("methodSourceTestArguments") |
|
|
||
|
|
||
| } | ||
| private static Stream<Arguments> methodSourceTestArguments() { |
|
감사합니다~ 천천히 읽으면서 피드백 반영하겠습니다! |
너무 세세하게 객체를 만들다 보니 test code에서 given에 객체를 만들때 시간을 많이 투자가 되는거 같네요.
LottoNumbers 객체를 없애버리고 이에 대한 것을 Lotto 객체에 두었다면 훨씬 편했을거 같습니다.
for loop 대신 stream을 더 많이 사용하고 싶은데 stream의 forEach, map등의 함수에서는 모든 변수들이 final로 정의되어 for loop을 또 많이 사용하게 된거 같습니다. 이것은 조금 더 찾아보면서 리팩토링 해보겠습니다!