[2단계 - 로또(수동)] 오찌(오지훈) 미션 제출합니다. #438

Ohzzi · 2022-03-01T12:37:35Z

안녕하세요 닉 :)

2단계 수동 구현을 하면서 여러가지로 머리를 싸맸네요...
개인적으로는 코드가 아직은 조금 난잡한게 아닌가 하고 생각이 들지만, 우선 구현을 끝내서 이렇게 PR을 드립니다!
질문이 있는 부분은 코드에 남기도록 할게요 :D
(1단계 PR에도 질문 남겨두었습니다!)

이번에도 많은 가르침 부탁드립니다!!

LottoPurchasingMoney가 로또 구매 금액(1000단위)만 남기게 하여 수익률이 작게 나오는 오류 수정

Ohzzi · 2022-03-01T12:39:50Z

src/main/java/controller/LottoController.java

+import view.OutputView;
+
+public class LottoController {
+    public void run() {


코드를 작성하고 보니 controller가 너무 난잡한게 아닌가 하는 생각이 들어요!
service 레이어를 분리해볼까 라고 생각은 했지만 아직 controller와 service의 분리 기준도 잘 모르겠고요!
생각해보면 controller에서 여러 메서드를 public으로 열어 놓고 main에서 여러 메서드를 호출하면서 진행해야 하는 게 아닐까 하는 생각도 하고 있습니다. 닉은 어떻게 생각하시나요??

음 저는 main은 클린하게 유지되어야 한다고 생각해요. (애플리케이션을 실행시키는 역할만)
말씀하신대로 controller에서 여러 메서드를 public으로 열어 놓고 main에서 여러 메서드를 호출하면서 진행하게 되면 오히려 Model과 View를 통해 애플리케이션의 흐름을 제어하는 Controller의 책임이 main으로 분산된다고 생각하거든요.

난잡하다고 느끼신 부분은 많은 private 메서드 때문일지도 모르겠네요. 그럴때마다 객체가 너무 많은 일을 하고 있지는 않은지 고민해보시면 좋아요.
아래에 따로 코멘트 남길게요 :)

Ohzzi · 2022-03-01T12:41:46Z

src/main/java/controller/LottoController.java

+                    winningLotto.containBonusBall(lotto)
+            );
+            winningStatistics.put(lottoRank);
+        });


당첨 통계를 map에 저장하는 로직이 controller에 있어서 부자연스러울까요...?

네네 안그래도 이 부분에서 위에 남긴 코멘트 때문에 이야기하려고 했는데 ㅎㅎ
통계 객체에서 충분히 해줄 수 있는 일이 아닐까요? :)

Ohzzi · 2022-03-01T12:43:42Z

src/main/java/model/LottoOrder.java

+package model;
+
+public class LottoOrder {
+    static final String TOO_MANY_MANUAL = "[ERROR] 수동 로또 구매 수는 전체 구매 수를 넘을 수 없습니다.";


수동 구매 개수와 자동 구매 개수를 저장할 방법을 골똘히 고민하다가, 우아한객체지향 세미나에서 주문, 배달과 같은 모든 상태를 객체로 만들어서 관리했던 것이 생각나서 저도 구매 정보를 LottoOrder로 만들어서 저장했습니다!

이렇게 구매 정보를 클래스로 분리하는 방법이 좋을까요?

이 클래스는 따로 비즈니스 로직이 없는게 아닌가 해서 DTO로 만들지 않았는데, 그래도 view로 넘길 때 DTO로 만들어 주는게 좋을까요?

네네 잘하셨습니다 👍

음 저는 이 객체도 DTO라고 생각해요. 해당 객체를 가지고 Controller와 View 레이어에서 정보를 주고받으니까요 :)

Ohzzi · 2022-03-01T12:44:30Z

src/main/java/model/LottoRank.java

