step4 #3258

joooahn · 2023-05-24T14:32:36Z

리뷰 잘 보고있습니다 이번에도 잘 부탁드립니다

feat: 수동 로또 구현

dave915

안녕하세요 주안님! 4단계도 구현 잘 해주셨네요 👍
몇가지 피드백 남겼습니다. 확인 부탁드릴게요!

dave915 · 2023-05-26T00:43:23Z

src/main/java/lotto/view/InputView.java

+        return manualTickets;
+    }
+
+    private static List<LottoNo> splitAndMakeList(String input) {


이 메소드는 Ticket 으로 옮겨도 되지 않을까요?
Ticket 클래스에서 1,2,3,4,5,6 같은 String으로 Ticket 생성하는 메소드를 지원하면 좋을 것 같아요 :)

dave915 · 2023-05-26T00:47:44Z

src/main/java/lotto/domain/LottoTickets.java

+
+    public static LottoTickets of(LottoTickets manualTickets, LottoTickets autoTickets) {
+        List<Ticket> tickets = new ArrayList<>();
+        tickets.addAll(manualTickets.getTickets());


값을 꺼내지말고 manualTickets.marge(autoTickets) 같은 메소드를 만들면 어떨까요?

dave915 · 2023-05-26T00:48:23Z

src/main/java/lotto/domain/LottoTickets.java

@@ -22,12 +38,24 @@ public int totalPrice() {
        return tickets.size() * 1000;


1000 은 LOTTO_PRICE 변수를 사용하면 좋을 것 같아요!

dave915 · 2023-05-26T00:49:30Z

src/main/java/lotto/domain/LottoTickets.java

 import java.util.*;
 import java.util.stream.Collectors;

+import static lotto.domain.Ticket.LOTTO_PRICE;
+
 public class LottoTickets {


LottoTickets 의 책임이 점점 많아지고 있어요.
다른 객체로 책임을 분산 할 수 없나 고민해볼까요? 🤔

1. Ticket 클래스 팩토리 메서드 추가 : String을 받아서 Ticket을 반환해줌 2. 값을 꺼내지 않고 2개의 Ticket을 merge하는 메소드 추가 3. 매직넘버 제거 4. LottoTickets의 책임 일부를 WinningStatus, Prize 클래스로 분산

dave915

주안님 안녕하세요! 피드백 반영 확인했습니다!
몇가지 피드백 더 남겼습니다. 확인 부탁드릴게요 :)

dave915 · 2023-05-29T02:44:22Z

src/main/java/lotto/domain/Prize.java

+        Map<Prize, Integer> map = new HashMap<>();
+        Arrays.stream(Prize.values())
+                .forEach(prizeType -> map.put(prizeType, countPrizeType(prizeTypesOfTickets, prizeType)));
+        return map;


이렇게도 할수 있을 것 같아요 :)

Suggested change

return map;

return prizeTypesOfTickets.stream()

.collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));

와 이 부분을 어떻게 깔끔하게 처리할 수 없어서 고민이 많았는데 이렇게 간단한 방법이 있었다니.. 감사합니다
이렇게 좋은 stream api + lambda 같은 모던 자바에 익숙해지려면 어떻게 해야 할까요?

많이 써보는 수 밖에 없을 것같아요 😭
저는 Stream 사용할때 Collectors, Comparator 에 뭐가있는지 . 찍어서 한번씩 찾아보고 있습니다 ㅋㅋ
집계, 정렬 같은 부분은 DB 쿼리 기준으로 생각하면 왠만한 부분은 Stream에도 구현되어 있더라구요.

dave915 · 2023-05-29T02:47:36Z

src/main/java/lotto/domain/Ticket.java

