oereo 2차과제 제출합니다! #7

oereo · 2021-04-08T23:38:58Z

1차 과제를 진행하면서 리뷰도 많이 받았고 기존 구조와 naming 등이 이상하다고 판단이 되서 다시 새롭게 만들어보는게 좋겠다는 생각을 하였습니다!
과제 난이도가 점점 어려워지고 있다는 것이 확연히 느껴지는 것 같아요 ㅠㅠㅠㅠ
중간에 오류가 나서 오랫동안 해결이 안되서 애를 먹은 부분에 대해서는 따로 슬랙에 공유를 하도록 하겠습니다!

…6개를 리스트로 반환하는 method 구현

…on 처리

… 로 변경

…alidLottoTicketOfNumberException 를 던지는 test case 작성

…hod 구현

…ottoAutomaticTicket method 구현

…LottoProfit method 구현

Jummi10

수고하셨습니다!!! 사소한 것까지 모두 적다보니 리뷰가 너무 많아졌네요..

전체적으로 lotto 패키지에 있는 객체들은 모두 빈약하고, LottoStore와 LottoMachine이 모든 책임을 떠맡고 있네요.
LottoStore나 LottoMachine이 모든 것을 관리하게 해서 여러 기능들이 섞인 메서드나 클래스가 나오게 된 것 같아요. application이 돌아가는 순서대로 메서드나 클래스를 분리하지 않고, 객체와 그가 가진 로직별로 나누고 각 도메인의 상태를 이용하여 그들에게 책임을 부여하면 좋을 것 같습니다!

자율적인 객체 공동체 설계하기 이 또한 동민님이 1단계에서 공유해주신 자료인데 저는 도움이 많이 되어서 세훈님께도 공유드립니다 ㅎㅎ

Jummi10 · 2021-04-09T04:21:20Z

src/main/java/lotto/domain/PurchaseAmount.java

+    public PurchaseAmount(int purchaseAmount) {
+        PurchaseAmountValidation purchaseAmountValidation = new PurchaseAmountValidation();
+        this.purchaseAmount = purchaseAmount;
+        purchaseAmountValidation.checkPurchaseAmountNotPositive(this.purchaseAmount);
+    }
+
+    public int getPurchaseAmount() {
+        return purchaseAmount;
+    }


저도 로또 1단계 구현했을 때 세훈님과 같이 PurchaseAmount와 PurchaseAmountValidation을 분리했었는데요, 게터를 제외한 행위가 아예 없는 빈약한 도메인 모델이라는 피드백을 받고 이에 대해서 생각해보았습니다. PurchaseAmount가 아무런 비즈니스 로직도 가지고 있지 않고 Validate 로직도 하나밖에 존재하지 않으니 PurchaseAmount를 더 풍부하게 만들어주면 어떨까요

첨부 동민님이 공유해주셨던 자료인데 다른 좋은 글들도 많으니 더 찾아보시면 좋을 것 같아요

자료 감사합니다!!!
정미님의 말씀에 동의하는 바에요!! 근데 하나 질문이 있습니다!
validation class를 따로 두는 이유가 각각의 클래스에서 공통적으로 사용하는 Validation체크로직이 있을 때 사용될 수 있다고 생각을 했는데 만약에 이렇게 사용할 경우 좀 더 generic한 Validation class를 만드는게 좋을까요?? 아니면 각각의 도메인 모델 안에 넣는 것이 유지 보수 면이나 여러면을 봤을 때 더 효율적일까요?

추가적으로 위에 정미님의 댓글에 답변을 못달아서 ㅠㅠ 여기에 답니다!!
먼저 시험기간일텐데 ㅠㅠ 리뷰 정말 감사드립니다!!
공유해주신 자료를 읽어보고 구조를 다시 한번 짜야겠다는 생각이 드네요 ㅠ 너무 현실세계에 맞춰서 Class를 만들다 보니 빈약한 도메인모델들이 나오게 되고 method들이 하나의 클래스안에 너무 많이 배치가 된 것 같아요!

공통적으로 사용하는 Validation체크로직이 있을 때 사용될 수 있다고 생각을 했는데 만약에 이렇게 사용할 경우 좀 더 generic한 Validation class를 만드는게 좋을까요?? 아니면 각각의 도메인 모델 안에 넣는 것이 유지 보수 면이나 여러면을 봤을 때 더 효율적일까요?

음 저도 처음에는 일단 모든 검증 로직을 각자 사용되는 클래스 밖으로 빼내서 따로 구현을 했었는데요! 뭐가 더 낫다라고는 아직 공부를 하는 입장이기에 단언할 수는 없지만 저는 이 부분을 구현하며 어떻게 생각을 했냐하면

PurchaseAmount에서 유효한 값이 주입되었는지 확인해야 한다

상태가 0 이하임을 체크하는 로직은 구입금액(PurchaseAmount)에서밖에 쓰이지 않음

PurchaseAmount 내부에 자신의 상태를 이용하는 로직이 한 개도 없었다

그리고 purchaseAmount를 검증하는 로직은 단 하나뿐이다

checkPurchaseAmountNotPositive()를 PurchaseAmount 안으로 넣어주자

수동 생성되는 번호 6개, 지난주 당첨 번호 6개, 보너스 번호 1개는 모두 1~45 사이의 유효한 값인지 확인해야 한다

숫자의 범위를 검증해주는 공통 검증 로직을 만들어야겠다

validateRange()를 가진 클래스 만들고 여기저기서 필요할 때 가져다 써야겠다

라는 생각의 흐름으로 리팩토링을 진행했습니다

흠 이에 대해서 다른 분들의 의견도 궁금하긴 하네요! 🤔

흐음 class로 따로 빼둬야 하는지에 대한 이유가 다른 class에서 공통적으로 사용하는지에 대한 유무로 판단을 하신 거군요! 결국 그러면 validationRange() 라는 class도 좀 더 generic하게 그리고 다른 class에서 사용할 수 있도록 naming도 중요하겠군요!

넵넵 저는 그런 식으로 생각을 했는데 음 꼭 이렇게 해야 한다까지의 의견은 아니어서 오늘 멘토분들이나 다른 분들께 한 번 질문해보는 것도 좋을 것 같아요

Jummi10 · 2021-04-09T05:54:15Z

src/main/java/lotto/ui/Receiver.java

+    private List<Integer> convertStringToInt(List<String> lottoManualTicket) {
+        return lottoManualTicket.stream()
+                .map(Integer::parseInt)
+                .collect(Collectors.toList());
+    }
+
+    private List<String> splitInputDelimiter(String lottoManualTicket) {
+        return Arrays.asList(lottoManualTicket.split(DELIMITER));
+    }


input을 받아 application에서 사용할 수 있는 값으로 만들어주는 로직이 ui에 들어갔군요

문자열을 split하는 것은 비즈니스 로직이 아닐까요?

Jummi10 · 2021-04-09T06:03:48Z

src/main/java/lotto/domain/NumberOfLottoTicket.java

+        LottoTicketOfNumberValidation lottoTicketOfNumberValidation = new LottoTicketOfNumberValidation();
+        this.totalNumberOfLottoTicket = calculateLottoTicketOfNumber(purchaseAmount.getPurchaseAmount());
+        this.numberOfManualLottoTicket = purchaseManualLottoOfNumber;
+        this.automaticLottoTicketOfNumber = calculateAutomaticLottoTicketOfNumber(totalNumberOfLottoTicket, numberOfManualLottoTicket);
+        lottoTicketOfNumberValidation.checkLottoTicketOfNumber(this.totalNumberOfLottoTicket);


totalNumberOfLottoTicket을 계산하고 다른 값을 넣어주기 전에 바로 Validation.check()를 해주는게 검증의 의미가 있을 것 같아요

그 저번주에 코드진행으로 보여주신 것에서
this.purchasedAmount = purchaseAmount 다음에 validateNotPositive()를 넣으신 것을 예제로 보여주신 부분이 있는데 이것과 다른건가요?

음 아뇨 값을 주입한 후에 validate를 진행하는 순서에 대해 얘기한 것이 아니라

this.automaticLottoTicketOfNumber = calculateAutomaticLottoTicketOfNumber(totalNumberOfLottoTicket, numberOfManualLottoTicket); // line 16

에서 totalNumberOfLottoTicket이 사용되는데, 여기서 검증이 된 totalNumberOfLottoTicket가 사용되어야지 맞지 않나라는 말이었습니다!

this.totalNumberOfLottoTicket = totalNumberOfLottoTicket -> this.totalNumberOfLottoTicket 검증 -> automaticLottoTicketOfNumber 계산할 때 totalNumberOfLottoTicket 사용 이 순서요!

Jummi10 · 2021-04-09T06:04:44Z

src/main/java/lotto/domain/NumberOfLottoTicket.java

+    public int getLottoTicketPrice() {
+        return LOTTO_TICKET_PRICE;
+    }


static이면 getter가 아니라 public으로 열어두고 사용해도 될 것 같아요

Jummi10 · 2021-04-09T06:05:58Z

src/main/java/lotto/domain/LottoStore.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class LottoStore {


어플리케이션의 전체적인 흐름을 담당하는 클래스군요

Jummi10 · 2021-04-09T08:33:06Z

src/main/java/lotto/ui/Printer.java

+    }
+
+    public void printAllMatchedLottoResults(Map<WinningStatus, Integer> lottoPrices) {
+        List<WinningStatus> keySet = new ArrayList(lottoPrices.keySet());


Suggested change

List<WinningStatus> keySet = new ArrayList(lottoPrices.keySet());

List<WinningStatus> keySet = new ArrayList<>(lottoPrices.keySet());

Jummi10 · 2021-04-09T08:45:58Z

src/test/java/lotto/domain/ProfitTest.java

+        Boolean isProfit = profit.isProfit();
+
+        //then
+        assertThat(true).isEqualTo(isProfit);


assertThat(actual).isEqualTo(expected) 의 형식입니다
연산의 결과로 가져온 isProfit이 true이길 기대한다이므로 두 개의 위치가 바뀌어야겠네요

Jummi10 · 2021-04-09T08:47:59Z

src/test/java/lotto/LottoTest.java

+        List<Integer> expected = lottoAutomaticTicket.getLotto();
+        System.out.println(expected);
+        //then
+        assertThat(expected).isEqualTo(actual);


인자가 들어갈 위치에 커서를 두고 ctrl+p를 해보면 어떤 parameter가 들어가야할 자리인지 알려준답니다!

Jummi10 · 2021-04-09T08:51:27Z

src/test/java/lotto/LottoTest.java

+    @Test
+    void 로또_객체를_생성한다(){
+        //given
+        List<Integer> actual = new ArrayList<>(Arrays.asList(6,7,8,9,10,11));
+
+
+        //when
+        List<Integer> expected = lottoAutomaticTicket.getLotto();
+        System.out.println(expected);
+        //then
+        assertThat(expected).isEqualTo(actual);
+    }


객체의 생성 자체를 테스트하는 것이 의미가 있을까요,,?

Jummi10 · 2021-04-09T08:52:45Z

src/test/java/lotto/LottoTicketOfNumberTest.java

+        assertThatExceptionOfType(NotValidLottoTicketOfNumberException.class).
+                isThrownBy(() -> new NumberOfLottoTicket(purchaseAmount, manualLottoTicketOfNumber));


Suggested change

assertThatExceptionOfType(NotValidLottoTicketOfNumberException.class).

isThrownBy(() -> new NumberOfLottoTicket(purchaseAmount, manualLottoTicketOfNumber));

assertThatExceptionOfType(NotValidLottoTicketOfNumberException.class)

.isThrownBy(() -> new NumberOfLottoTicket(purchaseAmount, manualLottoTicketOfNumber));

…s 에서 체크할 수 있도록 구조 변경

begaonnuri

구현 잘 해주셨네요 👍🏻
몇가지 변경사항 남겼으니 확인해주시고 궁금한 점이 있으면 DM주세요 :)

적절한 객체로 위임하는 것에 대해 초점을 맞춰서 변경해보시면 좋을것 같습니다😊

begaonnuri · 2021-04-11T15:41:08Z

README.md

@@ -1,5 +1,21 @@
 # 🚀 로또 1단계 - 자동

+## 구현할 기능 목록


기능목록을 구현하면 - [x]로 채워주는게 더 좋을것 같아요!

begaonnuri · 2021-04-11T15:45:57Z

src/main/java/lotto/LottoApplication.java

+        LottoApplication app = new LottoApplication();
+        app.run();


Application이 Application을 생성해서 실행시키는 것이 어색하게 느껴지네요.
run()은 애플리케이션의 흐름을 제어하면서 로직을 진행시키는 것 같은데 이것을 따로 객체로 분리해보면 어떨까요?
MVC 패턴과 Controller의 역할에 대해 학습해보면 좋을것같아요.

https://m.blog.naver.com/jhc9639/220967034588

begaonnuri · 2021-04-11T15:50:29Z

src/main/java/lotto/LottoApplication.java

+        PurchaseAmount purchaseAmount = lottoStore.inputLottoPurchaseAmount();
+        int lottoManualTicketNumber = lottoStore.inputLottoManualTicketNumber();
+        LottoManualTickets lottoManualTickets = lottoStore.inputLottoManualTickets(lottoManualTicketNumber);
+        NumberOfLottoTicket numberOfLottoTicket = lottoStore.informNumberOfLottoTicket(purchaseAmount, lottoManualTicketNumber);
+        LottoAutomaticTickets lottoAutomaticTickets = lottoStore.informLottoAutomaticTickets(lottoManualTickets, numberOfLottoTicket);
+        LastWeekWinningLotto lastWeekWinningLotto = lottoStore.inputLastWeekWinningLotto();
+        LastWeekWinningBonusBall lastWeekWinningBonusBall = lottoStore.inputLastWeekWinningBonusBall();
+        lottoStore.informLottoStatistics(numberOfLottoTicket, lottoManualTickets, lottoAutomaticTickets, lastWeekWinningLotto, lastWeekWinningBonusBall);


다양한 객체에게 역할을 위임하고 값을 리턴 받으면서 로직이 진행되어야 하는데,
지금처럼 LottoStore에게서만 위임한다면 굳이 LottoApplication으로 값을 리턴받을 필요가 있을까요?

그리고 한 가지 객체(LottoStore)에게만 값을 요청한다는 것은 그 객체가 갖고 있는 책임이 많다는 것을 의미하기도 해요.
역할에 맞는 다양한 객체로 분리해보는건 어떨까요?

begaonnuri · 2021-04-11T15:52:38Z

src/main/java/lotto/domain/LottoStore.java

+    }
+
+    public LottoManualTickets inputLottoManualTickets(int lottoManualTicketNumber) {
+        ArrayList<LottoManualTicket> lottoManualTicketDummy = new ArrayList<>();


(이펙티브 자바) 규칙64. 객체는 인터페이스를 사용해 참조하라 를 참고해보세요!

begaonnuri · 2021-04-11T15:57:46Z