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

Gyuchool · 2022-03-02T02:32:39Z

안녕하세요 핀! 2단게에서는 1단계에서 남겨주신 피드백들 반영하고 구현 사항도 추가했습니다.
중간에 깃 rebase를 안하고 진행해서 해결하느라 시간이 쪼금 걸렸네요 😁

이번에도 잘 부탁드립니다!!

Gyuchool · 2022-03-02T03:29:08Z

src/main/java/domain/AutoLottos.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class AutoLottos extends Lottos {


상속으로 클래스 분리에 질문이 있습니다!
처음에는 AutoLottos, ManualLottos 클래스들을 안만들고 그냥 변수명으로만 구분했는데 예전에 일급컬렉션 공부할때
4. 이름이있는 컬렉션 이름이 있는 컬렉션을 사용하면 검색도 쉽고 명확한 의미도 전달할수 있다고 해서 적용해봤습니다.

그런데 막상 진행해보니 자동과 수동사이에 로또번호 생성이 자동인지 아닌지를 제외하고 전부 공통 로직이어서
과한 클래스 분리가 아닌가 싶은 생각이 들었습니다. 🙄
전부 공통 로직을 가지면 그냥 Lottos하나로 구현하는게 오히려 헷갈리지 않을 것 같더라구요!

혹시 핀의 생각은 어떠한지 알수있을까요?🧐

자동 로또, 수동 로또는 로또를 어떻게 생성할 것인가에 대한 부분이지 않나요? 정적 팩토리 메서드로 충분할 것 같아요!
상속은 is-a 관계이며 확장하는 개념일 때 사용해도 좋다고 생각하지만, 상속은 될 수 있으면 지양하는 게 좋습니다.

lalize

안녕하세요. 기론!!
리뷰가 조금 늦어졌네요 🙇‍♂️
몇 가지 코멘트 남겨두었으니 확인 부탁드릴게요~

lalize · 2022-03-03T08:48:35Z

src/main/java/domain/Lotto.java

-        Collections.shuffle(lottoRange);
-        List<LottoNumber> numbers = lottoRange.subList(LOTTO_START, LOTTO_END);
-        return new Lotto(numbers);
+    public Set<LottoNumber> getLotto() {


getNumbers가 더 적합한 것 같아요.

lalize · 2022-03-03T08:49:52Z

src/main/java/domain/LottoNumber.java

 public class LottoNumber implements Comparable<LottoNumber> {
    private static final int MIN_LOTTO_NUMBER = 1;
    private static final int MAX_LOTTO_NUMBER = 45;
    static final String INVALID_LOTTO_NUMBER_RANGE = "[ERROR] 로또 번호는 1부터 45 사이의 숫자여야 합니다.";


Suggested change

static final String INVALID_LOTTO_NUMBER_RANGE = "[ERROR] 로또 번호는 1부터 45 사이의 숫자여야 합니다.";

private static final String INVALID_LOTTO_NUMBER_RANGE = "[ERROR] 로또 번호는 1부터 45 사이의 숫자여야 합니다.";

lalize · 2022-03-03T08:51:59Z

src/main/java/domain/LottoNumber.java

+        if (Objects.isNull(lottoNumber)) {
+            lottoNumber = new LottoNumber(number);
+        }


해당 조건에 걸린다는 것은 1~45 이외의 값이 입력으로 들어온 경우이니 바로 예외를 던지는 건 어떨까요?

lalize · 2022-03-03T08:53:06Z

src/main/java/domain/Lottos.java

+        return Statistic.valueOf(ranks);
+    }
+
+    public static Lottos add(ManualLottos manualLottos, AutoLottos autoLottos) {


add라기 보단 concat에 가깝지 않나요?

lalize · 2022-03-03T08:54:59Z

src/main/java/domain/RandomNumbersGenerator.java

+    private static final int MIN_LOTTO_NUMBER = 1;
+    private static final int MAX_LOTTO_NUMBER = 45;
+    private static final int LOTTO_LENGTH = 6;


상수가 여러 클래스에서 중복으로 선언되어 있네요!

음..🤔 LOTTO_LENGTH 상수는 Lotto 클래스에서 가져올 수 있겠는데 MAX_LOTTO_NUMBER와 MIN_LOTTO_NUMBER는 LottoNumber클래스에서 가져오기 힘들 것 같더라고요.

그래서 중복된 상수들을 어떻게 처리할지 고민해봤는데 Enum타입으로 상수들을 관리하는 건 어떻게 생각하실까요?

LottoInfo와 같은 로또 관련 상수들을 관리하는 enum을 만들까 생각해봤습니다!

막상 enum으로 만들고 나니 오히려 코드에서 가독성이 떨어지는 것 같네요..😂

다시 고민해보는데 애초에 상수를 public으로 선언하는 건 어떻게 생각하시나요? public으로 선언하면 enum을 사용할때 처럼get()함수를 사용하지 않아 깔끔해 보이는 장점이 있는 것 같습니다. 사용하는 상수가 단순히 검증들을 위한 로또 시작 번호, 로또 끝 번호, 로또 길이 라고 생각되어 유의미한 값을 같는 Rank클래스와는 다르다고 생각되어 enum으로까지 관리하지는 않아도 되지않을까 라고 생각해봤습니다.

핀이 생각하시기엔 어느 것이 더 좋다고 생각하시나요..? 혹시...제가 생각한 게 핀이 생각하신 의도와 거리가 먼 걸까요..?

넵 필요하다면 상수를 public으로 선언하라는 의미로 드린 코멘트였습니다 ㅎㅎ;

lalize · 2022-03-03T09:00:18Z

src/main/java/view/OutputView.java


-    public static void printCountOfLotto(int count) {
-        System.out.printf(COUNT_MESSAGE, count);
+    public static void printCountOfLotto(int totalLottoCount, int manualLottoCount) {


자동도 계산해서 받아오는 건 어떨까요?

lalize · 2022-03-03T09:06:36Z

src/main/java/view/OutputView.java

-    private static final String COUNT_MESSAGE = "%d개를 구매했습니다.\n";
+    private static final String COUNT_MESSAGE = "수동으로 %d장, 자동으로 %d개를 구매했습니다.\n";
    private static final String STATISTIC_RESULT_MESSAGE = "당첨 통계\n---------";
    private static final String WINNING_MESSAGE = "%d개 일치 (%d원)- %d\n";


Suggested change

private static final String WINNING_MESSAGE = "%d개 일치 (%d원)- %d\n";

private static final String WINNING_MESSAGE = "%d개 일치 (%d원)- %d개\n";

lalize · 2022-03-03T09:15:12Z

src/main/java/domain/AutoLottos.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class AutoLottos extends Lottos {


자동 로또, 수동 로또는 로또를 어떻게 생성할 것인가에 대한 부분이지 않나요? 정적 팩토리 메서드로 충분할 것 같아요!
상속은 is-a 관계이며 확장하는 개념일 때 사용해도 좋다고 생각하지만, 상속은 될 수 있으면 지양하는 게 좋습니다.

lalize · 2022-03-03T09:20:53Z

src/main/java/view/InputView.java

+    public static int askManualLottoCount(int maxLottoCount) {
+        System.out.println(INPUT_MANUAL_LOTTO_COUNT);
+        int manualLottoCount = convertToInt(scanner.nextLine());
+        validateCountRange(maxLottoCount, manualLottoCount);


Money 객체에 수동 로또, 자동 로또 구매 개수에 대한 도메인 규칙을 넣을 수 있지 않을까요?

제가 핀의 리뷰 의도에 맞게 작성한건지 모르겠습니다만🥲 제 생각대로 적용해봤을 때, LottoMachine에서 코드가 이렇게 나왔습니다!

여기서 궁금한 점이 생겼는데, 제가 생각했을 때, Money가 수동과 자동 로또 개수를 관리하는게 객체 관점에선 맞아 보이는데 실제 서비스 플로우를 보시면 (제가 생각하기에는) 검증을 위해 money객체를 불러와서 로또 개수를 검증하는 것처럼 보여서 부자연스러워 보입니다.
그래서 LottoManage와 같은 클래스를 하나 만들어서 관리하는 건 어떻게 생각하시는지 핀의 의견이 궁금합니다!!

money.buyCount(manualLottoCount)와 같은 메시지를 보내서 수동 로또, 자동 로또 개수를 한번에 가져올 수 있지 않을까요?

lalize · 2022-03-03T09:29:25Z

src/main/java/domain/Lotto.java

-        int matchCount = getMatchCount(winningNumber);
-        boolean hasBonusBall = winningNumber.isBonusBallMatch(this);
-        return Rank.valueOf(matchCount, hasBonusBall);
+    public static Lotto generateLottoNumbers(LottoNumbersGenerator lottoNumberGenerator) {


LottoNumbersGenerator가 유의미하게 사용되고 있지는 않은 것 같아요.
외부에서 로또 번호를 선택해서 로또를 만들어줘도 되지 않을까요?

유의미하게 사용되고 있지 않다는 게 어떤 의미인지 좀 더 힌트 주실 수 있을까요?😂

혹시 로또가 생성될 때, 수동 로또 또는 자동 로또로 생성될 수 있으므로 로또번호들을 생성할 때,
수동이든 자동이든 이 매서드(generateLottoNumbers)를 거칠 수 있으므로
LottoNumberGenerator를 사용하면 자동 로또에 대해서만 한정적이므로 유의미하지 않다는 것일까요?

generateLottoNumbers는 LottoNumbersGenerator가 생성하는 값을 단순히 Lotto로 포장하여 반환하는 로직인데 generateLottoNumbers를 거쳐야 할까요? LottoNumbersGenerator가 Lotto를 반환하면 안되나요?

자동 로또뿐만 아니라 수동 로또도 동일한 인터페이스로 생성을 해야 LottoNumbersGenerator 인터페이스를 만든 의미가 있지 않을까요?

처음에 LottoNumberGenerator를 이용해서 자동 로또만 생성할 때 사용하고 수동 로또를 생성할 때는 정적 팩토리 매서드나 생성자를 이용하려고 헀는데 , 이처럼하면 로또 생성 매서드에 일관성이 없고 협업을 할 때는 헷갈릴만한 요소가 존재하게 되는 것 같네요..😲

lalize

기론! 리뷰 반영 잘해주셨어요~
인터페이스를 통해 수동 로또 생성, 자동 로또 생성 구현 코드 잘 작성해주셨어요.
다만 인터페이스 사용하는 데 있어서 개선할 부분이 있어 추가 코멘트 남겨두었고, 전체적인 구조 개선에 대한 코멘트도 같이 남겨두었으니 확인 부탁드릴게요!
조금만 더 힘내보아용 :)

src/main/java/domain/ManualLottoGenerator.java

src/main/java/domain/Money.java

src/main/java/LottoMachine.java

lalize · 2022-03-05T11:05:50Z

src/main/java/LottoMachine.java

 public class LottoMachine {
+    private static final String DELIMITER = ", ";

    public void start() {


실세계에서는 로또 구매 시점과 당첨 결과 시점이 다를텐데요!
로또 구매와 당첨 결과 각각 메서드를 분리하여 Application에서 각각의 메서드를 호출하는 방식으로 리팩토링 해보면 어떨까요?

콘솔 입력/출력이 아닌 웹 요청/응답과 같은 구조에서도 Controller를 그대로 사용할 수 있으면 좋을 것 같아요.
Controller에서 view 패키지 결합을 끊어보면 어떨까요?

질문이 있습니다!
MVC패턴에서 controller는 model과 view에 의존해도 된다고 알고 있는데 controller에서 view의 의존성을 제거하는 이유가 있을까요?
웹의 구조에서도 적용하려는 의도로 봐서 view단을 다른 걸로 갈아 끼워도 언제든 적용할 수 있도록 하기 위해서라고 생각되는데 이러한 생각이 맞을까요?

src/main/java/domain/Lottos.java

src/main/java/domain/AutoLottoGenerator.java

lalize · 2022-03-05T11:17:41Z

src/main/java/domain/AutoLottoGenerator.java

+        List<LottoNumber> numbers = IntStream.rangeClosed(MIN_LOTTO_NUMBER, MAX_LOTTO_NUMBER)
+                .mapToObj(LottoNumber::of)
+                .collect(Collectors.toList());


LottoNumber.values() 정적 메서드를 구현하여 이미 저장하고 있는 값을 반환하면 어떨까요?

LottoNumber.values()를 만들어서 저장한 값들을 반환하는 이유가 'AutoLottoGenerator' 클래스에서는
받은 값들로 로또를 만들기만 하기 위해서 책임을 옮기는 것이라고 봐도 될까요?

lalize · 2022-03-05T11:24:48Z

src/main/java/view/OutputView.java

@@ -7,9 +7,9 @@
 public class OutputView {


InputView가 도메인을 의존하지 않도록 리팩토링 잘해주셨어요 💯

OutputView도 마찬가지로 도메인을 의존하지 않도록 리팩토링 해주실 수 있으실까요?

lalize · 2022-03-05T11:27:22Z

src/test/java/domain/FixedNumbersGenerator.java

+
+import java.util.Set;
+
+public class FixedNumbersGenerator implements LottoGenerator {


Suggested change

public class FixedNumbersGenerator implements LottoGenerator {

public class FixedLottoGenerator implements LottoGenerator {

ManualLottoGenerator도 테스트 해주세요!

lalize

기론! 리뷰 반영 잘해주셨어요:)
아직 개선할 부분이 보여서 코멘트 남겨두었으니 참고 부탁드릴게요!

src/main/java/Application.java

lalize · 2022-03-06T10:14:40Z

src/main/java/Application.java

+
+        Statistic statistic = lottoMachine.winningResult(inputWinningNumber, inputBonusBall, lottos);
+        OutputView.printStatistics(StatisticDto.from(statistic));
+        OutputView.printProfitRate(statistic.getProfitRate(new Money(inputMoney)));


InputView, OutputView는 도메인 객체를 전혀 모르는 외부 영역(예를들면 프론트엔드 코드 영역), Application은 프론트엔드와 백엔드를 연결해주는 가상의 연결 통로라고 생각하시고 미션을 진행하시면 더 좋은 연습이 될 것 같아요:)

넵! 하면서 Application을 어떻게 활용해야 할지 고민했었는데 덕분에 이해가 잘 된 것 같습니다!😯
좋은 말씀 감사합니다!

그렇다면 Application에서는 dto로 변환 매서드는 안 쓰도록 해야 하는 게 맞나요?
controller에서 반환할 때, 애초에 dto로 변환하도록 해야겠네요!

src/main/java/controller/LottoController.java

lalize · 2022-03-06T10:16:06Z

src/main/java/controller/LottoController.java

+        return lottoGenerators;
+    }
+
+    public Statistic winningResult(String[] inputWinningNumber, int inputBonusBall, Lottos lottos) {


StatisticDto를 반환해야하지 않나요?

src/main/java/domain/LottoNumber.java

src/main/java/view/OutputView.java

lalize · 2022-03-06T10:28:25Z

src/main/java/view/OutputView.java

    private static final String DELIMITER = ", ";
    private static final String OPEN_BRACKETS = "[";
    private static final String CLOSE_BRACKETS = "]";


상수명으로 의도가 제대로 드러나지 않는 것 같아요.

DELIMITER는 로또 번호를 구분하는데 사용하고 있으니 LOTTO_NUMBER_DELIMITER와 같이 네이밍하면 어떨까요?
다른 부분도 마찬가지로 의도를 드러내는 네이밍으로 변경해주세요.

src/main/java/domain/Lotto.java

lalize · 2022-03-06T10:50:34Z

docs/README.md

+    - [x] 중복된 번호가 있으면, 예외 발생

 ### 로또



코멘트가 안달려 여기다가 답변해둘게요.

Q. #444 (comment)
기론이 생각하신 부분이 맞고, 더 나아가서 view 패키지를 완전히 독립적인 다른 애플리케이션(프론트엔드)라고 생각하고 의존성을 끊어내보면 더 많이 고민하고 연습이 되지 않을까 싶은 의도가 있었습니다!

Q. #444 (comment)
LottoNumber를 의존하고 있고 LottoNumber가 캐싱하고 있기 때문에 메서드로 제공하여 중복을 제거하면 좋겠다는 의도로 드린 코멘트입니다:)

Gyuchool · 2022-03-07T07:24:21Z

src/main/java/controller/dto/LottosDto.java

+    public int size(){
+        return lottos.size();
+    }


dto는 정보를 운반만 하는 간단한 객체인 것으로 알고 있는데 이렇게 작은 매서드 하나정도 만들어서 사용해도 되나요? 아니면 그냥 없는 게 나을까요?
처음에는 LottosDto.getLottos().size()이런식으로 끌고가니 체인이 너무 길어진다고 생각돼서 dto에서 크기를 반환해주는 size()매서드를 만들면 어떨까 생각해보았는데 핀은 어떻게 생각하시나요?

추가적으로 dto의 위치에 대해서 핀의 생각이 궁금합니다!
저는 dto를 공부할때 자주 사용되는 위치에 디렉토리를 두는게 패키지 의존성(?) 적어서 관리하기 좋다라고 알고있어서 dto들을 반환하는 controller가 있는 클래스에 두었습니다.
핀이 생각하시기에는 이번 미션에서는 어느 곳에 두어 관리하는게 좋다고 생각하시나요?

저는 없는 게 낫다고 생각합니다. 허용하다 보면 dto에 하나 둘 로직이 생기게 되고, dto가 무거워집니다...
json같은 형식으로 직렬화할 때 size가 직렬화 대상이라면 로직을 수행하기 때문에 성능적으로도 부담이 돼요.
정말로 필요하다면 내부 필드로 size를 갖게하고 dto는 단순히 저장된 값을 반환하도록 할 것 같아요.

dto를 의존하는 가장 낮은 레이어에 위치하도록 할 것 같아요.
기론이 작성해주신 애플리케이션에서는 지금처럼 controller 레이어에 위치하는 게 적절해보여요.

lalize

안녕하세요 기론~
리뷰 반영 잘 해주셨어요 👍
몇 가지 코멘트 남겨두었으니 확인 부탁드리겠습니다!

lalize · 2022-03-07T08:24:07Z

src/main/java/controller/dto/StatisticDto.java

+
+public class StatisticDto {
+    private final Map<RankDto, Integer> statisticDto;
+    private final Statistic statistic;


마찬가지로 도메인 객체를 필드로 가지고 있어야 할 게 아니라 전달할 데이터를 계산해서 담아야 하지 않을까요?

lalize · 2022-03-07T08:46:11Z

src/main/java/controller/LottoController.java

+        Money money = new Money(inputMoney);
+        int autoLottoCount = money.getAutoLottoCount(manualLottoCount);
+        List<LottoGenerator> lottoGenerators = getLottoGenerators(manualLottoNumbers, autoLottoCount);
+        return LottosDto.from(Lottos.generateLottos(lottoGenerators));
+


컨트롤러에서 구입 금액을 생성하고, 자동 로또 개수를 구하고, 로또를 생성하는 비즈니스 로직이 존재하는데요!
LottoService를 만들어 책임을 위임해보는 건 어떨까요?

당첨 결과를 구하는 것도 마찬가지 입니다!

lalize · 2022-03-07T08:47:39Z

src/main/java/controller/LottoController.java

+        return lottoGenerators;
+    }
+
+    public StatisticDto winningResult(String[] inputWinningNumber, int inputBonusBall, Lottos lottos) {


View가 도메인에 의존하고 있는 구조인 것 같아요.
Lottos도 로또 구입할 때 처럼 List<String[]> lottos를 받아서 서비스에서 Lottos를 생성해야 하지 않을까요?

lalize · 2022-03-07T08:48:47Z

src/main/java/controller/dto/LottosDto.java

+
+public class LottosDto {
+    private final List<LottoDto> lottoDtos;
+    private final Lottos lottos;


View에서 도메인 로직을 수행하기위해 Dto에 도메인 필드로 가지고 있네요. 결국 View가 도메인을 의존하고 있는 게 아닌가요?

dto내에 있는 Lottos는 view에서 사용 안되고 controller를 통해 도메인을 넘겨줄 때, 사용하려고 받아둔 것이었는데
dDto가 도메인을 가지고 view에서 dto내부의 도메인을 사용하지 않아도 의존하는 것으로 보는군요! 😱
새롭게 알아갑니다 😲

여기서 말씀하신 것처럼 LottoService를 만들어서 사용하니 도메인을 dto내부에 가지고 있을 필요가 없었네요!

lalize · 2022-03-07T09:00:26Z

src/main/java/controller/dto/LottosDto.java

+    public int size(){
+        return lottos.size();
+    }


저는 없는 게 낫다고 생각합니다. 허용하다 보면 dto에 하나 둘 로직이 생기게 되고, dto가 무거워집니다...
json같은 형식으로 직렬화할 때 size가 직렬화 대상이라면 로직을 수행하기 때문에 성능적으로도 부담이 돼요.
정말로 필요하다면 내부 필드로 size를 갖게하고 dto는 단순히 저장된 값을 반환하도록 할 것 같아요.

lalize · 2022-03-07T09:02:16Z

src/main/java/controller/dto/LottosDto.java

+    public int size(){
+        return lottos.size();
+    }


dto를 의존하는 가장 낮은 레이어에 위치하도록 할 것 같아요.
기론이 작성해주신 애플리케이션에서는 지금처럼 controller 레이어에 위치하는 게 적절해보여요.

src/main/java/domain/Lottos.java

src/main/java/Application.java

lalize

기론! 리뷰 반영 확인했습니다!
의도와 다르게 반영된 부분이 있어서 추가 코멘트 드렸으니 고민해보시면 좋을 것 같아요.
이번 미션은 여기서 머지하도록 할게요~
고생하셨습니다! 👍

lalize · 2022-03-08T02:33:10Z

src/main/java/Application.java

+        int inputBonusBall = InputView.askInputBonusBall();
+
+        StatisticDto statisticDto = lottoController.winningResult(inputWinningNumber, inputBonusBall,
+                manualLottoNumbers, inputMoney);


수동 로또 번호만 입력하고 있어요 😭

lalize · 2022-03-08T02:34:29Z

src/main/java/controller/LottoController.java

+        Money money = lottoService.createMoney(inputMoney);
+        int autoLottoCount = lottoService.getAutoLottoCount(money, manualLottoCount);
+        Lottos lottos = lottoService.generateLottos(manualLottoNumbers, autoLottoCount);


서비스 로직이 응집도가 낮은 것 같아요.

로또 구입 기능을 제공하는 서비스가 되어야 하지 않을까요?
lottoService.purchase(inputMoney, manualLottoNumbers);

그렇네요 서비스에서 모든 것을 처리하고 결과만 반환하니 더욱 깔끔해진 것 같습니다!

여기서 서비스 로직이 응집도가 낮다는 것은 어떤 이유인지 알 수 있을까요?
제가 공부했던 바로는 클래스 내에서 필드로 선언된 맴버 변수가 클래스 내부의 각 매서드에서 모두 사용될수록 응집도가 높은 것으로 알고 있었습니다.
그런데 service는 상태를 가지고 있으면 멀티 스레드환경에서 동시성 이슈가 생길수 있으므로 갖고 있지 않도록 했고, 응집도도 높은 상태가 되는 줄 알았어서 질문 드립니다!

혹시 구매를 한다는 (크게보면)하나의 비즈니스 로직이 service에서 각각 흩어져서 호출하기 떄문에 응집도가 낮다고 보면 될까요?

lalize · 2022-03-08T02:35:50Z

src/main/java/controller/LottoController.java

+        this.lottoService = lottoService;
+    }
+
+    public LottosDto purchase(int inputMoney, int manualLottoCount, List<String[]> manualLottoNumbers) {


로또 구입할 때 구입 금액과 선택한 로또 번호만 주면 구입할 수 있지 않나요?

lalize · 2022-03-08T02:37:23Z

src/main/java/controller/LottoController.java

+
+    public StatisticDto winningResult(String[] inputWinningNumber,
+                                      int inputBonusBall,
+                                      List<String[]> manualLottoNumbers,


당첨 결과를 확인하는 모든 로또를 입력으로 받아야 하는 거 아닌가요!?

Suggested change

List<String[]> manualLottoNumbers,

List<String[]> lottos,

아예 로또들 Dto를 받도록 수정했습니다! List<String[]>으로 하려면 만들어진 Lottos를 다시 돌면서 List<String[]> 에 맞는 형으로 바꿔야 할것 같아서 Dto로 넘겼습니다.

그런데 이렇게하니 LottosDto가 많이 무거워진 것 같아서 어떻게 해야 할지 고민입니다..

lalize · 2022-03-08T02:40:12Z

src/main/java/controller/LottoController.java

+        WinningLotto winningLotto = lottoService.generateWinningLotto(
+                new ManualLottoGenerator(inputWinningNumber),
+                inputBonusBall);
+
+        Lottos lottos = lottoService.generateLottos(manualLottoNumbers, 0);
+        Statistic winningStatistics = lottos.getWinningStatistics(winningLotto);
+        Money money = lottoService.createMoney(inputMoney);
+        return StatisticDto.from(winningStatistics, winningStatistics.getProfitRate(money));


이 부분도 마찬가지로 서비스 로직의 응집도가 낮은 것 같습니다!
컨트롤러에서 비즈니스 로직을 구성하는 형태가 되었어요.

lalize · 2022-03-08T02:49:38Z

src/main/java/domain/Statistic.java

+        initStatistics();
+        for (Rank rank : ranks) {
+            statistics.put(rank, statistics.get(rank) + 1);
+        }


객체 생성 비용이 너무 커지니 가능하면 생성자에서는 로직은 처리하지 않는게 좋습니다!

정적 팩토리 메서드를 사용하여, 계산을 통해 생성한다는 의도를 드러내는 게 좋습니다~

lalize · 2022-03-08T02:50:01Z

src/main/java/domain/Statistic.java

+    public static Statistic valueOf(List<Rank> ranks) {
+        return new Statistic(ranks);


해당 정적 팩토리 메서드에서 계산하여 생성자를 호출하면 좋겠네요!

lalize · 2022-03-08T02:50:59Z

src/main/java/service/LottoService.java

+    public Money createMoney(int value) {
+        return new Money(value);
+    }
+
+    public int getAutoLottoCount(Money money, int manualLottoCount) {
+        return money.getAutoLottoCount(manualLottoCount);
+    }
+
+    public Lottos generateLottos(List<String[]> manualLottoNumbers, int autoLottoCount) {
+        List<LottoGenerator> lottoGenerators = manualLottoNumbers.stream()
+                .map(ManualLottoGenerator::new)
+                .collect(Collectors.toList());
+        IntStream.range(0, autoLottoCount)
+                .mapToObj(i -> new AutoLottoGenerator())
+                .forEach(lottoGenerators::add);
+        return Lottos.generateLottos(lottoGenerators);
+    }


로또 구입을 위한 구현을 숨기고 로또 구입이라는 하나의 메서드를 제공해주세요.

lalize · 2022-03-08T02:52:23Z

src/main/java/service/LottoService.java

+    public WinningLotto generateWinningLotto(LottoGenerator lottoGenerator, int inputBonusBall) {
+        Lotto lotto = lottoGenerator.generateLotto();
+        LottoNumber bonusBall = LottoNumber.of(inputBonusBall);
+
+        return new WinningLotto(lotto, bonusBall);
+    }


해당 부분도 마찬가지 입니다.
당첨 결과 메서드를 제공해주세요.

lalize · 2022-03-08T02:54:46Z

src/test/java/domain/LottoTest.java

-        Lotto lotto = Lotto.generateLottoNumber(1, 6);
-        Lotto actual = new Lotto(LottoNumberGenerator.of(1, 2, 3, 4, 5, 6));
+        Set<LottoNumber> lottoNumbers = LottoNumberGenerator.of(1, 2, 3, 4, 5, 6);
+        Lotto lotto = new FixedLottoGenerator(lottoNumbers).generateLotto();


FixedLottoGenerator를 테스트할 필요가 있을까요?

Gyuchool added 17 commits March 2, 2022 11:22

refactor: 로또 숫자들을 생성한다는 명확한 의미로 매서드 이름 변환

873a9f4

refactor: Money class VO객체로 전환

1210aad

refactor: LottoNumber에 캐싱 적용

d823a1f

Lotto의 맴버 변수 자려구조 Set으로 변경 및 랜덤 생성 매서드 인터페이스 적용

9ea144d

refactor: winningLotto에서 당첨 로또 확인하도록 수정 및 Statistic 생성과 동시에 초기화

b9dfe71

docs: 기능 요구 사항 추가

8116b94

feat: 수동으로 구매할 로또 수 입력

15432f7

feat: 로또 수 입력시, 자연수가 아니면 예외 발생

a23c181

feat: 검증 로직 적용

7497f85

feat: 수동으로 구매할 로또 번호 입력

a79cbb9

feat: 수동 로또와 자동 로또 합쳐서 결과 계산

2bc1eec

refactor: 자동로또와 수동로또 클래스 생성

d41a985

test: 수동로또와 자동로또가 합쳐서 하나의 로또로 되는지 테스트

069301c

test: 로또 번호 정상인 경우 테스트

62b2a54

refactor: @ParameterizedTest로 로또 등수 테스트

b4c3ec5

refactor: 테스트 이름이 명확하도록 수정

f17eaa4

docs: lotto 상속을 통해 예외처리 사항 업데이트

291d619

Gyuchool commented Mar 2, 2022

View reviewed changes

lalize suggested changes Mar 3, 2022

View reviewed changes

Gyuchool added 11 commits March 3, 2022 20:50

refactor: 상속으로 구현한 Lottos 해제

fa9501a

refactor: numbers 반환하는 매서드 이름 getNumbers로 수정

cee97de

refactor: 1부터 45사이의 숫자가 아니면 예외 발생 추가

449a848

refactor: 로또 개수 계산하는 매서드에서 자동 로또 개수를 인자로 받도록 변경

f66270d

refactor: money객체에서 자동, 수동 로또 개수 검증하기

71d6380

refactor: 에러 메세지 상수화

85e822b

refactor: 로또 번호들을 받음으로써 유의미한 매서드로 변환

7366195

refactor: 중복된 상수 제거

b69e4e9

refactor: 매서드 의미가 명확하도록 네이밍 변경

4d67be0

refactor: money객체에서 자동 로또 개수 검증하도록 책임 이전

94361e3

refactor: 수동 로또 생성기 인터페이스 추가

cc117ce

refactor: view에서 객체 생성 로직 제거

83a01c7

lalize suggested changes Mar 5, 2022

View reviewed changes

Gyuchool added 10 commits March 6, 2022 10:19

refactor: Money에서 로또 개수 상태 제거

6a8eefa

refactor: controller 패키지 정리

9875d20

refactor: 상수 정책에 맞게 이동

4764515

refactor: controller에서 view 의존성 제거

6680172

refactor: List<LottoGenerator>를 이용한 구조 개선

35ab131

refactor: 방어적 복사로 수정

43d82c9

refactor: 정적팩토리 네이밍 변경

dd4362c

test: 수동 로또 테스트 추가

a38645c

refactor: AutoLottoGenerator에서 로또 번호 생성 책임 이전

0287e78

refactor: view에서 도메인 의존성 제거

fb575e0

lalize suggested changes Mar 6, 2022

View reviewed changes

Gyuchool added 8 commits March 6, 2022 23:18

refactor: stream 사용 및 변수명 수정

70a69f2

refactor: convention에 맞게 수정

9cbc911

refactor: 중복된 로직 제거

46417a0

refactor: controller에서 Lottos생성하도록 수정

6faef49

refactor: controller에서 Lottos 생성하도록 수정

4987375

refactor: dto에서 collectingAndThen 매서드 적용

f37b823

refactor: 상수명 네이밍 수정

7519d44

refactor: 로또 구매할때, LottosDto를 반환하도록 수정

c89bff0

Gyuchool commented Mar 7, 2022

View reviewed changes

lalize suggested changes Mar 7, 2022

View reviewed changes

Gyuchool added 4 commits March 7, 2022 22:50

refactor: statisticDto내부에 도메인 제거

4574bf5

refactor: collectingAndThen사용으로 한번에 dto반환하도록 수정

f538071

refactor: LottoService 생성을 통한 구조 개선

ac1cb12

refactor: Money 도메인 의존성 제거

c9a174b

Gyuchool mentioned this pull request Mar 7, 2022

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

Merged

lalize approved these changes Mar 8, 2022

View reviewed changes

lalize merged commit 1c35a4e into woowacourse:gyuchool Mar 8, 2022

	static final String INVALID_LOTTO_NUMBER_RANGE = "[ERROR] 로또 번호는 1부터 45 사이의 숫자여야 합니다.";
	private static final String INVALID_LOTTO_NUMBER_RANGE = "[ERROR] 로또 번호는 1부터 45 사이의 숫자여야 합니다.";

	private static final String WINNING_MESSAGE = "%d개 일치 (%d원)- %d\n";
	private static final String WINNING_MESSAGE = "%d개 일치 (%d원)- %d개\n";


		import java.util.Set;

		public class FixedNumbersGenerator implements LottoGenerator {

	public class FixedNumbersGenerator implements LottoGenerator {
	public class FixedLottoGenerator implements LottoGenerator {

		- [x] 중복된 번호가 있으면, 예외 발생

		### 로또

		public static Statistic valueOf(List<Rank> ranks) {
		return new Statistic(ranks);

Comments

Conversation

Gyuchool commented Mar 2, 2022

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!

Gyuchool Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Gyuchool Mar 3, 2022 •

edited

Loading