Conversation
dompoo
left a comment
There was a problem hiding this comment.
호준님 바쁘신데도 고생 많으셨어요~
늦게 올려주셔서 바로 머지하겠지만, 언제든 디코에서 물어보셔도 좋습니다 :)
| public List<List<Integer>> getManualTicketNumbers(int count) { | ||
| System.out.println("수동으로 구매할 번호를 입력해 주세요."); | ||
| List<List<Integer>> manualNumbers = new ArrayList<>(); | ||
|
|
||
| for (int i = 0; i < count; i++) { | ||
| manualNumbers.add(parseNumbers(scanner.nextLine())); | ||
| } | ||
|
|
||
| return manualNumbers; | ||
| } |
There was a problem hiding this comment.
InputView에서 로또를 바로 만들어서 줘도 될 것 같은데,
Controller에서 만들게 된 이유가 따로 있을까요?
없다면 어디서 하는게 좋을지 고민해보면 좋을 것 같네요~!
| private void validateMoney(int money) { | ||
| if(money < TICKET_PRICE) { | ||
| if (money < TICKET_PRICE) { | ||
| throw new IllegalArgumentException("1000원 이상의 금액을 입력해주세요."); | ||
| } | ||
|
|
||
| if (money % TICKET_PRICE != 0) { | ||
| throw new IllegalArgumentException("1000원 단위의 금액을 입력해주세요."); | ||
| } | ||
| } | ||
|
|
||
| private void validateManualCount(int money, List<List<Integer>> manualNumbers) { | ||
| if (manualNumbers == null) { | ||
| throw new IllegalArgumentException("수동 번호는 비어 있을 수 없습니다."); | ||
| } | ||
|
|
||
| if (manualNumbers.size() > money / TICKET_PRICE) { | ||
| throw new IllegalArgumentException("수동 구매 수량이 구입 금액을 초과할 수 없습니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
요런 로직이 Service에 많아지면 점점 코드가 읽기 어려워질 것 같아요. 유지보수하기도 어렵고요.
무엇보다 LottoService는 어떤 상태를 가지지 않는 객체인 것 같은데, 그래서 이런 객체가 중요한 로직을 들고 있는 것도 고민 해볼만한 점이에요.
| private List<LottoTicket> generateManualTickets(List<List<Integer>> manualNumbers) { | ||
| List<LottoTicket> tickets = new ArrayList<>(); | ||
|
|
||
| for (List<Integer> numbers : manualNumbers) { | ||
| tickets.add(new LottoTicket( | ||
| numbers.stream() | ||
| .map(LottoNumber::new) | ||
| .toList() | ||
| )); | ||
| } | ||
|
|
||
| return tickets; | ||
| } | ||
|
|
||
| private List<LottoTicket> generateAutoTickets(int autoCount) { | ||
| List<LottoTicket> tickets = new ArrayList<>(); | ||
|
|
||
| for(int i = 0; i < ticketCount; i++){ | ||
| tickets.add(generateTicket()); | ||
| for (int i = 0; i < autoCount; i++) { | ||
| tickets.add(new LottoTicket(lottoGenerator.generateNumbers())); | ||
| } | ||
|
|
||
| return tickets; | ||
| } |
There was a problem hiding this comment.
이 로직은 LottoService가 들고 있는게 좋을까요 LottoTicket이 직접 들고 있는게 좋을까요?
| private List<LottoTicket> generateManualTickets(List<List<Integer>> manualNumbers) { | ||
| List<LottoTicket> tickets = new ArrayList<>(); | ||
|
|
||
| for (List<Integer> numbers : manualNumbers) { | ||
| tickets.add(new LottoTicket( | ||
| numbers.stream() | ||
| .map(LottoNumber::new) | ||
| .toList() | ||
| )); | ||
| } | ||
|
|
||
| return tickets; | ||
| } |
There was a problem hiding this comment.
LottoNumber를 직접 다 만들어서 LottoTicket한테 '너 이거 써'라고 말하고 있어요.
Tell Dont Ask라는 격언과 상충되는 코드인데, 요 글을 읽어보면서 고민해보면 좋을 것 같아요~
| import java.util.List; | ||
|
|
||
| public interface LottoGenerator { | ||
| List<LottoNumber> generateNumbers(); |
There was a problem hiding this comment.
컬렉션과 배열은 그 개수를 한정짓지 못한다는 문제가 있어요. 이것도 처음보는 사람이라면 6개가 나온다는 사실을 몰랐을 수도 있습니다.
인터페이스를 설계할 때에는 '그 구현체가 앞으로 절대 잘못 구현되지 않을 정도'로 정확하고 명확하게 구현해주는게 좋습니다.
| @DisplayName("로또 번호가 중복이 없는지 테스트") | ||
| @Test | ||
| void testLottoDuplicate() { | ||
| LottoGenerator generator = new RandomLottoGenerator(); | ||
|
|
||
| List<LottoNumber> numbers = generator.generateNumbers(); | ||
|
|
||
| assertThat(numbers).doesNotHaveDuplicates(); | ||
| } |
There was a problem hiding this comment.
이런것도 운에 의존하기 때문에 여러번 돌려주는게 더 안전할 것 같아요~ (그래도 100프로는 아니지만요)
| assertThat(result.get(WinningRank.THREE)).isEqualTo(1); | ||
| assertThat(result.get(WinningRank.FOUR)).isEqualTo(1); | ||
| assertThat(result.get(WinningRank.FIVE)).isEqualTo(1); | ||
| assertThat(result.get(WinningRank.SIX)).isEqualTo(1); | ||
| assertThat(result.get(WinningRank.FIVE_BONUS)).isEqualTo(0); |
There was a problem hiding this comment.
검증문이 여러개 일 때, 첫번째에서 실패하면 나머지 검증문은 실행되지도 않아요.
테스트가 10분~20분 걸리는 상황이 오면,
그런 상황에서도 나머지 검증문을 추가 실행해서 미리 모든 문제를 해결하고 싶을 수 있겠네요.
| private static final int LOTTO_MAX_NUMBER = 45; | ||
| private static final int LOTTO_MIN_NUM = 1; | ||
|
|
||
| Random random = new Random(); |
There was a problem hiding this comment.
접근제한자를 통일성 있게 붙여주고, 컨벤션을 지켜 관리하면 좋을 것 같아요.
다른 부분과 다르면 추가적인 의도가 있다고 생각되고, 계속 찾게 됩니다!
| private static List<LottoNumber> toLottoNumbers(List<Integer> numbers) { | ||
| return numbers.stream() | ||
| .map(LottoNumber::new) | ||
| .toList(); | ||
| } |
| private void validateSize(List<LottoNumber> numbers) { | ||
| if (numbers == null || numbers.size() != LOTTO_SIZE) { | ||
| throw new IllegalArgumentException("당첨 번호는 6개여야 합니다."); | ||
| } | ||
| } | ||
|
|
||
| private void validateDuplicate(List<LottoNumber> numbers, LottoNumber bonusNumber) { | ||
| if (numbers.contains(bonusNumber)) { | ||
| throw new IllegalArgumentException("보너스 번호는 당첨 번호와 중복될 수 없습니다."); | ||
| } | ||
|
|
||
| long distinctCount = numbers.stream() | ||
| .distinct() | ||
| .count(); | ||
| if (distinctCount != LOTTO_SIZE) { | ||
| throw new IllegalArgumentException("당첨 번호는 중복될 수 없습니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
LottoTicket과 비슷한 로직과 검증이 있어요.
LottoTicket을 활용해서 작성하는 방법도 좋을 것 같아요. (어렵지만요)
안녕하세요. 이번 미션을 진행하며
보너스 번호와 수동 번호 요구사항을 어디에 반영할지를 많이 고민했습니다.
보너스 번호를 단순히 int형으로 controller에서 받아서 관리한다면, 개념이 controller에 종속되는 것 같아, winningNumbers를 만들어 당첨 번호와 보너스 번호를 관리하도록 작성했습니다.
수동 번호도 처음엔 controller에서 로또 티켓으로 관리하려고 했습니다. 이건 줄이 너무 길어져서, LottoService안에 수동번호 조합, 자동번호 조합을 생성 및 관리하도록 작성했습니다.
고민하며 수정한 것들을 중간중간에 커밋을 했어야 하는데, 완성본만 커밋하니 설명하기가 어렵네요..
next-step에서 enum 사용을 요구해서 Dto대신 enum을 map으로 관리하도록 변경했습니다. map으로 관리하는게 그리 예뻐보이진 않지만, 이를 위해 클래스를 하나 더 만드는 것도 map으로 충분한데 과한 것 같아 유지했습니다.