+                .anyMatch(lottoNo -> lottoNo.equals(target));
+    }
+
+    private static List<LottoNo> splitAndMakeList(String input) {


이 메소드를 실행시키는 테스트가 없어보여요!

dave915 · 2023-05-29T02:49:47Z

src/main/java/lotto/domain/Ticket.java

+                .map(String::trim)
+                .map(Integer::parseInt)
+                .map(num -> {
+                    try {
+                        return LottoNo.from(num);
+                    } catch (TicketNumberOutOfBoundException e) {
+                        throw new RuntimeException(e);
+                    }
+                })


Lotto 에서 String 형태의 숫자를 받아서 Lotto를 생성해주면 어떨까요?
그리고, Stream 안에 try-catch 가 들어가면 가독성이 떨어지게 되는데요, 이부분은 어떻게 해결 할 수 있을까요 🤔

저도 lambda에서 try-catch가 복잡해질때가 고민이었는데 다음 문서 참고해 해결했습니다~ https://www.slipp.net/questions/572

dave915 · 2023-05-29T02:56:01Z

src/main/java/lotto/domain/LottoTickets.java

-                .filter(t -> t == prizeType)
-                .count();
+    private WinningStatus makeWinningStatus(List<Prize> prizeTypesOfTickets) {
+        return WinningStatus.from(prizeTypesOfTickets);
    }

    public double returnRate(WinningNumber winningNumber) {


지금 listOfPrize를 두번 만들고 있는데요, WinningStatus 만들때 totalPrice 만 같이 넘겨주면
WinningStatus 안에 있는 정보들로 returnRate도 만들어 줄 수 있을 것 같아요.

winningStatus(당첨 통계), returnRate(수익를)을 구할 땐 LottoTickets에게 WinningNumber만 주고 계산하도록 구현했어요
의미상 LottoTickets에게 당첨 번호를 주고 결과를 물어보는게 맞다고 생각해서입니다.

listOfPrice를 2번 사용한다면 listOfPrice가 수정될 경우 2군데 수정해야 한다는 단점이 있지만,
WinningStatus의 정보들로 returnRate를 만드려면 다음과 같이 파라미터를 통해 WinningStatus를 넘겨줘야 해 lottoTickets와 WinningStatus간의 새로운 의존관계가 생겨서 좋지 않다고 생각하는데 어떻게 생각하시나요?
lottoTickets.returnRate(winningStatus)

returnRate 메소드에서 winningNumber 는 List<Prize> types = listOfPrize(winningNumber); 를 구할때 사용 하고 있는것으로 보이는데요. WinningStatus 를 만들때 동일하게 listOfPrize(winningNumber); 의 결과를 이용 하는것으로 보여요.
그래서 WinningStatus의 필드인 winningStatus를 이용해서 충분히 같은 값을 구할 수 있다고 생각했습니다.

아래 LottoTickets 의 returnRate 메소드는

public double returnRate(WinningNumber winningNumber) { double result = (double) sumOfPrize(winningNumber) / totalPrice(); return Math.floor(result * 100) / 100; } private long sumOfPrize(WinningNumber winningNumber) { List<Prize> types = listOfPrize(winningNumber); return types.stream() .mapToLong(Prize::prize) .sum(); }

WinningStatus 에서는 아래 로직으로 대체 할 수 있을 것 같습니다.

public double returnRate() { double result = (double) sumOfPrize() / price; return Math.floor(result * 100) / 100; } private long sumOfPrize() { return winningStatus.entrySet().stream() .mapToLong(it -> it.getKey().prize() * it.getValue()) .sum(); }

1. groupingBy 사용 2. splitAndMakeList 테스트 작성 3. LottoNo 에서 String 형태의 숫자를 받아서 LottoNo를 생성 4. Stream 안의 try-catch 제거

dave915

주안님 피드백 잘 반영해주셨네요 💯
코멘트 주신 부분에 대한 답변과 몇가지 피드백 남겼습니다.
확인 부탁드릴게요! 😀

dave915 · 2023-05-30T15:36:17Z

src/main/java/lotto/domain/ThrowableFunctionWrapper.java

+    public static Function wrapper(ThrowableFunctionalInterface function) {
+        return t -> {
+            try {
+                return function.apply(t);
+            } catch (Exception e) {
+                throw new RuntimeException();
+            }
+        };
+    }


wrapper 에서도 제네릭 타입을 표시해주어야 할 것 같아요 :)
안그러면 경고가 뜨네요.

Suggested change

public static Function wrapper(ThrowableFunctionalInterface function) {

return t -> {

try {

return function.apply(t);

} catch (Exception e) {

throw new RuntimeException();

}

};

}

public static <T, R, E extends Exception> Function<T, R> wrapper(ThrowableFunctionalInterface<T, R, E> function) {

return t -> {

try {

return function.apply(t);

} catch (Exception e) {

throw new RuntimeException();

}

};

}

dave915 · 2023-05-30T15:36:23Z

src/main/java/lotto/domain/ThrowableFunctionWrapper.java

+package lotto.domain;
+import java.util.function.Function;
+
+class ThrowableFunctionWrapper {


dave915 · 2023-05-30T15:51:51Z

src/main/java/lotto/service/LottoGameService.java

+    public void buyTickets() throws TicketNumberOutOfBoundException, TicketPriceOutOfBoundException {
+        long money = InputView.inputMoney();
+        List<Ticket> manualTickets = InputView.inputManualTickets();
+        lottoTickets = LottoTickets.buyTickets(money, manualTickets);


현재 구조가 buyTickets 내부에서 buyManualTickets 와 buyAutoTickets 메소드를 호출 하고 있는것으로 보여요.
두 메소드는 지금 buyTickets 메소드 내에서 사용하는 것 이외에는 테스트에서만 사용하고 있는데 접근 제어자가 public 이더라구요.
테스트 하기 어려워서 public 으로 하신게 아닌가 생각이 드는데 맞을까요?

LottoGenerator를 인터페이스화 해서 아래처럼 구조를 바꿔보는건 어떠실까요? 🤔

LottoTickets menualLotto = LottoTickets.(money, 수동 LottoGenerator 구현체); LottoTickets autoLotto = LottoTickets.(menualLotto.remainMoney(money), 자동 LottoGenerator 구현체); LottoTickets lottoTickets = menualLotto.marge(autoLotto);

이 피드백은 필수는 아니고 해보시면 좋을 것 같아서 남겨둡니다.

feat: 수동 로또 구현

bf06145

feat: 수동 로또 구현

dave915 requested changes May 26, 2023

View reviewed changes

dave915 requested changes May 29, 2023

View reviewed changes

feat: 피드백 반영

8a038cf

1. groupingBy 사용 2. splitAndMakeList 테스트 작성 3. LottoNo 에서 String 형태의 숫자를 받아서 LottoNo를 생성 4. Stream 안의 try-catch 제거

dave915 requested changes May 30, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

step4 #3258

step4 #3258

joooahn commented May 24, 2023

dave915 left a comment

dave915 May 26, 2023

dave915 May 26, 2023

dave915 May 26, 2023

dave915 May 26, 2023

dave915 left a comment

dave915 May 29, 2023

joooahn May 29, 2023

dave915 May 30, 2023

dave915 May 29, 2023

dave915 May 29, 2023

joooahn May 29, 2023

dave915 May 29, 2023

joooahn May 29, 2023

dave915 May 30, 2023

dave915 left a comment

dave915 May 30, 2023

dave915 May 30, 2023

dave915 May 30, 2023

		@@ -22,12 +38,24 @@ public int totalPrice() {
		return tickets.size() * 1000;

	return map;
	return prizeTypesOfTickets.stream()
	.collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));

step4 #3258

Are you sure you want to change the base?

step4 #3258

Conversation

joooahn commented May 24, 2023

dave915 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

dave915 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

dave915 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