[2단계 - 로또(수동)] 더즈(이동규) 미션 제출합니다. #431
Conversation
| assertThat(inputLottoNumbers).isNotSameAs(copiedLottoNumbers); | ||
| assertThatThrownBy(() -> copiedLottoNumbers.add(LottoNumber.getInstance(45))) | ||
| .isInstanceOf(UnsupportedOperationException.class); | ||
| } |
There was a problem hiding this comment.
참조가 잘 끊어졌는지 리스트가 불변인지 확인하기 위해 작성했었습니다. 근데 List.copyOf의 기능 자체를 테스트하는 것처럼 보이네요 삭제하는게 나을까요?
There was a problem hiding this comment.
아 DisplayName이 방어적 복사 테스트로 되어있어서 List.copyOf불변으로 동작하는지 보기위한 테스트인가 싶어서 여쭤봤었어요. 굳이 삭제하실 필요는 없을거 같아요 ㅎㅎ
| InputView.getManualNumbers(manualCount)); | ||
|
|
||
| lottoTickets.addAll( | ||
| lottoMachine.purchaseLottoTicketsByAuto(money.consume(manualCount * LOTTO_TICKET_PRICE))); |
There was a problem hiding this comment.
현재 Money랑 LottoCount랑 역할이 겹쳐보이는데요. Money도 현재 살수 있는 로또의 갯수를 반환하도록 역할을 가지고 있고 LottoCount도 이 역할을 할 수 있도록 값타입으로 정의하신걸로 보여서요.
하나로 정리하면 어떨까요?
There was a problem hiding this comment.
넵 생각해보니 두 객체가 비슷한 일을 하고 있었네요! 수동 개수를 포장하려다 보니 역할이 겹친 것 같습니다.. 수동 개수를 포장하기가 애매해서 고민 했었는데 컨틀롤러에서 '개수'를 다루지 않고 '수동으로 살 금액'과 '자동으로 살 금액'을 사용하여 Money가 개수와 금액이 모자랄 때에 대한 검증을 전담하도록 했습니다!
Money totalMoney = Money.from(InputView.getMoney());
Money moneyOfManual = Money.from(InputView.getManualCount() * LOTTO_TICKET_PRICE);
Money moneyOfAuto = totalMoney.consume(moneyOfManual);
이렇게 하면 개수가 마이너스일 때도, 총 금액이 수동 개수만큼 사지 못할 때도 Money가 예외를 던질 수 있습니다. LottoCount는 삭제했습니다.
| return lottoTicketNumbers.stream() | ||
| .map(numbers -> numbers.stream() | ||
| .map(LottoNumber::getInstance) | ||
| .collect(Collectors.toList())) |
There was a problem hiding this comment.
numbers.stream()
.map(LottoNumber::getInstance)
.collect(Collectors.toList()
위 부분도 가독성을 위해서 private 메서드로 따로 정의해주면 좋을거 같아요 :)
src/test/java/LottoMachineTest.java
Outdated
| for (LottoTicket lottoTicket : lottoTickets) { | ||
| List<Integer> ticketNumbers = lottoTicket.getTicketNumbers(); | ||
| assertThat(ticketNumbers).isEqualTo(iterator.next()); | ||
| } |
There was a problem hiding this comment.
assertJ를 쓴다면 Method 체이닝으로 좀더 가독성있게 테스트를 작성할수 있어요~!
assertThat(lottoTickets)
.map(LottoTicket::getTicketNumbers)
.contains(lottoTicket1, lottoTicket2);
iterable / array 부분은 https://assertj.github.io/doc/#assertj-core-group-assertions 를 참고하면 좋을거 같아요~
There was a problem hiding this comment.
assertJ에도 스트림처럼 map 등을 이용해서 메서드 체이닝을 할 수 있군요...! 컬렉션 데이터에 대한 테스트를 앞으로도 많이 할 것 같은데 한 번 공부해보겠습니다 감사합니다!
src/test/java/LottoMachineTest.java
Outdated
| import domain.*; | ||
| import org.junit.jupiter.api.DisplayName; | ||
| import org.junit.jupiter.api.Test; | ||
| import static org.assertj.core.api.Assertions.*; |
There was a problem hiding this comment.
현재 테스트 클래스들이 모두 test/java 하위에 존재하네요. 혹시 의도하신걸까요?
현재는 클래스가 많지는 않지않아서 두드러지지 않을 수 있지만 프로덕션 코드와 동일한 패키지 구조로 테스트 구조를 맞춰놓으면 가독성과 유지보수에도 좀 더 도움이 될거에요~
| @@ -6,6 +6,8 @@ | |||
|
|
|||
| private static final int MINIMUM_AMOUNT = 10; | |||
| private static final String MONEY_MUST_BE_DIVIDABLE_BY_TEN = "금액은 10 단위로 나누어 떨어져야 합니다."; | |||
| public static final String MONEY_MUST_BE_OVER_ZERO = "금액은 마이너스가 될 수 없습니다."; | |||
| public static final String NOT_SUFFICENT_MONEY = "해당 금액으로 구입할 수 없습니다"; | |||
There was a problem hiding this comment.
1단계때도 피드백 드린 내용이긴 한데요. LottoConstant에도 상수가 모여있고 Money에도 상수가 정의되어 있네요.
상수를 관리할 Constant 클래스를 정의하는 것과 상수에 의미에 맞는 적절한 클래스에서 관리하는 것 둘 다 괜찮은 선택지라고는 생각하는데요. 통일성 있게 가져가는 것도 중요하다고 생각해요.
두 선택지 중에 더 나은 방법이 어떤건지 한번 고민해보시고 한가지 방법을 선택해서 가져간다면 추후 유지보수에도 도움이 될거라 생각합니다.
안녕하세요 철시! 2단계로 다시 찾아 뵙게 되었습니다ㅎㅎ
2단계 피드백도 잘 부탁드려요!!
수동 구매 추가
로또를 나타내는
LottoTicket이 애초에List<LottoNumber>로 생성되기 때문에LottoTicket은 변화가 없었습니다. 수동 기능을 추가할 땐LottoMachine에서 수동 구매 메서드를 추가하고 컨트롤러에서 자동 구매 리스트와 수동 구매 리스트를 합치면서 수동 구매를 추가하였습니다.로또 개수 값타입 생성
수동 구매 개수에 대해서도 역할이 필요할 것 같아
LottoCount값 타입을 만들어 주었습니다. 입력 금액과 관련하여 살 수 있는 로또 개수에 관한 검증이 이루어집니다.view에는 dto를 넘기게 하기
LottoTicketDto를 만들고view에는dto와enum객체 정도만 사용되게 만들었습니다.