+
+import java.util.Arrays;
+
+public enum LottoRank {


마찬가지로 계층을 넘어갈 때 Enum도 DTO로 만들 필요가 있는지 궁금합니다!

Ohzzi · 2022-03-01T12:46:18Z

src/main/java/view/InputView.java

+        }
+        out.println("수동으로 구매할 번호를 입력해 주세요.");
+        List<List<Integer>> manualLottoNumbers = new ArrayList<>();
+        for (int i = 0; i < manualLottoCount; i++) {


이 부분에서는 일단 고전적인 for문을 사용했는데요, IntStream을 돌려서 인덱스 값은 무시하고 mapToObj와 collect로 반환하는 방법을 먼저 생각했었습니다! 다만 IntStream의 iterating 값이 안쓰이는데 stream을 열면 안되나? 하는 생각에 우선 고전적인 for문으로 작성했는데요, 이런 경우에 좋은 방법이 있을까요?

저는 어떤 수가 주어졌을 때, 그 수만큼 반복하는 행위는 일반 for문으로 작성해요.
고전이라고 표현하셨지만 ㅎㅎ 고전이라기 보다는 가독성이 훨씬 나을 때가 많거든요 :)

Ohzzi · 2022-03-01T12:47:42Z

src/test/java/model/LottoTest.java

+        Lotto actual = Lotto.from(lottoNumbers);
+
+        // then
+        assertThat(actual).isNotNull();


isNotNull로 정상 생성 테스트를 진행해도 될까요? 아니면 생성자 테스트는 예외 상황에 대한 테스트만 진행하면 굳이 진행하지 않아도 되는 걸까요?

오 이 코멘트를 지금 발견했네요. 저도 isNotNull()로 인스턴스 생성 테스트 진행합니다 :)
항상 정상 케이스와 예외 케이스 모두를 검증하는 차원에서요

Ohzzi · 2022-03-01T13:25:03Z

src/main/java/model/dto/LottoDto.java

+public class LottoDto {
+    private final List<LottoNumber> lottoNumbers;
+
+    public LottoDto(List<LottoNumber> lottoNumbers) {


이 부분에 DTO 관련 질문이 하나 더 있습니다! 제가 코드를 작성하고 보니까 controller에서 DTO에 필요한 값을 다 꺼내다가 생성자로 넣어주었는데요, 그냥 DTO 생성자에 원본 객체를 넣어주고 생성자 안에서 getter로 꺼내 쓰는 쪽이 조금 더 좋은 코드라고 보시나요?

네네 (파라미터가 많은 경우) 코드도 깔끔해지고 어떤 객체의 데이터를 꺼내서 만든 DTO야, 라는 것이 생성자에서 드러나면 해당 DTO를 사용하는 클라이언트 입장에서도 이해하기 쉬울 것 같아요 :)

wbluke

전 단계에서 구조를 잘 잡아주셔서 크게 어려운 건 없어보였네요 ㅎㅎ
몇 가지 가벼운 코멘트 남겼으니 한번 확인 부탁드려요 🙂

wbluke · 2022-03-02T23:57:40Z

src/main/java/model/LottoNumber.java

@@ -27,7 +28,7 @@ private LottoNumber(int number) {

    public static LottoNumber valueOf(int number) {
        validateRange(number);


안그래도 이 부분은 생성자에서 validation을 해야하나 정적 메서드에서 해야 하나 고민했던 부분이었는데 해결이 됐네요!! :)
그런데 질문이 있습니다! LottoNumber 같은 경우에는 생성자를 처음에 pool을 만들때만 쓰고 있어서 여기에 validation이 들어가면 이후 프로덕션에서 사용할 수가 없는데, 이런 경우에는 정적 팩토리 메서드에 들어가도 될까요?

아 네네 그런 경우는 어쩔 수 없겠네요 :)

wbluke · 2022-03-03T00:03:57Z

src/main/java/controller/LottoController.java

+import view.OutputView;
+
+public class LottoController {
+    public void run() {


음 저는 main은 클린하게 유지되어야 한다고 생각해요. (애플리케이션을 실행시키는 역할만)
말씀하신대로 controller에서 여러 메서드를 public으로 열어 놓고 main에서 여러 메서드를 호출하면서 진행하게 되면 오히려 Model과 View를 통해 애플리케이션의 흐름을 제어하는 Controller의 책임이 main으로 분산된다고 생각하거든요.

난잡하다고 느끼신 부분은 많은 private 메서드 때문일지도 모르겠네요. 그럴때마다 객체가 너무 많은 일을 하고 있지는 않은지 고민해보시면 좋아요.
아래에 따로 코멘트 남길게요 :)

wbluke · 2022-03-03T00:06:32Z

src/main/java/controller/LottoController.java

+                    winningLotto.containBonusBall(lotto)
+            );
+            winningStatistics.put(lottoRank);
+        });


