[1단계 - 로또 구현] 기론(김규철) 미션 제출합니다. by Gyuchool · Pull Request #334 · woowacourse/java-lotto

Gyuchool · 2022-02-23T09:10:06Z

안녕하세요 핀!
우테코 4기 기론이라고 합니다!!😃😃

리뷰 잘 부탁드립니다. 미션 진행 중 궁금한 점은 커맨트로 남겨 두었습니다!

좋은 하루 보내세요. 감사합니다!

Gyuchool · 2022-02-23T09:14:00Z

src/main/java/Lotto.java

+    @Override
+    public String toString() {
+        String lotto = numbers.stream().map(String::valueOf)
+                .collect(Collectors.joining(", "));
+        return "[" + lotto + "]";
+    }


toString 매서드를 정의하여 로또들을 출력하는 함수를 만들지, 아니면 getLotto()처럼 get함수를 만들어서 OutputView에서 출력 형식을 만드는 게 좋을지 고민되었습니다.

OutputView 클래스가 출력을 담당하는 클래스라 OutputView에서 만들고 싶었는데 get()함수를 이용해서 정보를 가져오면 이중 포문으로 너무 복잡한 로직이 되지 않나 싶어서 일단은 toString으로 변환해서 가져왔는데
핀의 생각은 어떠한지 궁금합니다!

toString을 재정의한 목적에 따라 다를 것 같아요.
단순히 View를 그리기위해서면 저라면 toString을 재정의하진 않을 것 같아요.

nested-loop가 문제가 된다면 가장 간단한 방법은 메서드로 분리하기 입니다.

toString을 재정의한 목적을 듣고 찾아보았는데요. 🤨
제가 생각해본 결과로는 toString을 로그를 찍을 목적으로 사용하는게 좋다고 생각되었습니다.

view에서 요구하는 출력 결과물들과 로그를 찍을떄의 정보들이 다를 수가 있어서 toString으로는 로그를 찍을 때만 사용하고, view를 그리기 위한 출력은 view단에서 만들어서 보여주는 것이 좋을 것같다고 결론을 냈습니다.

혹시 핀은 이에 대해서 어떻게 생각하시는지 첨언해주실수 있으실까요?🥲

넵 저도 객체에 대한 정보를 확인하기 위한 용도로 재정의하면 될 것 같다는 생각입니다.

Gyuchool · 2022-02-23T13:08:31Z

src/test/java/domain/StatisticsTest.java

+        Statistic statistic = new Statistic(result);
+        Money money = new Money(10_000);
+        double realProfitRate = (Rank.FIFTH.getWinningPrice() + (Rank.FOURTH.getWinningPrice() * 2)) / (double) money.getMoney();
+        double profitRate = statistic.getProfitRate(money);
+
+        assertThat(profitRate).isEqualTo(realProfitRate);


안녕하세요 핀! 여기서 궁금한 점이 생겨서 질문드립니다!
기존에는 사진처럼 계산하기 쉬운 수익률로 테스트를 해서 해당 값을 하드코딩으로 넣었습니다!
사진처럼 코드를 작성하면 코드 줄이 짧고 계산하는 로직을 작성하지 않아도 되어서 빠르게 테스트를 돌릴 수 있었습니다.

하지만 이후 변화가 생기면 테스트 수익률을 다시 계산해서 테스트를 해야 한다고 생각되어서, 조금 복잡하더라도 직접 계산하는 로직을 짜고 값을 구한 후 테스트를 하는 것으로 바꿨습니다.

여기서 핀은 테스트 코드에서 실제 값을 구하기 위해 로직을 작성하는 것에 대해 어떻게 생각하시는지 궁금합니다!

지금은 간단하지만 나중에 복잡한 로직이 있으면 테스트 코드가 길고 복잡하게 되지 않을까 하고 생각되어서 질문 드립니다!
감사합니다

