Step2 리뷰 요청드립니다! #3776
Step2 리뷰 요청드립니다! #3776
Conversation
| List<LottoTicket> lottoTickets; | ||
| LottoMatch lottoMatch; |
| @Override | ||
| public String toString() { | ||
| return matchNumber + "개 일치 (" + prizeMoney + "원)"; | ||
| } |
There was a problem hiding this comment.
enum도 비즈니스 로직의 일부이므로 view 부분의 구현을 enum에 두는건 좋지 않을 것 같아요.
|
|
||
| private final int matchNumber; | ||
| protected final int prizeMoney; | ||
| abstract int totalPrizeMoney(int count); |
There was a problem hiding this comment.
등수별로 모두 동일한 구현체를 가진 것 같은데요. 추상 메서드로 정의될 필요가 있는지 고민이 필요해보여요.
그리고 현재 로또 도메인에서 enum이 로또의 등수별 개수 및 당첨 금액을 나타내는 역할을 하고 있습니다. 고정된 상수들만 가지는게 아니라 받은 개수만큼 곱해서 계산해주는 역할을 가지는게 적절할지도 고민해보시면 좋겠네요.
| for (int i = 0; i < ticketNum; i++) { | ||
| this.lottoTickets.add(new LottoTicket()); | ||
| } | ||
|
|
||
| return this.lottoTickets; |
There was a problem hiding this comment.
중요한 부분은 아니지만 Stream.generate를 통해 불변 형태로 표현할 수 있을 것 같네요. 가변보다는 불변 형태로 표현하는게 사이드 이펙트도 적고 가독성 측면에서도 좋을 것 같아요.
| return this.lottoTickets; | ||
| } | ||
|
|
||
| public HashMap<LottoMatchNumber, Integer> winLotto() { |
There was a problem hiding this comment.
일반적으로 메서드 리턴 타입이나 파라미터 등은 다형성 측면에서 이점이 없기 때문에 구체 타입(HashMap)을 사용하지 않습니다. 이유가 있는게 아니라면 인터페이스인 Map을 리턴 타입으로 쓰시는게 어떨까요?
| public void inputTargetNumbers(String input) { | ||
| lottoMatch = new LottoMatch(input); | ||
| } |
There was a problem hiding this comment.
이런 형태로 사용할거면 인스턴스 변수 없이 바로 리턴하는 형태로 사용하면 어떨까요?
| public HashMap<LottoMatchNumber, Integer> matchResult(List<LottoTicket> myNumbers) { | ||
| HashMap<LottoMatchNumber, Integer> results = new HashMap<>(); | ||
| results.put(LottoMatchNumber.MATCH3, 0); | ||
| results.put(LottoMatchNumber.MATCH4, 0); | ||
| results.put(LottoMatchNumber.MATCH5, 0); | ||
| results.put(LottoMatchNumber.MATCH6, 0); | ||
|
|
||
| for (LottoTicket myNumber : myNumbers) { | ||
| int matchCount = myNumber.checkMatch(targetNumbers); | ||
| inputMatchResult(matchCount, results); | ||
| } | ||
|
|
||
| return results; | ||
| } |
There was a problem hiding this comment.
Stream의 groupingBy나 Function.identity를 활용해보아도 좋을 것 같네요.
| public LottoMatch(String input) { | ||
| targetNumbers = new ArrayList<>(); | ||
| String[] inputs = input.split(", "); | ||
|
|
||
| for (int i = 0; i < inputs.length; i++) { | ||
| targetNumbers.add(Integer.valueOf(inputs[i])); | ||
| } |
There was a problem hiding this comment.
생성자는 이미 생성이란 책임을 가지고 있는데 생성자 내부에서 split을 통한 파싱 및 여러 책임이 포함되어 있네요.
생성자에 로직을 넣고 싶다면 정적 팩터리로 분리하고 정적 팩터리에 로직을 추가한 뒤 거기서 생성자를 호출하거나 하는 형태로 구현하는건 어떨까요?
| public LottoTicket() { | ||
| this.lottoNumbers = new ArrayList<>(); | ||
| for (int i = 1; i <= 45; i++) { | ||
| this.lottoNumbers.add(i); | ||
| } | ||
| lottoNumbers = issueTicket(); | ||
| } | ||
|
|
||
| public LottoTicket(List<Integer> ticketNumbers) { | ||
| this.lottoNumbers = new ArrayList<>(ticketNumbers); | ||
| } |
There was a problem hiding this comment.
제약 조건 검증이 전혀 포함되어 있지 않네요. 도메인의 특성을 잘 나타내지 못하는 것 같습니다.
- 1 - 45까지의 숫자만 존재해야 함
- 6개의 중복되지 않은 숫자로 이루어져야 함
위와 같이 로또 도메인의 특성이 클래스에 반영되도록 설계해보시면 좋겠습니다.
No description provided.