[로또] 김시영 미션 제출합니다. #108

wfs0502 · 2025-05-17T16:06:46Z

안녕하세요! 리뷰 잘 부탁드립니다. 감사합니다🙇‍♀️

Lotto, LottoNumber 같은 도메인 객체들을 어떻게 나누고 어디까지 분리할지에 대한 판단을 어느 정도까지 하고 시작해야 하는지가 어려웠습니다.
리팩토링은 당연히 계속 해야겠지만 수정이 자주 발생해 구현에 시간이 더 오래 걸린 것 같아서 궁금합니다.

hong-sile · 2025-05-19T02:25:31Z

Lotto, LottoNumber 같은 도메인 객체들을 어떻게 나누고 어디까지 분리할지에 대한 판단을 어느 정도까지 하고 시작해야 하는지가 어려웠습니다. 리팩토링은 당연히 계속 해야겠지만 수정이 자주 발생해 구현에 시간이 더 오래 걸린 것 같아서 궁금합니다.

실제 수정이 발생하는 건 자연스러운 겁니다. 우리는 이서비스에 대한 이해도를 쌓기전에 개발을 시작하니까요.

음... 제 개인적인 방법이 하나 있는데 맨 처음에 요구사항을 정리하고 그 요구사항에서 최대한 도메인 모델들을 추출하고 책임을 먼저 설계하고 시작하는 편입니다.

기회가 된다면 다음에 한번 보여드릴 게요.
시영님도 한번 요구사항 정리해보고, 그 요구사항에서 설계를 해보는걸 시작해보면 좋을 것 같습니다.

hong-sile

미션 수고하셨습니다. 전체적으로 잘 짜주셨군요. stream도 잘 써주시고요.

몇몇 부분에 대한 코멘트를 남겼습니다.
한번 디미터의 법칙과 도메인 로직에 책임을 모아보는걸 중점적으로 리팩터링해주시면 좋을 것 같아요

늘 그렇듯 리뷰에 대한 피드백,반박,질문은 언제나 환영입니다.

hong-sile · 2025-05-18T17:20:54Z

src/test/java/domain/LottoResultTest.java