테스트하려는 대상이 반환하는 값과 기대하는 값이 똑같은 코드를 통해 계산 된다면 프로덕션 코드가 잘못 되어도 테스트가 통과되지 않을까요? 그런 이유로 저는 테스트 코드에 로직을 작성하지 않는 것을 선호해요.

lalize

안녕하세요 기론!
첫번째로 올라온 PR이네요! ⚡
작성하신 코드 확인해서 몇 가지 코멘트 남겨두었으니 고민해보시고 반영 부탁드릴게요!
추가로 궁금한 점 있으시면 코멘트나 DM 편하게 주세요 😃

lalize · 2022-02-24T00:11:34Z

src/main/java/LottoMachine.java

+import view.InputView;
+import view.OutputView;
+
+public class LottoMachine {


로또 머신이 컨트롤러 역할을 하고 있는데 내부 상태를 갖고 있네요.
멀티 스레드 환경에서 로또 머신을 사용하면 어떤 문제가 발생할 수 있을까요?

음🤔 상태를 가지고 있는 상태에서 다른 요청이 들어오면 중간에 값이 변경이 될 수 있겠네요..?

final을 추가하여 상수로 만들어준다.

상태를 갖는 변수들을 제거해준다.

이 두 가지 방법으로 스레드 세이프로 만들 수 있다고 생각되는데 괜찮을까요?

final을 추가하면 재할당은 안되지만 객체 내부 상태는 변경될 수 있지 않을까요?
그것도 같이 고민해서 내부 상태를 갖지 않도록 하면 될 것 같아요!
리팩토링 잘해주셨습니다 :)

lalize · 2022-02-24T00:12:10Z

src/main/java/domain/Lotto.java

+    public Lotto(List<Integer> numbers) {
+        this.numbers = numbers;
+    }


주 생성자를 통해 로또를 생성하는 경우 로또 번호에 대한 검증, 로또 번호 개수에 대한 검증이 누락되어 있어요.

외부에서 numbers 변경하면 로또 객체 내부 상태도 변하게 되네요 😭
로또 객체 내부의 상태를 안전하게 보장하려면 어떻게 해야할까요?

아래 글을 한번 읽어보시면 좋을 것 같아요!

방어적 복사와 Unmodifiable Collection

lalize · 2022-02-24T00:20:18Z

src/main/java/domain/Lotto.java

+    private static final int MIN_LOTTO_NUMBER = 1;
+    private static final int MAX_LOTTO_NUMBER = 45;
+    private static final int LOTTO_START = 0;
+    private static final int LOTTO_END = 6;
+    private static final String DELIMITER = ", ";
+    private static final String OPEN_BRACKETS = "[";
+    private static final String CLOSE_BRACKETS = "]";
+    private final List<Integer> numbers;


상수와 필드 사이 빈줄을 추가하면 가독성이 더 좋습니다.

Suggested change

private static final int MIN_LOTTO_NUMBER = 1;

private static final int MAX_LOTTO_NUMBER = 45;

private static final int LOTTO_START = 0;

private static final int LOTTO_END = 6;

private static final String DELIMITER = ", ";

private static final String OPEN_BRACKETS = "[";

private static final String CLOSE_BRACKETS = "]";

private final List<Integer> numbers;

private static final int MIN_LOTTO_NUMBER = 1;

private static final int MAX_LOTTO_NUMBER = 45;

private static final int LOTTO_START = 0;

private static final int LOTTO_END = 6;

private static final String DELIMITER = ", ";

private static final String OPEN_BRACKETS = "[";

private static final String CLOSE_BRACKETS = "]";

private final List<Integer> numbers;

lalize · 2022-02-24T10:45:12Z

src/main/java/domain/Lotto.java

