[1단계 - 로또 구현] 다니(이다은) 미션 제출합니다. #248
Conversation
- 메세지를 인자로 받아 생성되도록 리팩토링하였습니다.
- LottoTicket에 대한 일급 컬렉션
- 맞춘 개수에 따른 상금에 대한 정보를 가지는 enum 클래스 Rank를 구현했습니다.
- Shallow Copy로 인해 발생하는 버그로, Deep Copy를 통해 버그를 해결하였습니다.
📚 다니의 학습 로그[TDD] 테스트 주도 개발 - 5내용
링크[Java] 일급 컬렉션 - 4내용
링크
[Java] 원시값 및 문자열 포장 - 5내용
링크
[Java] Enum - 4내용
링크[Java] 스트림 - 5내용
링크[Java] 제네릭 - 2내용
링크[Java] 초기화 블럭 - 3내용
링크 |
There was a problem hiding this comment.
안녕하세요 다니, 리뷰어 코니입니다 👋
전체적으로 깔끔하게 잘 구현해 주셨고, 책임이 적절하게 나뉘어서 이해하기 좋은 코드였어요. 원시값 포장, 일급 컬렉션 사용, 스트림 활용 등도 훌륭합니다 💯
질문에 답을 드리자면,
1번. 저는 지금은 커밋 메시지에 prefix를 작성하지 않고 있어요. 크게 중요한 건 아니라고 생각해서, 다니의 취향대로 하시면 될 것 같습니다.
3번. 요구사항에서의 원시값 포장과 Integer 타입이 Wrapper 타입이란 건 맥락이 조금 다른 것 같아요 ㅎㅎ 우선 여기선 굳이 필요하지 않아 보입니다.
2번 질문에 대한 답변은 해당 위치에 남겼어요~ 추가적인 질문이나 의견은 언제든지 코멘트 및 DM으로 알려주세요!
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| public class LottoNumberTest { | ||
| @DisplayName("같은 숫자인 경우 정상 처리된다") |
There was a problem hiding this comment.
같은 숫자인 경우 무엇이 정상 처리된다는 것일까요? 이름만 보고도 테스트가 하는 역할을 명확히 알 수 있다면 특히나 테스트가 실패했을 때 빠르게 상황을 파악할 수 있답니다!
| assertThat(first).isNotEqualTo(second); | ||
| } | ||
|
|
||
| @DisplayName("45를 초과한 수가 입력될 경 예외를 발생시킨다.") |
There was a problem hiding this comment.
| @DisplayName("45를 초과한 수가 입력될 경 예외를 발생시킨다.") | |
| @DisplayName("45를 초과한 수가 입력될 경우 예외를 발생시킨다.") |
😎
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class LottoPurchase { |
There was a problem hiding this comment.
LottoPurchase라는 이름만 보고는 어떤 역할을 하는 객체인지 단번에 이해하긴 어렵네요. 돈을 주면 로또를 돌려주니 LottoMachine 같은 이름은 어떨까요? 더 좋은 이름이 있을 수도 있구요.
There was a problem hiding this comment.
9bd543f 수정했습니다!
제가 기존에 사용한 LottoPurchase보다 코니가 제시해준 LottoMachine이 훨씬 좋은 이름 같아요! 추천 감사합니다 😆
| public static LottoTicket of(List<LottoNumber> shuffleLottoNumbers) { | ||
| return new LottoTicket(shuffleLottoNumbers); | ||
| } |
There was a problem hiding this comment.
List<LottoNumber>를 매개변수로 받는 생성자도 있고 정적 팩터리 메서드도 있군요. 밖에서List<LottoNumber>로LottoTicket을 생성하고자 할 때 어떤 것을 사용할지는 어떻게 판단해야 할까요? 정적 팩터리 메서드가 굳이 필요할까요?- 매개변수로 들어오는
LottoNumber들이 섞였는지 안 섞였는지는LottoTicket의 관심사가 아닐 것 같아요. 따라서 매개변수 이름에shuffle이 들어갈 필요는 없어 보여요.
There was a problem hiding this comment.
There was a problem hiding this comment.
https://johngrib.github.io/wiki/static-factory-method-pattern/
⟪이펙티브 자바⟫에서는 정적 팩터리 메서드의 장점을 다음과 같이 소개하고 있어요.
- 이름이 있으므로 생성자에 비해 가독성이 좋다.
- 호출할 때마다 새로운 객체를 생성할 필요가 없다.
- 하위 자료형 객체를 반환할 수 있다.
- 형인자 자료형(parameterized type) 객체를 만들 때 편하다.
저도 위와 같은 장점을 누릴 수 있는 경우에 정적 팩터리 메서드를 사용하는 것은 좋다고 생각해요! 그런데 대부분의 경우에는 of(), valueOf() 처럼 별로 의미 없는 이름 + 단순한 생성자 리턴의 조합으로 사용하고 계신 것 같더라고요 ㅎㅎ 저는 웬만하면 생성자를 사용하는 것이 더 명확하다고 생각해서 선호하지만, 이것 역시 취향 차이입니다. 다니도 조금 더 공부해 보시고 언제 정적 팩터리 메서드를 사용했을 때 코드가 더 좋아지는지 느끼면서 본인만의 생각을 쌓아 나가시면 돼요.
| private static boolean isNoneDuplicationNumberOrThrow(List<LottoNumber> lottoNumbers, LottoNumber lottoNumber) { | ||
| if (lottoNumbers.contains(lottoNumber)) { | ||
| throw new IllegalArgumentException(ERROR_INVALID_DUPLICATION_NUMBER); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| private static void isValidNumberCountOrThrow(List<String> lottoNumbers) { | ||
| if (lottoNumbers.size() != TOTAL_LOTTO_NUMBER_COUNT) { | ||
| throw new IllegalArgumentException(ERROR_INVALID_NUMBER_COUNT); | ||
| } | ||
| } |
There was a problem hiding this comment.
is나 has 등으로 시작하는 메서드는 보통 메서드의 내용이 단순히 조건식인 경우가 많아요. 특히 위의 정적 팩터리 메서드에서 필터링에 isNoneDuplicationNumberOrThrow()를 사용하고 있는데, 필터링 도중에 예외가 발생할 수도 있겠군요 😱 괜찮은 설계일까요?
또한, 중복 확인 로직이 다소 복잡한데요, 우리가 이미 알고 있는 Set을 활용해 보면 어떨까요?
There was a problem hiding this comment.
6382155 수정했습니다!
이 내용도 리팩토링 하면서 궁금한 점이 생겼는데요.
Q. 필터링 도중에 예외를 발생시키는 건 나쁜 설계인가요?? 미리 검증을 하면 생성되지 않을 객체가 생성돼서 그런 건가요??
There was a problem hiding this comment.
음 저는 필터링을 시켰을 때 예외가 발생한다면 예상치 못한 동작이라고 생각할 거 같아요... 이것도 역시 취향 차이일 수 있으려나요 🤔
| return Arrays.stream(Rank.values()) | ||
| .filter(item -> item.getCount() == count) | ||
| .findAny() | ||
| .orElse(NOTHING); |
src/test/java/domain/MoneyTest.java
Outdated
| Money money1 = new Money("1000"); | ||
| Money money2 = new Money("1000"); | ||
| assertThat(money1).isEqualTo(money2); |
There was a problem hiding this comment.
| Money money1 = new Money("1000"); | |
| Money money2 = new Money("1000"); | |
| assertThat(money1).isEqualTo(money2); | |
| Money money1 = new Money("1000"); | |
| Money money2 = new Money("1000"); | |
| assertThat(money1).isEqualTo(money2); |
이렇게 한 줄씩 띄워 주면 가독성이 훨씬 좋아져요.
| private final InputView inputView = InputView.getInstance(); | ||
| private final OutputView outputView = OutputView.getInstance(); | ||
|
|
||
| public void run() { |
There was a problem hiding this comment.
이 메서드 내에서도 조금씩 줄바꿈을 해 주시면 조금 더 숨을 쉬며 읽을 수 있을 것 같아요 😳
| public LottoNumber(String input) { | ||
| validateInputFormat(input); |
There was a problem hiding this comment.
LottoNumber라는 도메인 객체가 input을 받고 검증해 객체를 생성하고 있군요. 클래스 이름에도 숫자가 들어가는 만큼, 숫자만 받을 수 있게 해줘도 좋을 것 같아요. 물론 이건 저의 개인적인 취향일 수도 있어요.
There was a problem hiding this comment.
15a7254 수정했습니다!
코니 말대로 클래스명에도 숫자가 들어가니까 문자열보다 숫자를 받는 게 통일성도 있고 맞는 거 같아요 !
|
|
||
| public static List<LottoNumber> shuffleLottoNumbers() { | ||
| Collections.shuffle(lottoNumbers); | ||
| List<LottoNumber> splitLottoNumbers = new ArrayList<>(lottoNumbers.subList(MINIMUM_RANGE, MAXIMUM_RANGE)); |
There was a problem hiding this comment.
https://docs.oracle.com/javase/7/docs/api/java/util/ArrayList.html#subList(int,%20int)
Returns a view of the portion of this list between the specified fromIndex, inclusive, and toIndex, exclusive. The returned list is backed by this list, so non-structural changes in the returned list are reflected in this list, and vice-versa.
public List<E> subList(int fromIndex, int toIndex) {
subListRangeCheck(fromIndex, toIndex, size);
return new SubList(this, 0, fromIndex, toIndex);
}ArrayList의 subList() 코드를 보면 위와 같이 SubList라는 타입을 리턴하고 있는데요,
private class SubList extends AbstractList<E> implements RandomAccess {
private final AbstractList<E> parent;
private final int parentOffset;
private final int offset;
int size;
SubList(AbstractList<E> parent, int offset, int fromIndex, int toIndex) {
this.parent = parent;
this.parentOffset = fromIndex;
this.offset = offset + fromIndex;
this.size = toIndex - fromIndex;
this.modCount = ArrayList.this.modCount;
}
...
}놀랍게도 이 SubList는 내부적으로 부모 리스트를 참조하고 있네요. 따라서 new ArrayList<>()로 새롭게 만들어 주지 않으면, 계속 부모를 참조하면서 부모의 변경을 따라가게 돼요. LottoNumberRepository에서의 사용 맥락에서 보면 List<LottoNumber> lottoNumbers가 부모 리스트인데 이 리스트가 Collections.shuffle()에 의해 계속 섞이고, 그 변화를 모든 subList가 공유하게 되죠. 결국 계속 섞이다가 마지막에 섞인 결과가 lottoNumbers의 최종 상태가 될 텐데, 앞에서 생성된 모든 subList가 이 최종 상태의 lottoNumbers에서 0~6번 인덱스의 원소를 추출하게 됩니다. 이해가 되셨을까요? 😇
There was a problem hiding this comment.
아하 부모 리스트 참조 때문이었군요..!!! subList()를 쓸 때는 new ArrayList<>()로 새로 생성하는 게 좋겠네요! LottoNumberRepository로 구체적인 설명을 해주셔서 바로 이해가 됐습니다! 궁금증 해결해주셔서 감사합니다 🥰
- 객체의 역할을 더 잘 나타내는 이름으로 변경했습니다.
- validateBonusBall 메소드의 매개변수에 winningNumbers도 추가했습니다.
- isValidNumberCountOrThrow에서 validateNumberCount로 메소드명을 변경했습니다. - isNoneDuplicationNumberOrThrow에서 validateDuplicationNumber로 메소드명을 변경했습니다. - 로또 번호 중복 확인 로직에 filter 대신 Set을 사용했습니다.
- LottoNumber의 경우 String에서 int로 변환했습니다. - Money의 경우 String에서 long으로 변환했습니다.
- LottoTickets를 List<LottoTicket>으로 변경했습니다.
|
안녕하세요, 코니 🙌 피드백 바탕으로 리팩토링 했는데, 피드백 보면서 궁금했던 내용은 해당 코멘트에 👀와 함께 궁금한 점을 적어두었습니다! 이번에도 코드 리뷰 잘 부탁드립니다 ✨ |
| } | ||
|
|
||
| private void validateNumberRange(int number) { | ||
| if ((number < MIN_NUMBER_RANGE) || (number > MAX_NUMBER_RANGE)) { |
There was a problem hiding this comment.
| if ((number < MIN_NUMBER_RANGE) || (number > MAX_NUMBER_RANGE)) { | |
| if (number < MIN_NUMBER_RANGE || number > MAX_NUMBER_RANGE) { |
괄호는 없어도 될 것 같아요.
| outputView.printLottoTicket(lottoTickets); | ||
| outputView.newLine(); |
There was a problem hiding this comment.
이 정도는 OutputView에서 알아서 한다면 컨트롤러가 일일이 지시할 필요가 없지 않을까요?
안녕하세요, 코니! 다니입니다 🙌
이번에 처음 뵙는데, 코드 리뷰 잘 부탁드립니다! 😊
저는 로또 미션을 통해 모든 원시값과 문자열을 포장, enum 활용, 일급 콜렉션 사용을 경험했습니다.
특히, 모든 원시값과 문자열 포장은 처음 해봤는데 페어와 함께한 덕분에 금방 적용할 수 있었습니다 !
또한, 로또를 구현하며 TDD를 다시 이해할 수 있었는데요.
무조건 테스트 코드를 먼저 작성하고, 다음에 프로덕션 코드를 작성하는 연습을 했습니다.
제가 미션을 진행하며 궁금한 점을 몇 가지 정리했는데요, 질문 드려도 괜찮을까요??
궁금한 점 1.
메소드명을 변경할 때 커밋 메시지 타입을
style로 적었는데요, refactor와 style 사이에서 고민을 했습니다.혹시 코니는 보통 어떤 타입을 사용하나요? 둘 중 어떤 게 더 나은 건가요??
궁금한 점 2.
로또 번호 자동 생성 기능은
Collections.shuffle()을 이용해서 무작위로 섞고subList()를 사용해서 상위 6개 인덱스만 선택해서 발급되게 구현했는데요. 현재 코드에는new ArrayList<>()의 인자로subList()가 들어가서 매번 문제 없이 랜덤값이 반환되는데,new ArrayList<>()가 없이subList()만 작성하면 모든 경우에 동일한 List가 반환됩니다.혹시
subList()가 객체 생성을 매번 따로 안하는 건가 싶어서hashCode()로 hash 주소도 확인해봤는데, 매 순간마다 다른 값을 반환했습니다. 그래서subList()도 객체를 따로 생성하는 거 같은데, 왜ArrayList<>()로 복사를 하지 않으면 전부 같은 subList가 나오는 건지 이유를 알 수 있을까요??궁금한 점 3.
프로그래밍 요구사항에 따라 모든 원시값과 문자열을 포장하던 도중에 궁금증이 생겼습니다.
아래 코드처럼 Integer는 Wrapper 타입이라 따로 포장을 안했는데, 이런 경우에도 포장하는 게 필요할까요??
제 질문은 여기까지 입니다!
코드 리뷰를 하면서 코드에 부족한 점이나 잘못된 부분이 있다면 부담 없이 전부 지적해주세요!
피드백 기다리고 있겠습니다. 귀한 시간 내주셔서 감사합니다!! 🥰