네네 안그래도 이 부분에서 위에 남긴 코멘트 때문에 이야기하려고 했는데 ㅎㅎ
통계 객체에서 충분히 해줄 수 있는 일이 아닐까요? :)

wbluke · 2022-03-03T00:11:21Z

src/main/java/model/LottoOrder.java

+package model;
+
+public class LottoOrder {
+    static final String TOO_MANY_MANUAL = "[ERROR] 수동 로또 구매 수는 전체 구매 수를 넘을 수 없습니다.";


네네 잘하셨습니다 👍

음 저는 이 객체도 DTO라고 생각해요. 해당 객체를 가지고 Controller와 View 레이어에서 정보를 주고받으니까요 :)

wbluke · 2022-03-03T00:12:51Z

src/main/java/model/WinningStatistics.java

-        winningStatistics = new EnumMap<>(LottoRank.class);
-        Arrays.stream(LottoRank.values())
-                .forEach(lottoRank -> winningStatistics.put(lottoRank, 0));
+        winningCounts = new EnumMap<>(LottoRank.class);


요런건 필드에서 바로 초기화하는게 더 깔끔할 것 같아요 :)
(생성자가 여러 개로 늘어날 경우를 대비해서)

wbluke · 2022-03-03T00:15:57Z

src/main/java/model/dto/LottoDto.java

+public class LottoDto {
+    private final List<LottoNumber> lottoNumbers;
+
+    public LottoDto(List<LottoNumber> lottoNumbers) {


네네 (파라미터가 많은 경우) 코드도 깔끔해지고 어떤 객체의 데이터를 꺼내서 만든 DTO야, 라는 것이 생성자에서 드러나면 해당 DTO를 사용하는 클라이언트 입장에서도 이해하기 쉬울 것 같아요 :)

wbluke · 2022-03-03T00:18:19Z

src/main/java/view/InputView.java

+        return inputInteger();
+    }
+
+    public static List<List<Integer>> inputManualLottoNumbers(int manualLottoCount) throws IOException {


이런 이중 자료구조는 흐름을 해석하기가 쉽지 않게끔 하는데요.
DTO로 감싸보면 좋을 것 같아요 :)

wbluke · 2022-03-03T00:21:14Z

src/main/java/view/InputView.java

+        }
+        out.println("수동으로 구매할 번호를 입력해 주세요.");
+        List<List<Integer>> manualLottoNumbers = new ArrayList<>();
+        for (int i = 0; i < manualLottoCount; i++) {


저는 어떤 수가 주어졌을 때, 그 수만큼 반복하는 행위는 일반 for문으로 작성해요.
고전이라고 표현하셨지만 ㅎㅎ 고전이라기 보다는 가독성이 훨씬 나을 때가 많거든요 :)

wbluke · 2022-03-03T00:25:57Z

src/test/java/model/LottoOrderTest.java

+        // then
+        assertThatThrownBy(() -> new LottoOrder(purchasingMoney, manualCount))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessage(LottoOrder.TOO_MANY_MANUAL);