+        this.numbers = numbers;
+    }
+
+    public static Lotto generateNumber() {


메서드명만으로 무엇을 하는지 알기가 쉽지 않네요.
조금 더 적절한 네이밍은 없을까요?

로또 번호 생성기로서 Lotto.generateNumber()로 읽을때 로또 번호 생성이라고 생각되어서 적절하다고 생각되어서 저렇게 지었었습니다.🥲

그렇다면 매서드 이름을 지을 때, 클래스 이름은 고려하지않고 매서드 이름으로만 의미가 분명하게 전달되도록 네이밍하는게 좋을까요?

우선 매서드 명을 generateLottoNumber로 로또번호생성한다라는 의미로 바꿔봤습니다!

lalize · 2022-02-24T10:46:02Z

src/main/java/domain/Lotto.java

+        Collections.sort(numbers);
+        return new Lotto(numbers);


해당 정적 팩토리 메서드로 로또를 생성하지 않으면 로또 번호가 정렬되지 않은 상태로 생성될 수도 있겠네요.
로또 객체가 생성될 때 항상 정렬된 상태를 보장하려면 어떻게 해야할까요?

생성자에서 정렬을 해서 반환하면 될 것같아요!
그런데 궁금한 점이 생겼습니다.🤔 핀이 정적 팩토리 메서드로 로또를 생성하지 않으면이라고 생각하시는 이유가 궁금합니다!
정적 팩토리 매서드가 객체 생성의 역할을 하는 클래스 메서드이고 생성자 대신해서 사용한다고 알고있습니다.
그래서 객체를 생성할때는 정적 팩토리 매서드를 거쳐서 생성되기 때문에 정적 팩토리 매서드를 통해서 생성하지 않는다는 것은 생각을 못해봐서 그렇습니다!

생성자가 public으로 열려 있기 때문에 다른 누군가가 해당 생성자를 이용해서 개발하는 경우는 어떻게 될까요?
객체의 제약사항, 정책과 같은 부분을 주 생성자에서 한번 처리하면 다른 생성자나 팩토리 메서드가 추가될 때 객체 내부 상태를 완전한 상태로 보장할 수 있습니다!

lalize · 2022-02-24T11:48:14Z

src/main/java/view/OutputView.java

+    public static void printStatistics(Statistic statistic) {
+        System.out.println(STATISTIC_RESULT_MESSAGE);
+
+        statistic.getStatistics().entrySet().stream().filter(statistics -> statistics.getKey().getCount() != ZERO).forEach(statistics -> {


코드 전반적으로 stream 메서드 체이닝이 너무 길어서 코드가 읽기 쉽지 않네요.
한 줄에 . 하나만 찍는 것은 어떨까요?

lalize · 2022-02-24T11:51:54Z

src/test/java/domain/LottoTest.java

+        Rank rank = fiveMatchLotto.match(winningNumber, bonusBall);
+
+        assertThat(rank).isEqualTo(Rank.SECOND);
+    }


모든 등수에 대해 테스트 해보면 좋을 것 같아요.

lalize · 2022-02-24T11:54:32Z

src/test/java/domain/MoneyTest.java

+
+public class MoneyTest {
+
+    @Test


1000원, 1001원, 1500원, 1999원 등 경계 값에 대해서도 테스트 해보면 좋을 것 같아요.

lalize · 2022-02-24T11:58:23Z

src/test/java/domain/MoneyTest.java

+
+    @Test
+    @DisplayName("1000원 미만이 입력되었을 때, 예외 발생 테스트 ")
+    public void inputUnder1000Test() {


999원도 테스트 해보면 좋을 것 같아요!
그리고 1000원, 1001원 등 여러 케이스에 대해 정상적으로 생성 되는지도 테스트 해봅시다~

lalize · 2022-02-24T12:05:01Z

src/test/java/domain/LottoTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class LottoTest {


Lotto뿐만 아니라 도메인 객체의 모든 public 메서드(단순 getter 등 은 제외)에 대해서 단위 테스트를 작성해보면 좋을 것 같아요!

lalize

기론! 리뷰 반영 잘해주셨어요 👍
몇 가지 코멘트 남겨두었으니 확인해주시고 다음 단계에 반영 부탁드릴게요!
1단계는 여기서 머지하겠습니다!
추가로 궁금한 점 있으시면 코멘트나 DM 편하게 주세요~

lalize · 2022-02-27T07:09:25Z

src/main/java/LottoMachine.java

+import view.InputView;
+import view.OutputView;
+
+public class LottoMachine {


final을 추가하면 재할당은 안되지만 객체 내부 상태는 변경될 수 있지 않을까요?
그것도 같이 고민해서 내부 상태를 갖지 않도록 하면 될 것 같아요!
리팩토링 잘해주셨습니다 :)

lalize · 2022-02-27T07:10:56Z

src/main/java/domain/Lotto.java

+    private static final int LOTTO_END = 6;
+    private static final int LOTTO_SIZE = 6;
+
+    private final List<LottoNumber> numbers;


List 보다 더 적합한 타입이 있지 않을까요?

set으로도 가능할 것 같네요! 그런데 set을 사용하면 set이 애초에 중복을 무시해버려서 중복이 발생했다 라는 에러 메시지를 내기 어렵더라구요.
그렇다고 List를 인수로 갖는 생성자를 하나 더 만들어서 사용하기엔 자료형에 맞는 검증 매서드를 또 만들어야해서 비효율적이라고 생각했습니다. 그래서 우선은 에러 메세지 자체를 개수가 불일치하다고 메세지를 보내도록 했는데, 이와 관련해서 힌트가 있을까요?

중복이 발생할 수 없는 구조에서 중복이라는 예외를 던질 필요가 있을까요? 기론이 생각해주신 것 처럼 로또 개수 사이즈에 대한 예외만 던지면 되지 않을까요?

lalize · 2022-02-27T07:11:58Z

src/main/java/domain/Lotto.java

+    public Lotto(List<LottoNumber> numbers) {
+        validateSize(numbers);
+        validateDuplicate(numbers);
+        Collections.sort(numbers);


외부의 numbers도 정렬이 될텐데 의도한 것일까요?

자료구조를 Set으로 바꾸면서 더이상 내부에서 정렬을 굳이 할 필요가 없어서 view에서 출력할때 정렬하도록 수정했습니다!

src/main/java/domain/Lotto.java

src/main/java/domain/Lottos.java

lalize · 2022-02-27T07:31:10Z

src/test/java/domain/LottoTest.java

+
+        assertThat(rank).isEqualTo(Rank.SIXTH);
+    }
+


지금도 좋지만 1~5등, 꽝 @ParameterizedTest로 테스트 해볼 수 있겠네요!

src/test/java/domain/LottoTest.java

lalize · 2022-02-27T07:33:29Z

src/test/java/domain/MoneyTest.java

+
+    @ParameterizedTest
+    @ValueSource(ints = {1000, 1001, 1500, 1999})
+    @DisplayName("로또 1개 생성되는지 테스트")


정말 로또 1개를 생성하는 테스트인가요?

로또를 생성하는 게 아니라 구매할 수 있는 로또 개수를 카운팅하는 테스트인데 명명이 잘못된것같네요!

src/main/java/domain/Lotto.java

src/main/java/domain/Money.java

src/main/java/domain/LottoNumber.java

Gyuchool added 30 commits February 22, 2022 14:19

docs: 기능 요구 사항 정리

3b1d183

feat: 구입 금액에 따른 생성할 수 있는 로또 개수 기능 구현

6189117

feat: 1000원 미만이 입력되었을 때, 예외 발생

b63f426

feat: 로또에서 일치하는 번호 개수 확인

46b7fb2

feat: 보너스 볼과 일치하는 번호가 있는지 테스트

978d1f1

feat: 구입 금액에 따른 개수만큼 로또 생성

80ed3a8

feat: 당첨 번호 통계 구현

54b006e

feat: 수익률 계산하는 기능 구현

baf7697

docs: 수익률 계산하는 기능 구현 완료

f39d4a0

feat: 구입 금액 입력 기능

226a6b7

feat: 지난주 당첨 번호 입력 기능

a73369c

feat: 보너스 볼 입력 기능

dc3fc06

feat: 입력 금액이 숫자인지 검증하는 기능

29e3bf2

feat: 로또 개수 출력 기능

f402da8

feat: 로또 번호 출력

29fc4af

docs: 로또 번호 출력 기능 구현

0a033db

feat: 지난주 당첨 번호 입력

4221ae1

feat: 6자리인 당첨 번호 입력 기능

2991cf2

feat: 당첨 번호에 보너스 볼과 중복되면 예외 발생

916f5e4

feat: 당첨 통계 출력

c69e49e

fix: 로또 번호 5개 출려에서 6개 출력으로 수정

ec75f80

refactor: 당첨 번호가 6자리가 아니면 예외 발생

9096506

feat: 수익률 출력

f32f2d1

feat: 당첨 번호 범위 검증 기능

d592acc

feat: 보너스 볼 범위 검증 추가

824f3a9

refactor: 맴버 변수 접근지정자 private으로 변경

8c874de

refactor: indent 1로 줄이기 및 매직넘버 제거

324a942

refactor: 자바 컨벤션에 맞게 수정

bda3916

refactor: 당첨 번호 범위 검증 로직 생성자에서 적용

ec0049d

feat: 당첨 번호에 중복된 번호가 있을 시 예외 발생

c0bde0f

Gyuchool added 6 commits February 23, 2022 22:00

feat: 테스트 코드에서 매직 넘버 제거

87453e4

refactor: 매직 넘버 상수화

1acf181

refactor: 매서드 이름 분명한 의미 갖도록 수정

fd18564

refactor: 적절한 반환 타입으로 수정

968fa5b

refactor: 원시값 포장

a4530f7

refactor: 자바 컨벤션에 맞게 수정

1df830d

Gyuchool commented Feb 24, 2022

View reviewed changes

lalize suggested changes Feb 24, 2022

View reviewed changes

Gyuchool added 17 commits February 25, 2022 15:22

refactor: Lotto view생성 로직을 toString에서 OuputView로 옮김

5b68885

refactor: 컨트롤러에서 내부 상태를 갖지 않도록 변경

6380be7

refactor: 방어적 복사로 변경

3bd58ce

refactor: 로또 번호에 대한 검증 추가

fb98ebd

refactor: 로또 번호 생성하는 매서드명 변경

eab308b

refactor: 로또가 생성될 떄 항상 정렬된 상태 보장하도록 생성자에서 sort 수행

9e6a793

refactor: 불필요한 생성자 제거

11bd1be

refactor: EnumMap으로 구현체 변경

ee30176

refactor: stream filter에서 anyMatch사용으로 전환

add8c6a

refactor: Integer변환 에러 발생시 try catch로 묶기

d3443b0

refactor: 로또 번호 생성시에 검증을 VO 객체에게 넘김

a2f23d1

refactor: 모든 등수에 따른 로또 당첨 테스트 추가

99d1fb9

test: 금액 입력 경계값 테스트 추가

c5dfa35

refactor: 경계값에 따른 테스트케이스 추가

2728a67

test: 도메인에 대한 테스트 추가

c644a68

fix: 당첨번호 최대 일치 개수 6개로 수정

703f35b

refactor: 방어적 복사로 반환 추가

91c1c1f

lalize approved these changes Feb 27, 2022

View reviewed changes

lalize reviewed Feb 27, 2022

View reviewed changes

src/main/java/domain/LottoNumber.java Show resolved Hide resolved

lalize reviewed Feb 27, 2022

View reviewed changes

src/main/java/domain/LottoNumber.java Show resolved Hide resolved

lalize merged commit 86b478a into woowacourse:gyuchool Feb 27, 2022

Gyuchool mentioned this pull request Mar 7, 2022

[로또 미션] 기론 1단계 및 최종 리뷰 정리 woowacourse-study/2022-back-end-code-review-study#9

Merged


		import static org.assertj.core.api.Assertions.assertThat;

		public class LottoTest {

Conversation

Gyuchool commented Feb 23, 2022

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!

lalize 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!

lalize 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees