[로또 main2] 조윤호 제출합니다 #4
Conversation
| p.isPrized() && | ||
| p.getMatchNum() <= matches && | ||
| p.getGrade() <= prize.getGrade() | ||
| ) { |
There was a problem hiding this comment.
41~43 라인을 하나의 메소드로 뽑으면 가독성도 좋아지고, 메소드 이름으로 어떤 조건인지 표현 가능할 것 같습니다!
|
|
||
| @Override | ||
| public Lotto getLotto(int min, int max, int num){ | ||
| List<Integer> numbers = Randoms.pickUniqueNumbersInRange(min,max,num); |
There was a problem hiding this comment.
stream() 사용하시면 22라인까지 1줄로 표현할 수 있을 것 같아요
| private UserOutput() { | ||
| } | ||
|
|
||
| public static void printLottos(LottoOutputModel model) { |
There was a problem hiding this comment.
System.out.printf 사용하시면 1519 라인을 1줄로 줄일 수 있을 것 같고, 2022 라인도 stream().map 사용하시면 더욱 가독성 있게 작성할 수 있을 것 같습니다
| private void validateBonusNumber(List<Integer> numbers, int num){ | ||
| if (numbers.contains(num)) | ||
| throw new IllegalArgumentException("[ERROR] 보너스 번호와 당첨 번호에 중복이 있습니다."); | ||
| } |
| @@ -0,0 +1,8 @@ | |||
| package lotto.lotto.lottoCreator; | |||
|
|
|||
| import camp.nextstep.edu.missionutils.Randoms; | |||
| import lotto.lotto.Lotto; | ||
|
|
||
| public interface LottoCreator { | ||
| public Lotto getLotto(int min, int max, int num); |
There was a problem hiding this comment.
인터페이스는 컴파일 시 자동으로 접근제한자를 public으로 인식하기 때문에 접근제한자는 생략하셔도 무방합니다!
| private void validateNumRange(int min, int max) { | ||
| for (int n : numbers) | ||
| if (n < min || n > max) | ||
| throw new IllegalArgumentException("[ERROR] 올바른 범위 내의 숫자를 입력해주세요."); | ||
| } |
There was a problem hiding this comment.
로또 숫자의 의미를 가장 잘 표현할 수 있는 타입이 정수형일까요?
로또 숫자와 같이 단순 정수가 아닌 의미있는 값은 객체로 관리하는 것이 좋다고 생각합니다!
| import java.util.List; | ||
|
|
||
| public class RandomLottoCreator implements LottoCreator{ | ||
| private static final LottoCreator instance = new RandomLottoCreator(); |
There was a problem hiding this comment.
상수 네이밍은 대문자와 언더바(_)로 구성해주세요!
ex)
private static final LottoCreator INSTANCE = new RandomLottoCreator();
private static final LottoCreator LOTTO_CREATOR = new RandomLottoCreator();| int commas = input.length() - input.replace(String.valueOf(","), "").length(); | ||
| if (commas + 1 != strList.size()) | ||
| throw new IllegalArgumentException("[ERROR] 정수와 ,로 이루어진 배열을 입력해주세요."); |
There was a problem hiding this comment.
구분자에 대한 검증같은데, 예외 메시지가 어색한 것 같습니다.
그리고 구분자로 사용되는 쉼표(,)는 상수화하면 구분자가 바뀌었을 때 예외 메시지와 replace()에서 사용되는 구분자를 실수없이 수정할 수 있을 것 같아요!
| UserOutput.printLottos( | ||
| new LottoOutputModel( | ||
| lottoNum - manualNum, manualNum, bundle.getLottosNumList() | ||
| )); |
There was a problem hiding this comment.
출력을 위한 값들을 담고 있는 객체가 정말 필요할까요?
로또 게임을 여러번 진행한다고 하면, LottoOutputModel이 계속 생성될텐데 객체 생성시 초기화된 값들을 그대로 꺼내서 사용하는 행위는 불필요해보입니다!
| List<LottoCreator> creators = Stream.generate(RandomLottoCreator::from) | ||
| .limit(lottoNum - manualNum) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
RandomLottoCreator가 2개 이상 생성되어야 할까요?
한 번 생성된 RandomLottoCreator에서 getLotto()만 새롭게 호출해도 랜덤으로 생성된 로또를 발급할 수 있을 것 같아요.
| } | ||
| } | ||
|
|
||
| public List<Integer> getNumList(String input) { |
There was a problem hiding this comment.
해당 메소드에 책임이 너무 많은 것 같아요. 메소드의 책임이 최소화되도록 하면 좋을 것 같아요!
| @@ -0,0 +1,35 @@ | |||
| package lotto.purchase; | |||
|
|
|||
| public class PurchaseService { | |||
There was a problem hiding this comment.
PurchaseService는 유틸성 클래스로 사용하는 것보다 객체로 생성하는게 더 좋지 않을까요?
| @@ -0,0 +1,35 @@ | |||
| package lotto.purchase; | |||
|
|
|||
| public class PurchaseService { | |||
| this.bonus = builder.bonus; | ||
| } | ||
|
|
||
| public static class Builder { |
main1 리뷰해주신 내용들 반영 + main2 요구사항 적용했습니다!
Bonus 클래스 추가하면서 좀 허점이 보이지만...!! 오늘은 여기까지인 것 같아요😭 일단 제출해봅니당
이펙티브 자바 읽기 시작하면서, 정적 팩토리 메소드, 빌더 등등을 적용시켜 보았는데, 아낌없이 조언 남겨주시면 감사하겠습니다!