이런 부분 같은 경우 고민이 필요한데요.
사실 예외 메시지가 실수로 수정되어도 항상 통과하는 테스트가 되어버립니다.
특히나 해당 메시지가 사용자 단까지 흘러가는 아주 중요한 텍스트라면 더욱 이슈가 될 수 있겠죠.
그래서 이런 문자열 메시지 검증은 상수를 가져오기보다는 직접 "" 문구를 넣어서 테스트를 작성하면, 후에 실수로 텍스트가 변경되었다 하더라도 사전에 방지하는 효과를 얻을 수 있습니다 :)

Ohzzi · 2022-03-03T12:02:00Z

피드백 주신 내용들 반영해보았습니다!
사실 뭔가 계속 더 리팩토링 하려고 건드려야겠다는 생각이 지속적으로 드는데, 어느 부분에 손을 대야 할 지 모르겠더라고요...
우선은 그래서 피드백 내용과 제 눈에 걸리는 부분들만 더 리팩토링하고 마무리했습니다!

코드에서 더 지적해주실 부분이 있다면 얼마든지 지적해주세요 :)

Ohzzi · 2022-03-03T12:03:12Z

src/main/java/controller/LottoController.java

+    }
+
+    private List<Lotto> purchaseManualLotteries(LottoOrder lottoOrder) {
+        LottoFactory lottoFactory = LottoFactory.getInstance();


현재는 LottoFactory를 싱글톤으로 만들어서 유지하고 있는데, 사실상 유틸 클래스와 다름이 없다는 생각도 듭니다!
싱글톤 vs static 유틸 클래스 어느쪽으로 만드는 것이 더 좋은 선택일까요?

좋은 질문이네요.
자바에서 기본적으로 static은 안티 패턴입니다.
객체지향 패러다임의 장점을 사용할 수 없거든요.
게다가 LottoFactory는 도메인 모델이지 유틸 클래스가 아닙니다ㅎㅎ

그래서 설계하실 때는 최대한 정적 클래스를 지양하시고, 도메인 로직과 관련이 없지만 가공 등의 로직이 필요한 경우만 유틸 (정적) 클래스로 만드시는 게 좋습니다. (ex. String <-> LocalDate 변환을 돕는 LocalDateUtils)

Ohzzi · 2022-03-03T12:05:03Z

src/main/java/model/WinningStatistics.java

-    public int get(LottoRank lottoRank) {
-        return winningStatistics.get(lottoRank);
+    public Map<LottoRank, Integer> getWinningCounts() {
+        return Collections.unmodifiableMap(winningCounts);


지금은 생성자에서는 List나 Map 클래스의 copyOf로 Immutable한 collection을 참조를 끊어서 복사해 오고, getter에서는 원본 참조는 유지하되 unmodifiable이 되도록 변경해서 반환하고 있습니다. 그런데 getter에서도 copyOf로 통일하는 것이 나을까 라는 생각이 들어서 이 부분에 대해서도 닉의 의견을 듣고 싶습니다!

상황에 따라 맞게 쓰면 될 것 같은데요, 저는 불변 정도면 충분하다고 생각하긴 합니다 :)

wbluke

마지막까지 고생 많으셨어요 👍 크게 피드백 드릴 부분은 없네요.
미션 하시면서 아프셨다고 하셨는데 건강 조심하시고요ㅠㅠㅎㅎ
다음 미션도 화이팅입니다 🙂

Ohzzi added 13 commits February 28, 2022 16:38

refactor: 부적절한 메서드 및 변수 명 변경

c2a725a

refactor: 로또 숫자 풀의 자료구조를 List에서 Map으로 변경

18b9248

refactor: LottoFactory에서 LottoNumber의 pool을 사용하도록 수정

78a8ecf

refactor: validation 로직 위치 변경

6901251

refactor: 통계 출력 시 for 대신 foreach 사용

fd1dd79

fix: 수익률 계산 오류 수정

af74366

LottoPurchasingMoney가 로또 구매 금액(1000단위)만 남기게 하여 수익률이 작게 나오는 오류 수정

refactor: WinningStatistics 초기화 시 stream.foreach 대신 일반 for-each 사용하도록 변경

90fe99c

feat: 수동 구매 장 수, 자동 구매 장 수를 담는 로또 주문 추가

77537f7

feat: 수동 로또 번호를 담는 수동 주문 구현

fba05f3

test: 로또 주문 테스트 추가

2dd2b25

fix: 수동 로또 구매 수가 전체 구매 수를 넘었을 때 프로그램이 종료되는 오류 수정

d40e678

refactor: view로 도메인을 바로 넘기지 않고 DTO를 넘기도록 수정

8ec4b36

fix: 수동 구매 장 수가 없을 경우 불필요한 메시지 출력하지 않도록 수정

5b9054a

Ohzzi commented Mar 1, 2022

View reviewed changes

Ohzzi changed the base branch from main to ohzzi March 1, 2022 12:50

Ohzzi commented Mar 1, 2022

View reviewed changes

wbluke suggested changes Mar 3, 2022

View reviewed changes

Ohzzi added 6 commits March 3, 2022 14:42

refactor: DTO 생성시에 원래 객체를 받아서 생성하도록 수정

19dffa8

refactor: WinningStatistics가 생성시에 수익률까지 모두 계산하도록 수정

209840f

refactor: 테스트에서 에러 메시지 상수를 가져와서 사용하지 않고 문자열 직접 테스트

9ad9ef7

refactor: 이중 리스트를 DTO의 리스트로 변경

44f0a5e

refactor: WinningStatistics 생성자를 정적 팩터리 메서드로 변경

3feafd9

refactor: getter에서 unmodifiable 자료구조를 반환하도록 수정

9475824

Ohzzi commented Mar 3, 2022

View reviewed changes

refactor: 클래스 명과 필드 명 적절하게 수정

dd3506a

wbluke approved these changes Mar 4, 2022

View reviewed changes

wbluke merged commit 9fc5386 into woowacourse:ohzzi Mar 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 로또(수동)] 오찌(오지훈) 미션 제출합니다. #438

[2단계 - 로또(수동)] 오찌(오지훈) 미션 제출합니다. #438

Ohzzi commented Mar 1, 2022 •

edited

Loading

Ohzzi Mar 1, 2022

wbluke Mar 3, 2022

Ohzzi Mar 1, 2022

wbluke Mar 3, 2022

Ohzzi Mar 1, 2022

wbluke Mar 3, 2022

Ohzzi Mar 1, 2022

Ohzzi Mar 1, 2022

wbluke Mar 3, 2022

Ohzzi Mar 1, 2022

wbluke Mar 4, 2022

Ohzzi Mar 1, 2022

wbluke Mar 3, 2022

wbluke left a comment

wbluke Mar 2, 2022

wbluke Mar 3, 2022

wbluke Mar 3, 2022

wbluke Mar 3, 2022

wbluke Mar 3, 2022

wbluke Mar 3, 2022

wbluke Mar 3, 2022

wbluke Mar 3, 2022

wbluke Mar 3, 2022

Ohzzi commented Mar 3, 2022

Ohzzi Mar 3, 2022

wbluke Mar 4, 2022

Ohzzi Mar 3, 2022

wbluke Mar 4, 2022

wbluke left a comment

		@@ -27,7 +28,7 @@ private LottoNumber(int number) {

		public static LottoNumber valueOf(int number) {
		validateRange(number);

[2단계 - 로또(수동)] 오찌(오지훈) 미션 제출합니다. #438

[2단계 - 로또(수동)] 오찌(오지훈) 미션 제출합니다. #438

Conversation

Ohzzi commented Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbluke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ohzzi commented Mar 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbluke left a comment

Choose a reason for hiding this comment

Ohzzi commented Mar 1, 2022 •

edited

Loading