+class LottoResultTest {
+    @Test
+    void secondRankTest() {
+        Lotto lotto1 = new Lotto(Stream.of(1, 2, 3, 4, 5, 6)


stream.of와 list.of의 차이는 무엇인가요? 어떠한 상황에서 시영님은 stream.of를 쓰시나요?

map 연산을 바로 사용하고자 Stream.of을 사용했습니다!
스트림 없이 컬렉션만 필요할 때는 List.of()을 사용합니다

hong-sile · 2025-05-19T00:59:17Z

src/main/java/controller/LottoController.java

+        }
+        int autoCount = count - manualCount;
+
+        List<Lotto> lottoList = new ArrayList<>();


객체명에서 List와 같은 자료형을 명시하기보다. lottos 와 같은 복수형으로만 표현하는건 어떨까요??

그리고 제가 어째서 이런 제시?를 하는걸까요?

List라고 명시하면 나중에 내부 구현이 바뀌는 경우에 변수명도 수정돼야해서 유지보수가 어렵습니다. 그리고 가독성면에서도 더 좋을 것 같습니다!

hong-sile · 2025-05-19T02:02:27Z

src/main/java/domain/LottoRank.java

+        this.prize = prize;
+    }
+
+    public static LottoRank findRank(int matchCount, boolean bonusMatch) {


음 정적으로 Rank를 반환하는 메서드를 만든건 좋은 것 같아요.

그런데, 만약 Enum의 값이 바뀌면 이 메서드도 수정되어야 해요. 로직이 살짝 흩어져있다는 느낌이 들어요. 어떻게 하면 변경에도 실수할만한 요소를 줄일 수 있을까요?

2등과 5등 조건 때문에 처음에는 정적으로 작성했었는데 찾아보니 BiPredicate를 활용해서 각 Enum이 스스로 정하도록 구현하는 방법이 더 유지보수에 좋을 것 같습니다!

오 BiPredicate까지 하실거라곤 생각도 못했었요.
Enum과 함수형 인터페이스까지 잘 쓰시는군요

hong-sile · 2025-05-19T02:05:06Z

src/main/java/domain/LottoResult.java

+        return (double) totalPrize / totalCost;
+    }
+
+    public Map<LottoRank, Integer> getResult() {


일급컬렉션을 사용하는데, 직접적으로 map을 반환해서 연산하는군요.
이 map을 이용해서만 구할 수 있는 연산은 별도의 메서드로 추가하는건 어떨까요?

물론 이 경우엔 시영님이 값을 잘 복사해서 넘겨주어서, 나중에 값이 바뀐다던가 하는 문제가 생길 요소는 없습니다. 이렇게 계속 방어적으로 반환하는건 되게 잘하시는 것 같아요.
그래도 유사한 값이 클래스 객체록 관리하지 않고 돌아다니는건 응집도가 떨어진 느낌이 들어서요.

추가적으로 일급컬렉션을 왜 사용하는지?에 대해서 학습하고 리뷰 답변할 떄 적어주시면 좋을 것 같습니다.

아 일급 컬렉션의 의도를 이제 이해한 것 같습니다!!! 내부 컬렉션을 직접 노출시키지 않고 캡슐화하는게 핵심인데 직접 반환하게되면 소용이 없네요.. 그대로 Map을 반환하기보다 관련 연산들을 메서드로 제공하도록 개선하겠습니다!

hong-sile · 2025-05-19T02:07:46Z

src/main/java/domain/LottoResult.java

+            int matchCount = winningLotto.countMatch(lotto);
+            boolean bonusMatch = winningLotto.matchBonus(lotto);
+            LottoRank rank = LottoRank.findRank(matchCount, bonusMatch);
+            result.put(rank, result.getOrDefault(rank, 0) + 1);


이런건 map의 merge 메서드를 활용해볼 수 있어요.

기존 키에 값이 있으면 +1을하고, 없으면 값을 넣는건 되게 자주쓰는 방식이니 좋습니다.

오 훨씬 편리하네요!!! 감사합니다 코드에 적용하겠습니다!

hong-sile · 2025-05-19T02:09:03Z

src/main/java/domain/LottoRank.java

+    THIRD(5, 1500000),
+    FOURTH(4, 50000),
+    FIFTH(3, 5000),
+    MISS(0, 0);


요것까지 정의한 경우는 처음보는 것 같은데, Miss를 추가한것도 엄청 좋은 것 같네요.

hong-sile · 2025-05-19T02:10:47Z

src/main/java/domain/Lotto.java

+    public static final String ERROR_DUPLICATION = "로또 번호는 중복될 수 없습니다.";
+    private final List<LottoNumber> numbers;
+
+    public Lotto(NumberGenerator generator) {


this 생성자를 이용해서, validate 로직활용한 것도 좋네요. 요런식으로 default 생성자하나를 두고,
다른 방법으로 init하는 생성자가 추가될 때, 기존 validate 룰을 지키기 좋습니다.

불변식이란 키워드도 한번 알아보면 좋을 것 같아요

hong-sile · 2025-05-19T02:12:15Z

src/main/java/domain/WinningLotto.java

+
+    public int countMatch(Lotto other) {
+        return (int) other.getNumbers().stream()
+                .filter(winningLotto.getNumbers()::contains)


이렇게 직접적으로 get해서 가져오는 것보다, 원하는 값을 요청하는 방식이 좀 더 캡슐화와 응집도를 지키기에 좋습니다.

한번 디미터의 법칙에 대해서 학습해보시면 좋을 거 같아요

이렇게 get만 하게 되면, 도메인이 단순히 값만 갖게되고 나중에 외부에서

getXXX().getAAA()... 과 같은 형태가 되어서 되게 상위에서 다루기 힘든 형태가 되고맙니다.

디미터의 법칙을 찾아보고 적용해보니 구조가 더 명확해지고 가독성도 좋아진 것 같습니다.
메서드를 추가해서 외부에서는 필요한 값만 요청하도록 리팩토링하겠습니다. 피드백 감사합니다!!

hong-sile · 2025-05-19T02:23:05Z

src/test/java/domain/LottoResultTest.java

@@ -0,0 +1,33 @@
+package domain;


로직의 많은 부분이 Controller에 몰려있는 것 같아요.

이렇게 view가 결합되어있는 로직에서는 테스트를 짜기 어렵습니다. 최대한 도메인으로 로직을 밀어넣고, 관련한 테스트를 짜보면 어떨까요?

아 확실히 로직 분리가 부족해서 테스트하기 어려웠던 것 같습니다! 해당 부분 리팩토링하겠습니다 감사합니다

wfs0502

디미터의 법칙과 일급 컬렉션에 대해 더 깊이 이해하게 되었습니다! 단순히 코드를 수정하는 것을 넘어 객체지향 설계 원칙을 적용할 수 있었던 경험이었습니다.
항상 좋은 피드백 감사드립니다!

wfs0502 · 2025-05-22T10:23:50Z

src/main/java/domain/LottoRank.java

+        this.prize = prize;
+    }
+
+    public static LottoRank findRank(int matchCount, boolean bonusMatch) {


2등과 5등 조건 때문에 처음에는 정적으로 작성했었는데 찾아보니 BiPredicate를 활용해서 각 Enum이 스스로 정하도록 구현하는 방법이 더 유지보수에 좋을 것 같습니다!

wfs0502 · 2025-05-22T10:39:28Z

src/main/java/controller/LottoController.java

+        }
+        int autoCount = count - manualCount;
+
+        List<Lotto> lottoList = new ArrayList<>();


List라고 명시하면 나중에 내부 구현이 바뀌는 경우에 변수명도 수정돼야해서 유지보수가 어렵습니다. 그리고 가독성면에서도 더 좋을 것 같습니다!

wfs0502 · 2025-05-22T12:12:41Z

src/main/java/domain/LottoResult.java

+            int matchCount = winningLotto.countMatch(lotto);
+            boolean bonusMatch = winningLotto.matchBonus(lotto);
+            LottoRank rank = LottoRank.findRank(matchCount, bonusMatch);
+            result.put(rank, result.getOrDefault(rank, 0) + 1);


오 훨씬 편리하네요!!! 감사합니다 코드에 적용하겠습니다!

wfs0502 · 2025-05-22T12:31:46Z

src/main/java/domain/WinningLotto.java

+
+    public int countMatch(Lotto other) {
+        return (int) other.getNumbers().stream()
+                .filter(winningLotto.getNumbers()::contains)


디미터의 법칙을 찾아보고 적용해보니 구조가 더 명확해지고 가독성도 좋아진 것 같습니다.
메서드를 추가해서 외부에서는 필요한 값만 요청하도록 리팩토링하겠습니다. 피드백 감사합니다!!

wfs0502 · 2025-05-22T12:40:41Z

src/test/java/domain/LottoResultTest.java

+class LottoResultTest {
+    @Test
+    void secondRankTest() {
+        Lotto lotto1 = new Lotto(Stream.of(1, 2, 3, 4, 5, 6)


map 연산을 바로 사용하고자 Stream.of을 사용했습니다!
스트림 없이 컬렉션만 필요할 때는 List.of()을 사용합니다

wfs0502 · 2025-05-22T12:50:48Z

src/main/java/domain/LottoResult.java

+        return (double) totalPrize / totalCost;
+    }
+
+    public Map<LottoRank, Integer> getResult() {


아 일급 컬렉션의 의도를 이제 이해한 것 같습니다!!! 내부 컬렉션을 직접 노출시키지 않고 캡슐화하는게 핵심인데 직접 반환하게되면 소용이 없네요.. 그대로 Map을 반환하기보다 관련 연산들을 메서드로 제공하도록 개선하겠습니다!

wfs0502 · 2025-05-22T12:58:10Z

src/test/java/domain/LottoResultTest.java

@@ -0,0 +1,33 @@
+package domain;


아 확실히 로직 분리가 부족해서 테스트하기 어려웠던 것 같습니다! 해당 부분 리팩토링하겠습니다 감사합니다

hong-sile

전체적으로 잘 해주셨습니다. 2번째 리뷰가 늦어져서 죄송합니다..

다음 미션진행하시죠!

이번 미션에서 책임을 여러 곳으로 분리해보는 걸 시도해봤으니 더 잘 할 수 있을 거라고 생각합니다.

indent(인덴트, 들여쓰기) depth를 2를 넘지 않도록 구현한다. 1까지만 허용한다.
예를 들어 while문 안에 if문이 있으면 들여쓰기는 2이다.
힌트: indent(인덴트, 들여쓰기) depth를 줄이는 좋은 방법은 함수(또는 메서드)를 분리하면 된다.

함수(또는 메서드)의 길이가 10라인을 넘어가지 않도록 구현한다.
함수(또는 메서드)가 한 가지 일만 하도록 최대한 작게 만들어라.

요 요구사항을 한번 전체적으로 잘 지켜보면 좋을 것 같습니다.

전체적으로 메서드가 하나의 일만 하도록 하는 강제사항들인데 다음엔 꼭 이걸 지켜보면서 짜보면 좋을 것 같습니다.

수고하셨습니다. 디스코드에 KPT 회고도 올려주세용

hong-sile · 2025-05-23T10:09:43Z

src/main/java/domain/Lottos.java

+
+    public static Lottos create(List<List<Integer>> manualNumbers, int autoCount, NumberGenerator generator) {
+        List<Lotto> totalLottos = new ArrayList<>();
+        for (List<Integer> numbers : manualNumbers) {


요렇게 List를 이용해서 Lotto 객체를 생성하는 로직 또한 Lotto에 있으면 어떨까요?

요구사항 중에
indent(인덴트, 들여쓰기) depth를 2를 넘지 않도록 구현한다. 1까지만 허용한다.
이런 것이 있었는데 요런 요구사항에 맞춰보았으면 어떨까하네용

hong-sile · 2025-05-23T10:13:14Z

src/main/java/domain/Lotto.java

+    }
+
+    private void validateDuplication(List<LottoNumber> numbers) {
+        if (numbers.stream()


요런 stream 연산을 if문 내부에 넣게 되면 보기 힘들 수 있어요. 요런 것들은 라인을 분리하던거 별도로 메서드로 분리해보면 좋을 것 같습니다.

indent(인덴트, 들여쓰기) depth를 2를 넘지 않도록 구현한다. 1까지만 허용한다.

요 요구사항이 있는 이유는 각 메서드마다 책임을 최대한 작게 가져가기 위함이에요. 한번 이런걸 시도해보면 좋을 것 같습니다.

hong-sile · 2025-05-23T10:20:42Z

src/main/java/domain/Lottos.java

+        this.lottos = lottos;
+    }
+
+    public static Lottos create(List<List<Integer>> manualNumbers, int autoCount, NumberGenerator generator) {


요건 객체를 생성하는데 여러 연산이 들어가서 조금 중요한 로직인 것 같아요. 이 메서드에 대해서도 테스트를 짜보는건 어떨까요?

hong-sile · 2025-05-23T10:21:02Z

src/main/java/domain/WinningLotto.java

+        this.bonusNumber = bonusNumber;
+    }
+
+    public static WinningLotto create(List<Integer> winningNumbers, int bonusNumber) {


요 정적팩터리 메서드도 테스트해봤으면 좋았을 것 같아요

wfs0502 added 20 commits May 17, 2025 13:24

feat: 로또 랜덤 번호 생성 구현

f709bec

refactor: 로또 번호 생성 로직 Generator로 분리

4f6403d

feat: InputView, ResultView 구현

08f79a8

feat: 당첨 번호 입력 및 검증 구현

d5dfd5c

refactor: 상수 분리

6a11768

feat: 로또 번호 일급 컬렉션 Numbers 구현

f8c7d6c

feat: Lottos 구현

1364893

refactor: LottoController 분리

f2af3c4

feat: LottoRank 구현

608255c

feat: 당첨 결과 계산 및 출력 구현

e65cda3

fix: 출력 메시지 수정

e708800

feat: 수익률 계산 및 출력

3974147

feat: 보너스 번호 입력 및 검증 구현

aaec4b7

feat: 2등 당첨 구현

845570f

feat: 수동 로또 구현

182d396

fix: 수익률 출력 메시지 수정

fa0cb60

fix: 수익률 출력 메시지 수정

ebd94ee

feat: 수동 구매 수량 초과 시 예외 발생 추가

5f32192

Refactor: Numbers 클래스 Lotto 클래스로 통합

aaf9a42

test: 테스트 코드 추가

8bc96eb

hong-sile suggested changes May 19, 2025

View reviewed changes

wfs0502 added 4 commits May 22, 2025 21:35

refactor: LottoRank 랭크 판별 로직 동적화

23a5b8e

refactor: LottoController 변수명 수정

3427ca1

refactor: LottoResult 랭크 카운트 개선

6423647

refactor: Lotto 디미터 법칙 적용

3314072

wfs0502 commented May 22, 2025

View reviewed changes

wfs0502 added 2 commits May 22, 2025 23:45

refactor: LottoController 책임 분리

b673b11

refactor: LottoResult Map 직접 노출 제거

380426d

hong-sile approved these changes May 23, 2025

View reviewed changes

boorownie merged commit 7bfa61a into next-step:wfs0502 Jun 3, 2025

[로또] 김시영 미션 제출합니다. #108

[로또] 김시영 미션 제출합니다. #108

Uh oh!

Conversation

wfs0502 commented May 17, 2025

Uh oh!

hong-sile commented May 19, 2025

Uh oh!

hong-sile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfs0502 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hong-sile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!