[그리디] 김태우 로또 미션 3,4,5 단계 제출합니다. #146
Conversation
…to, showLotteryStatistics)로 분리
There was a problem hiding this comment.
안녕하세요, 태우님 :)
일정보다 리뷰가 조금 늦어졌지만, 함께 힘내서 열심히 해봐요!
질문에 대한 답변
이번 구현 과정에서 생긴 궁금점은 입력받은 문자열을 어디서 파싱해야 하는가 입니다.
저는 View에서는 입력과 출력만 담당하고, 파싱은 Service에서 처리하는 방식으로 구현했습니다.
이렇게 하면 역할이 분명히 나뉘는 장점이 있다고 생각하는데, 상희님은 보통 어떤 방식을 선호하시는지 궁금합니다.
Service에서 처리하면 역할이 명확해진다는 태우님의 의견도 충분히 일리가 있다고 생각합니다. 뷰를 최대한 단순하게 유지하려는 방식에 해당하는 것 같아요. 저는 간단한 파싱이라면 view에서 담당하게 냅뒀던 것 같아요. 만약 조금 복잡해보이거나 하면 utils 패키지를 만들어 그 안에 파싱 로직을 담았던 적도 있습니다. 태우님 덕분에 어디에 위치하는 게 좋을지 생각해보는 계기가 되었는데, 'model인 service에 위치할 때 어울리는가?' 를 생각해보면 좋을 것 같아요! 파싱 로직이 비즈니스 규칙에 해당하는지, 로또라는 도메인과 관련이 있는지에 대해 생각해보고 model이라는 위치가 적절한지에 대해 고민해보면 답이 나오지 않을까 생각합니다!
또한, 이번 미션에서는 네이밍을 이해하기 쉽게 하려고 노력했는데
메서드별 책임이 잘 드러나고 있는지, 읽기에도 무리가 없는지 피드백 부탁드립니다 🙏
아직 배우는 단계라 제가 미처 생각하지 못한 부분이 많습니다.
더 개선할 수 있는 점이 있다면 자유롭게 말씀해주시면 감사하겠습니다!
쓱 봤을 때, 전체적으로 형식 같은 건 굉장히 잘 지켜주신 것 같아요! 클래스명은 명사구, 메서드 명은 동사구 이런 것들을 지키려고 노력해주신 것 같아서 좋았어요 ㅎㅎ 다만 createInputLotto 요 메서드 같은 경우는 auto인지 manual인지 좀 더 구체적으로 적어도 좋을 것 같았고, mergeRepositories 요 메서드 같은 경우는 사실 조금 직관적이지 않은 것 같았어요. 저희가 다루는 게 로또인데 ‘저장소 병합’이라는 용어가 한 번 더 생각을 하게 만드는 이름인 것 같았습니다. 조금 더 비즈니스 용어 중심으로 개선하면 어떨까요?
전체 리뷰
-
와일드 카드 제거
아래처럼 *를 사용하여 전체를 임포트하는 것을 와일드 카드 임포트라고 하는데요,
사용하면 임포트가 더 깔끔해질 수 있다는 장점이 있지만 단점 또한 존재합니다.<단점>
서로 다른 패키지에 이름이 같은 클래스가 존재할 경우 → 오류 발생
코드를 처음 읽는 사람 → 명시적 임포트가 없다면 어떤 클래스가 사용되었는지 파악 어려움이런 이유에서 저는 와일드 카드 제거를 추천합니다! → 방법
-
디렉토리 명 변경
현재 디렉토리 명이inputView라고 되어 있는 것 같아요,
안의 클래스 명들은 input, output view를 모두 포함하고 있어서 좀 더 포괄적으로view라고 변경하는 것은 어떨까요?
✚ 추가로 PurchaseAmount가 view 디렉토리 안에 위치한 이유는 무엇인지 궁금합니다!
질문
태우님은 현재 LottoNumber, LottoNumbers, LottoTicketBundle에서 equals랑 hashcode 재정의를 해주셨는데요!
혹시 각각이 어떤 이유에서 재정의했으며, 어떤 로직에서 재정의한 부분이 사용되는지에 대해 저에게 설명해주실 수 있을까요?
| String profitRate = lottoService.calculateProfitRate(matchCounts, purchaseAmount); | ||
| outView.printLotteryStatistics(matchCounts, profitRate); | ||
| } | ||
| } |
There was a problem hiding this comment.
전체적으로 Controller 클래스임에도 출력문이 직접적으로 작성되어 있는 것 같아요!
MVC 패턴을 적용시키신 김에 출력의 책임은 컨트롤러에서 제외하는 것이 어떨까요?
There was a problem hiding this comment.
저는 Controller가 출력까지 책임지는 것으로 이해하고 있었는데,
상희님 말씀대로라면 Controller가 그 책임을 갖지 않는 구조로 보입니다.
혹시 출력의 책임은 어느 계층에서 가져가는 게 더 적절할지 조언해주실 수 있을까요?
There was a problem hiding this comment.
제가 말한 부분은 아래 코드인데요, 현재 print문은 모두 OutputView에서 작성해주신 것 같아요!
컨트롤러에서의 print문은 제거하고, OutputView가 출력의 책임을 모두 담당하게 하는 것이 어떨까요?
System.out.println();| public List<LottoNumber> getNumbers() { | ||
| return Collections.unmodifiableList(numbers); | ||
| } |
There was a problem hiding this comment.
불변성을 보장하도록 로직을 작성하셨네요. 좋습니다 👍
다만, 해당 클래스의 생성자 코드를 살펴보시면 방어 로직이 중복될 수 있다는 것을 발견할 수 있습니다.
public LottoNumbers(List<LottoNumber> numbers) {
validateSize(numbers);
this.numbers = List.copyOf(numbers); // <--- 이 부분!
}List.copyOf()의 동작에 주목할 필요가 있는데요, 인자로 받은 컬렉션의 복사본 자체가 이미 불변 리스트를 생성하여 반환하는 동작을 수행합니다. 따라서 getter에서 불변성을 보장하는 로직은 중복되는 방어 로직으로 보입니다.
There was a problem hiding this comment.
말씀해주신 대로 getter에서 Collections.unmodifiableList()로 한 번 더 감싸는 부분은 중복된 방어 로직이라 수정하겠습니다.
getNumbers()에서는 바로 numbers를 반환하도록 변경하겠습니다.
| for (LottoNumber number : oneLotto) { | ||
| if (lastNumbers.contains(number.getNumber())) { | ||
| matchCount++; | ||
| } | ||
| } |
There was a problem hiding this comment.
이 부분도 depth가 2이상인 것 같습니다.
어떤 방식으로 수정하면 좋을지 고민해보시면 좋을 것 같아요!
There was a problem hiding this comment.
if문 내부 로직을 별도의 메서드(containsNumber)로 분리하고,
해당 메서드가 1 또는 0을 반환하도록 하여 matchCount에 더하는 방식으로 depth를 줄여보았습니다.
src/main/java/model/MatchResult.java
Outdated
| public int getCount() { | ||
| return count; | ||
| } | ||
|
|
||
| public String getDescription() { | ||
| return description; | ||
| } |
There was a problem hiding this comment.
현재 사용하지 않는다고 뜨는데, 안 쓰는 코드는 지워주셔도 될 것 같아요!
There was a problem hiding this comment.
네, 현재 사용하지 않는 코드라 말씀해주신 대로 제거했습니다. 감사합니다.
src/main/java/model/MatchResult.java
Outdated
| for (MatchResult result : values()) { | ||
| if (result.count == count && result != FIVE_BONUS) return result; | ||
| } |
There was a problem hiding this comment.
요 부분 depth가 2이상인 것 같은데 어떻게 수정하면 좋을까요?
There was a problem hiding this comment.
맨 처음에는 if문 내부의 로직을 하나의 메서드로 분리하여 MatchResult를 반환하도록 시도했습니다.
하지만 이 방식은 조건을 만족하더라도 for문이 계속 실행되는 문제가 있었고,
동시에 fromCount의 반환값을 적절히 처리하기 어려웠습니다.
그래서 새로운 방식으로 두 개의 메서드를 분리했습니다.
하나(isMatched)는 조건을 만족하면 boolean 값을 반환하도록 하고,
다른 하나(findMatchResult)는 이 boolean 값이 true일 경우 해당 enum을,
그렇지 않은 경우 ZERO를 반환하도록 변경했습니다.
이를 통해 fromCount의 모든 반환값을 깔끔하게 설정할 수 있었습니다.
| @Test | ||
| void 고정된숫자를_반환() { | ||
| // Given | ||
| List<Integer> input = List.of(1, 2, 3, 4, 5, 6); | ||
| FixedLottoNumberGenerator generator = new FixedLottoNumberGenerator(input); | ||
|
|
||
| // When | ||
| LottoNumbers lotto = generator.generate(); | ||
|
|
||
| // Then | ||
| // LottoNumbers.getNumbers()는 LottoNumber 객체들이라서 | ||
| // 그대로 toString()해서 비교하면 간단해짐 | ||
| assertEquals(input.toString(), lotto.getNumbers().toString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
현재 이 테스트는 생성자에 넣은 값이 generate() 메서드를 통해 그대로 반환되는지 확인하는 '기본 동작 검증' 테스트라고 생각합니다!
여기서 '더 유의미한 테스트는 무엇일까'에 대해 생각해보면 어떨까요?
There was a problem hiding this comment.
숫자를 제어할 수 있을 때 더 유의미한 테스트는 예외 상황을 검증하는 것이라고 생각합니다.
따라서 6개의 숫자가 아닌 경우나 1~45 범위를 벗어나는 숫자가 포함된 경우에
예외가 발생하는 테스트를 추가해보겠습니다.
| // Then | ||
| assertEquals(1, result.get(MatchResult.FIVE_BONUS)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixture 객체
제가 하늘님께도 남겼던 리뷰인데 참고하면 좋을 것 같아요!
There was a problem hiding this comment.
말씀해주신 Fixture 객체 개념을 적용해서,
테스트 전용 LottoFixture 클래스를 생성하고 중복 코드를 정리했습니다.
테스트 코드의 가독성이 훨씬 좋아졌습니다 👍
좋은 개념 알려주셔서 감사합니다.
| @Test | ||
| void 랜덤생성기는_항상_6개의_번호를_만든다() { | ||
| RandomLottoNumberGenerator generator = new RandomLottoNumberGenerator(); | ||
| LottoNumbers lottoNumbers = generator.generate(); | ||
| assertEquals(6, lottoNumbers.getNumbers().size()); | ||
| } | ||
| } |
There was a problem hiding this comment.
태우님은 로또 번호를 생성하실 때, 45개의 숫자를 섞고 numbers.subList(0, 6) 코드로 6개를 잘라서 생성하고 있습니다. subList 메서드는 Java의 내장 기능으로, 6개의 요소를 반환하는 것이 이미 보장되어 있습니다. 즉, 이 테스트는 저희의 비즈니스 로직보다는 Java 라이브러리의 기능을 검증하는 것에 가까워 보입니다.
저희가 단위 테스트의 커버리지를 올리는 것도 중요하겠지만, 그 전에 이 테스트가 '우리의 비즈니스 규칙'을 잘 검증하고 있는지에 대한 고민을 같이 해보는 건 어떨까요?
There was a problem hiding this comment.
저희의 비즈니스 규칙 중 하나는 ‘로또는 6개의 번호를 가져야 한다’는 것이라고 생각했습니다.
그래서 해당 규칙을 검증하기 위해 6개의 번호가 생성되는지를 테스트했는데,
제가 구현할 때 사용한 subList(0, 6)이 이미 Java 내부적으로 6개의 요소를 보장한다는 점을 이해했습니다.
따라서 이 테스트는 비즈니스 로직 자체를 검증하기보다는 Java 라이브러리의 동작을 확인하는 테스트에 가깝다고 생각합니다.
즉, 이 테스트는 제거해도 괜찮을 것 같습니다.
로또 번호는 1부터 45 사이의 숫자만 포함된다고 알고 있습니다.
createInputLotto 메서드는 현재 수동 로또 번호를 입력받을 때와 지난주 당첨 번호를 입력받을 때 모두 사용하고 있습니다.
equals와 hashCode는 객체를 설계할 때 기본적으로 고려해야 하는 개념으로, 저번 리뷰 과정에서 배웠습니다. |
There was a problem hiding this comment.
createInputLotto 메서드는 현재 수동 로또 번호를 입력받을 때와 지난주 당첨 번호를 입력받을 때 모두 사용하고 있습니다.
그래서 ‘수동 로또 번호 생성’으로 이름을 한정하면 의미 전달이 오히려 애매해질 것 같아 createInputLotto로 유지했습니다.
혹시 더 좋은 이름이 있다면 추천해주시면 감사하겠습니다!!
앗 제가 자동, 수동 로또에서 둘 다 사용된다는 것을 놓쳤나봐요. 유지하는 게 좋을 것 같아요 👍
이번 미션에서는 로또 번호와 로또 객체가 핵심 도메인이라고 판단했기 때문에,
PurchaseAmount는 모델보다는 뷰에 더 가깝다고 생각했습니다.
이 부분에 대해서는 코멘트로 자세히 답변하도록 하겠습니다. 저는 사용자의 입력을 받아서, 결과를 보여주는 것이 view의 역할이라고 생각하는데요. 결론만 말씀드리자면, 저는 PurchaseAmount 가 하나의 도메인 객체로 충분하다고 생각합니다!
equals와 hashCode는 객체를 설계할 때 기본적으로 고려해야 하는 개념으로, 저번 리뷰 과정에서 배웠습니다.
그때 공부했던 내용을 실제로 적용해보고 싶어서 이전 단계에 반영해보았습니다.
이번 로직에서는 필수적인 부분은 아니지만, 객체의 동등성 개념을 연습하고 익히는 데 도움이 되었다고 생각합니다.
오 리뷰를 적용하려고 노력하신 점이 보여서 좋네요. 하지만 이번 로직에서는 필수적인 부분이 아니라고 하셨는데, 저는 LottoNumber에 재정의된 부분은 필수적으로 있어야 할 것 같아요. 또한 재정의한 부분을 이용하면 좋은 코드도 발견했는데, 이는 코멘트로 달아뒀습니다!
| equals, hashcode 정상 동작 | equals, hashcode 주석처리 |
|---|---|
 |
 |
private boolean matchBonus(List<LottoNumber> oneLotto, LottoNumber bonusBall, int count) {
if (count == 5) {
return oneLotto.contains(bonusBall);
}
return false;
}주석 처리 했을 때 돌아가지 않는 이유는 해당 코드 때문인데요, 이유를 찾아보시고 설명해주시면 좋을 것 같습니다.
✚ 추가적으로, 태우님은 아래의 두 클래스에도 재정의를 해주셨습니다.
LottoNumbers = 로또 한 장
LottoTicketBundle = 로또 여러 장
현실 세계에서의 의미를 비교해볼 때, 이렇게 생각될 것 같아요. 그렇다면 이 둘은 값 객체일까요?
태우님이 [1,2,3,4,5,6] 로또 한 장을 구매했습니다. 저도 정확히 같은 번호로 [1,2,3,4,5,6] 따라 샀다고 가정한다면, 저희 둘의 로또는 같을까요?
| String profitRate = lottoService.calculateProfitRate(matchCounts, purchaseAmount); | ||
| outView.printLotteryStatistics(matchCounts, profitRate); | ||
| } | ||
| } |
There was a problem hiding this comment.
제가 말한 부분은 아래 코드인데요, 현재 print문은 모두 OutputView에서 작성해주신 것 같아요!
컨트롤러에서의 print문은 제거하고, OutputView가 출력의 책임을 모두 담당하게 하는 것이 어떨까요?
System.out.println();| return counts; | ||
| } | ||
|
|
||
| private int matchLottoNumber(List<LottoNumber> oneLotto, List<LottoNumber> lastLotto) { |
There was a problem hiding this comment.
메서드 인자에 oneLotto, lastLotto라고 적힌 부분이 꽤 있는 것 같은데,
다른 사람들이 이 네이밍을 통해 해당 변수들이 의미하는 것을 알기 힘들 것 같습니다.
조금 더 해당 변수들의 의미를 담은 네이밍으로 변경하는 것은 어떨까요?
| } | ||
|
|
||
| private int containsNumber(List<Integer> lastNumbers, LottoNumber number) { | ||
| if (lastNumbers.contains(number.getNumber())) return 1; |
There was a problem hiding this comment.
이 부분은 사실 굳이 number.getNumber() 없이도 바로 비교를 진행할 수 있을 것 같아요.
태우님께서 리뷰를 반영하셔서 equals, hashcode를 재정의해주셨는데,
그렇기 때문에 값 객체(값이 같으면 같은 객체로 취급)의 특성을 살려 객체 자체를 비교해도 정상 동작할 것 같습니다!
private int containsNumber(List<Integer> lastNumbers, LottoNumber number) {
if (lastNumbers.contains(number)) {
return 1;
}
return 0;
}추가적으로 스트림을 사용하면 이 부분도 depth를 줄이며 간결하게 표현 가능합니다!
private int matchLottoNumber(List<LottoNumber> oneLotto, List<LottoNumber> lastLotto) {
return (int) oneLotto.stream()
.filter(number -> lastLotto.contains(number)) // 메소드 참조로 lastLotto::contains 로도 가능
.count();
}| TWO(2, 0), | ||
| THREE(3, 5000), | ||
| FOUR(4, 50000), | ||
| FIVE(5, 150000), |
There was a problem hiding this comment.
여기 0이 하나 빠진 것 같습니다!
그리고 ONE과 TWO는 출력 결과에 영향을 미치지 않을 것 같은데,
정의해주신 이유가 궁금합니다!
| public static MatchResult fromCount(int count, boolean bonusMatch) { | ||
| if (count == 5 && bonusMatch) { | ||
| return FIVE_BONUS; | ||
| } | ||
|
|
||
| MatchResult[] results = values(); | ||
| return findMatchResult(results, count); | ||
| } | ||
|
|
||
| private static MatchResult findMatchResult(MatchResult[] results, int count) { | ||
| int i = 0; | ||
| while (i < results.length && !isMatched(results[i], count)) { | ||
| i++; | ||
| } | ||
|
|
||
| if (i == results.length) { | ||
| return ZERO; | ||
| } | ||
|
|
||
| return results[i]; | ||
| } | ||
|
|
||
| private static boolean isMatched(MatchResult result, int count) { | ||
| return result.count == count && result != FIVE_BONUS; | ||
| } | ||
| } |
There was a problem hiding this comment.
depth를 줄이는 방법으로 함수의 분리를 택하셨군요!
메서드 단위로 책임을 분리시키는 것도 하나의 방법이 될 수 있죠 좋아요!
다만, 태우님도 느끼셨을 것 같은데 코드가 조금 길어지고 복잡해진 것 같아요.
또한 배열을 사용하셨는데, 현재 ENUM 객체를 만드신 이유가 궁금해졌습니다.
태우님은 ENUM을 사용하신 이유가 무엇인가요?
배열을 의미 있는 단위에 사용하게 되면
- 단순 숫자인 인덱스에 코드 작성자가 어떤 의미를 부여했는지
- 그 의미에 해당하는 값이 무엇을 의미하는지
이 두 가지에 대한 생각을 정리하고 읽어야 하고,
심지어는 코드 작성자의 의도를 추측해야 하는 상황이 발생할 수 있어요.
팀원이 추측을 잘못한 후 코드를 작성하면 참사가 발생할 수도 있을 것 같습니다.
결론적으로, 태우님이 ENUM을 사용하여 각각의 값의 의미를 명확히 나타낸 것과는
다른 방향으로 코드가 흘러가게 될 수 있을 것 같아요 🥲
아래는 스트림을 사용한 코드인데요, 어색할 수 있지만 때로는 depth를 해결하는 좋은 방법으로 작용할 수 있을 것 같아서 코드 예시를 가져왔습니다. 이런 방식은 어떠신가요?
public static MatchResult fromCount(int count, boolean bonusMatch) {
if (count == 5 && bonusMatch) {
return FIVE_BONUS;
}
return Arrays.stream(values())
.filter(result -> result.count == count && result != FIVE_BONUS)
.findFirst()
.orElse(ZERO);
}| private static void validateNegative(int value) { | ||
| if (value < 0) { | ||
| throw new IllegalArgumentException("구입 금액은 0원 이상입니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
[1] PurchaseAmount는 View인가 Domain인가?
구입 금액은 0원 이상이어야 한다는 규칙은 '화면에 보여주기 위한 규칙'일까요, 아니면 '로또 게임의 핵심 규칙'일까요?
| public int howManyLottos() { | ||
| return value / 1000; | ||
| } |
There was a problem hiding this comment.
[2] PurchaseAmount는 View인가 Domain인가?
구입 금액으로 로또를 몇 개 살 수 있는지 계산하는 로직은 어디에 속하는 책임일까요?
만약 로또 가격이 1500원으로 바뀌는 비즈니스 규칙 수정 사항이 발생한다면, View와 Domain 중 어느 부분을 수정하는 것이 의미적으로 자연스러울까요?
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (!(o instanceof PurchaseAmount)) return false; | ||
| PurchaseAmount money = (PurchaseAmount) o; | ||
| return value == money.value; | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(value); | ||
| } |
There was a problem hiding this comment.
[3] PurchaseAmount는 View인가 Domain인가?
이 클래스는 왜 값으로 동등성을 비교하고 있을까요?
안녕하세요 상희님!
상희님께 리뷰를 처음 요청드리게 되었는데, 이번 기회에 많이 배우겠습니다 🙌
저번 미션에서는 Enum을 활용해 제3자가 코드를 더 쉽게 이해할 수 있도록 하는 피드백을 받았습니다.
이번 미션에서는 그 부분을 신경쓰면서 메서드명과 지역 변수명을 보다 명확하게 작성하려고 노력했습니다.
또한, 메서드별 코드 길이가 길어지는 문제를 책임을 분리하는 방식으로 개선하고자 했습니다.
제 코드 구성은 아래와 같습니다.
클래스 구성
controller
로또 게임 실행 흐름 관리
(금액 입력 → 수동/자동 티켓 생성 → 당첨 번호/보너스 볼 입력 → 통계 출력)
View
구입 금액, 수동 구매 개수, 수동 번호, 당첨 번호, 보너스 볼 입력 처리
금액 안내, 구매 수량, 로또 번호, 당첨 통계 출력
구입 금액을 값 객체로 포장 (0원 이상 검증, 로또 구매 개수 계산)
model (Domain)
1~45 사이 번호 하나를 원시값 포장 (범위 검증)
LottoNumber 6개를 모은 일급 컬렉션 (6개 검증, 정렬, 불변 리스트)
여러 장의 LottoNumbers를 저장·조회하는 저장소 (읽기 전용 리스트 반환)
당첨 결과(3개, 4개, 5개, 5개+보너스, 6개 등)와 상금 정의
로또 생성, 입력 로또 변환, 당첨 결과 판별, 수익률 계산 등 핵심 비즈니스 로직
1~45 번호를 섞어 6개를 랜덤으로 생성하는 기본 구현체
Main
프로그램 엔트리 포인트
이번 구현 과정에서 생긴 궁금점은 입력받은 문자열을 어디서 파싱해야 하는가 입니다.
저는 View에서는 입력과 출력만 담당하고, 파싱은 Service에서 처리하는 방식으로 구현했습니다.
이렇게 하면 역할이 분명히 나뉘는 장점이 있다고 생각하는데, 상희님은 보통 어떤 방식을 선호하시는지 궁금합니다.
또한, 이번 미션에서는 네이밍을 이해하기 쉽게 하려고 노력했는데
메서드별 책임이 잘 드러나고 있는지, 읽기에도 무리가 없는지 피드백 부탁드립니다 🙏
아직 배우는 단계라 제가 미처 생각하지 못한 부분이 많습니다.
더 개선할 수 있는 점이 있다면 자유롭게 말씀해주시면 감사하겠습니다!