크리스마스 프로모션 코드리뷰 ❤︎ by jisu-om · Pull Request #3 · jisu-om/java-christmas6-jisu-om

jisu-om · 2023-11-16T05:59:54Z

아래는 제가 궁금한 점들 입니다.

이벤트 날짜 조건을 어떻게 일반화 할 수 있을지?

설계를 좀더 깔끔하게 해보고 싶습니다.
2-1) BenefitCalculator, EventFinder, BadgeGenerator 객체를 만들었다가 삭제하였습니다. 사용되는 곳이 한 곳이라 그냥 메서드로 만들어서 처리하였습니다.
2-2) 확장을 위해서는 이벤트의 조건과 혜택금액을 enum(EventDetail)으로 만들기보다 이벤트를 상속받는 각 이벤트를 객체로 만드는 것이 좋았을지 싶었습니다.

OutputView 에 정보를 전달하는 방법
ResultDto를 생성하여 outputView 에서 getter로 필요한 정보를 꺼낼 수 있도록 만들었습니다.

재참여율 목표를 5%로 설정했을까?
코드로 이런 점을 구현할 수 있을까?

…쉽도록 변경

… 으로 만들어 관리하도록 함

…생성, 이를 총괄하는 christmasEventManager 생성

…는 기능 구현

… 말고 다른 이름으로 변경

Arachneee

리뷰남기고 갑니다~

Arachneee · 2023-11-16T14:05:37Z

src/main/java/christmas/controller/OrdersInputHandler.java

+public class OrdersInputHandler {
+    private final InputView inputView;
+    private final OutputView outputView;
+
+    private OrdersInputHandler(InputView inputView, OutputView outputView) {
+        this.inputView = inputView;
+        this.outputView = outputView;
+    }


핸들러 클래스가 적절하게 사용된거 같아 인상적이네요. 핸들러를 사용하면서 Controller에 inputView가 사용안되는데 Controller가 inputView에 의존하지않고 핸들러에만 의존하는 건 어떨까요?

오 그러네요!!! @.@ 감사합니다 :)

두 핸들러 모두 같은 로직을 처리하고 있으니 이걸 인터페이스로 만들어서 상속할 수도 있지 않을까요!

Arachneee · 2023-11-16T14:20:18Z

src/main/java/christmas/domain/event/EventDetail.java

+    CHRISTMAS_D_DAY(
+            "크리스마스 디데이 할인",
+            date -> date >= EVENT_START && date <= CHRISTMAS_EVENT_END,
+            BASE_PRICE_CONDITION,
+            orders -> true,
+            (date, orders) -> BASE_DISCOUNT + calculatePassedDays(date) * CHRISTMAS_RATE),
+    WEEKDAYS(
+            "평일 할인",
+            date -> date >= EVENT_START && date <= CHRISTMAS_EVENT_END && (date % 7 >= 3 || date % 7 == 0),
+            BASE_PRICE_CONDITION,
+            orders -> orders.existsOrderItemByCategory(DESSERT),
+            (date, orders) -> orders.countOrderItemByCategory(DESSERT) * DAILY_RATE),
+    WEEKENDS(
+            "주말 할인",
+            date -> date >= EVENT_START && date <= EVENT_END && (date % 7 == 1 || date % 7 == 2),


date 객체를 전달하고 date 메소드로 로직을 넣는 것이 더 깔끔할 거 같아요

@Arachneee
findByCondition에서 VisitingDate , Orders 를 전달받아 해당되는 이벤트를 찾아내는데
Date 라는 객체를 새로 만들어서... 그 안에 date 관련 로직을 넣으라는 말씀이실까요?
이해가 잘 안되어서... 혹시 가능하다면 예시 코드를 보여주실 수 있으실까요? :)

Arachneee · 2023-11-16T14:28:32Z

src/main/java/christmas/constants/DiscountConstants.java

+    public static final long GIVE_AWAY_PRICE_CONDITION = 120_000L;
+    public static final long GIVE_AWAY_PRICE = MenuItem.샴페인.getPrice();
+    public static final String GIVE_AWAY_ITEM = MenuItem.샴페인.name();


샴페인 증정이 하드코딩된 느낌이 듭니다.

억 맞습니다

Constants를 Enum으로 관리하지 않고 final로 관리했을때
enum 대비 어떤 이점이 있을 수 있을까요?

Arachneee · 2023-11-16T14:30:30Z

src/main/java/christmas/domain/badge/BadgeCondition.java

+    STAR("별", 5_000L, 10_000L),
+    TREE("트리", 10_000L, 20_000L),
+    SANTA("산타", 20_000L, Long.MAX_VALUE),
+    NONE("없음", 0L, 5_000L);


maxCondition이 다음 타입의 minCondition이랑 같아서 중복되는 것 같습니다.

맞네요.. 이렇게 말고 다르게 처리하도록 바꿔야겠어요!! :)

Arachneee · 2023-11-16T14:34:05Z

src/main/java/christmas/validationUtils/OrdersValidationUtils.java

+public class OrdersValidationUtils {
+    private static final int MAXIMUM_ORDER_TOTAL_QUANTITY = 20;
+
+    public static void validateDuplicates(List<OrderItem> orders) {
+        Set<MenuItem> uniqueOrders = orders.stream()
+                .map(OrderItem::provideMenuItem)
+                .collect(Collectors.toSet());
+        if (orders.size() != uniqueOrders.size()) {
+            throw new IllegalArgumentException(INVALID_ORDERS.getMessage());
+        }
+    }


검증로직을 분리한 이유가 있으신가요?

@Arachneee
OrdersValidationUtils 와 OrderItemValidationUtils 로 나눈 이유를 말씀하실까요?
섞이는게 싫어서 분리했는데 지금와서 생각해보니 굳이 분리할 필요가 없었을 수도 있겠다 싶어요
OrderItem 은 Orders 를 구성하는(?) 객체이기도 해서 하나의 ValidationUtils 로 묶어도 되겠네요

junslog

지수님! 너무 깔끔하게 잘 짜신 코드라 생각합니다. 많이 배워가요 :)
코멘트 드릴게 없어서 몇번씩 보면서 쥐어 짜냈네요!
4주차까지 달려오느라 고생하셨고, 앞으로도 같이 성장해나가요ㅎㅎ

junslog · 2023-11-16T13:14:11Z

src/main/java/christmas/domain/badge/BadgeCondition.java

+public enum BadgeCondition {
+    STAR("별", 5_000L, 10_000L),
+    TREE("트리", 10_000L, 20_000L),
+    SANTA("산타", 20_000L, Long.MAX_VALUE),


👍👍👍 명시적으로 Long.MAX_VALUE 로 표현한게 인상깊어요

세번째 필드는 굳이 안써도 되었을 것 같아요ㅠㅠ ㅎㅎㅎ

junslog · 2023-11-16T13:16:42Z

src/main/java/christmas/domain/badge/BadgeCondition.java

+                .filter(badge ->
+                        totalBenefitAmount >= badge.getMinCondition() && totalBenefitAmount < badge.getMaxCondition())
+                .findFirst()
+                .orElse(NONE);


해당 로직에서는 음수인 값이( 입력 예외 상황 ) 들어오면 배지가 없음을 나타내는 식으로 구현하셨는데,
이 때 예외 값도 던져주고 호출하는 쪽에서 처리해줘도 좋았을 거 같아요!
물론 안정적이고 너무 좋은 코드라 생각합니다.

junslog · 2023-11-16T13:18:59Z

src/main/java/christmas/domain/event/EventDetail.java

+import java.util.function.Predicate;
+
+import static christmas.constants.DateConstants.*;
+import static christmas.constants.DiscountConstants.*;


지금은 큰 상관 없지만 와일드카드 import 는 지양하는게 어떨까요?

junslog · 2023-11-16T13:20:53Z

src/main/java/christmas/domain/event/EventDetail.java

+    public String getEventName() {
+        return eventName;
+    }
+}


이벤트를 이렇게 Enum으로 싹다 모아서 관리하니 너무 보기 편하고 응집되어 있는 코드 느낌이 나네요.
배워갑니다!

junslog · 2023-11-16T23:58:00Z

src/main/java/christmas/constants/DateConstants.java

+    public static final int EVENT_START = 1;
+    public static final int EVENT_END = 31;
+    public static final int CHRISTMAS_EVENT_END = 25;
+}


Enum으로 관리하지 않고 클래스로 관리한 이유가 궁금합니다!

junslog · 2023-11-17T00:05:02Z

src/main/java/christmas/dto/OrdersDto.java

+    }
+
+    public List<OrderItemDto> getOrderItemDtos() {
+        return List.copyOf(orderItemDtos);


이미 정적 팩토리 메서드 형식으로 생성될 때
.toList() 로 불변으로 만들어 두었는데, List.copyOf() 사용하는 이유가 있을까요?

junslog · 2023-11-17T00:13:07Z

src/main/java/christmas/validationUtils/OrdersValidationUtils.java

+    public static void validateDuplicates(List<OrderItem> orders) {
+        Set<MenuItem> uniqueOrders = orders.stream()
+                .map(OrderItem::provideMenuItem)
+                .collect(Collectors.toSet());


아래와 같이 불변 컬렉션으로 만드는 건 어떨까요?
.collect(Collectors.toUnmodifiableSet());

junslog · 2023-11-17T00:13:38Z

src/main/java/christmas/validationUtils/Parser.java

+    }
+
+    private static void validateEmpty(String input) {
+        if (input == null || input.isEmpty()) {


input이 null일 경우는 언제일까요?

junslog · 2023-11-17T00:14:33Z

src/main/java/christmas/view/OutputView.java

+    }
+
+    public void printResultStart(int date) {
+        System.out.printf((RESULT_SRART_FORMAT) + "%n", date);


'%n' 을 상수로 관리하는것도 좋을 것 같습니다.

junslog · 2023-11-17T00:17:50Z

src/test/java/christmas/domain/visitingDate/VisitingDateTest.java

+
+public class VisitingDateTest {
+    @DisplayName("1 이상 31 이하이면 방문 날짜 정상 생성된다.")
+    @Test


@valuesource 를 사용하시지 않은 이유가 있을까요?

kkonii

지수 님 고생많으셨어요!
그동안의 피드백들을 코드에 많이 녹여내신 게 보여요🔥
배울 점이 많았던 리뷰였습니다!

kkonii · 2023-11-16T23:12:24Z

src/main/java/christmas/controller/OrdersInputHandler.java

+public class OrdersInputHandler {
+    private final InputView inputView;
+    private final OutputView outputView;
+
+    private OrdersInputHandler(InputView inputView, OutputView outputView) {
+        this.inputView = inputView;
+        this.outputView = outputView;
+    }


두 핸들러 모두 같은 로직을 처리하고 있으니 이걸 인터페이스로 만들어서 상속할 수도 있지 않을까요!

kkonii · 2023-11-17T00:40:07Z

src/main/java/christmas/dto/OrderItemDto.java

+    public OrderItem toOrderItem() {
+        return OrderItem.of(menuName, quantity);
+    }


동사 기반의 이름 규칙!

kkonii · 2023-11-17T00:51:06Z

src/main/java/christmas/view/OutputViewMessageConstants.java

view 패키지로 분류하신 게 인상깊어요 :)

h-beeen · 2023-11-17T04:50:47Z

src/main/java/christmas/controller/ChristmasController.java

+import christmas.view.InputView;
+import christmas.view.OutputView;
+
+public class ChristmasController {


해당 컨트롤러에 모든 비즈니스 로직의 운영 책임이 몰려있군요!
그렇다면, 컨트롤러 Layer에서 단위테스트를 계획하게 된다면, 문제가 발생하지 않을까요?
단위테스트를 계획했지만, 실상 통합테스트 환경에서 테스트하게 되니까요!

h-beeen · 2023-11-17T04:51:29Z

src/main/java/christmas/controller/VisitingDateInputHandler.java

+    public VisitingDate createVisitingDate() {
+        while (true) {
+            try {
+                int input = inputView.readVisitingDate();
+                return VisitingDate.from(input);
+            } catch (IllegalArgumentException e) {
+                outputView.printError(e.getMessage());
+            }
+        }
+    }


공통된 ExceptionHandling 로직을 제네릭 또는 인터페이스로 추상화해서 활용해보면 좋을 것 같아요

h-beeen · 2023-11-17T04:52:49Z

src/main/java/christmas/domain/event/EventDetail.java

+            (date, orders) -> BASE_DISCOUNT + calculatePassedDays(date) * CHRISTMAS_RATE),
+    WEEKDAYS(
+            "평일 할인",
+            date -> date >= EVENT_START && date <= CHRISTMAS_EVENT_END && (date % 7 >= 3 || date % 7 == 0),


이 부분도 메소드로 한번 더 분리해주면 좋을 것 같아요.
가령 isWeekDay()라는 메소드로 분리해준다면,
해당 메소드의 기능을 직관적으로 파악할 수 있겠군요!

h-beeen · 2023-11-17T04:53:27Z

src/main/java/christmas/domain/event/EventDetail.java

+    private final String eventName;
+    private final Predicate<Integer> dateCondition;
+    private final long priceCondition;
+    private final Function<Orders, Boolean> itemCondition;
+    private final BiFunction<VisitingDate, Orders, Long> benefitCalculator;


멤버변수 5개..!

우아한테크코스 클린코드 컨벤션에서 3개 이상의 인스턴스 변수 사용을 금지하고 있습니다!
이 부분은 Enum 값 분리를 통해 책임을 분산시켜주면 좋을 것 같아요!

h-beeen · 2023-11-17T04:54:32Z

src/main/java/christmas/domain/orders/MenuItem.java

+    양송이수프(APPETIZER, 6_000L),
+    타파스(APPETIZER, 5_500L),
+    시저샐러드(APPETIZER, 8_000L),
+    티본스테이크(MAIN, 55_000L),
+    바비큐립(MAIN, 54_000L),
+    해산물파스타(MAIN, 35_000L),
+    크리스마스파스타(MAIN, 25_000L),
+    초코케이크(DESSERT, 15_000L),
+    아이스크림(DESSERT, 5_000L),
+    제로콜라(DRINK, 3_000L),
+    레드와인(DRINK, 60_000L),
+    샴페인(DRINK, 25_000L);


오버플로우를 고려하셔서 long Type으로 정의하신 걸까요?
20개가 최대 주문이기에, 오버플로우가 날 것이라 예견되지는 않지만,
추후 변화에 따른 사이드 이펙트에 대응하기에 적절한 설계인 것 같아 인상적이네요!

h-beeen

안녕하세요! 지수님!
4주간 정말 고생 많으셨습니다!
제 코드리뷰가 마지막 유종의 미를 거두는데,
조금이나마 도움이 되길 바라는 마음에
꼼꼼하게 코드리뷰를 진행했답니다 :)

4주간 너무 고생 많으셨고
추후 최종 코테에서 뵙길 바래요.

감사합니다 :)

… 찾도록 변경

…할인 적용기간 변경

… 변경, 증정이벤트 상품 개수 하드코딩 변경

jisu-om added 30 commits November 10, 2023 23:40

Docs: 구현할 기능 목록 작성

458390f

Docs: 구현할 기능 목록 수정

dda1629

Test(visitingDateTest): 방문날짜 생성 관련 테스트코드 작성

93d479d

Docs: README 수정

c4b3f57

Feat: 방문날짜(visitingDate 클래스) 생성

7348967

Test: 주문메뉴(orders) 테스트코드 작성

5740eca

Feat: 주문메뉴(orders)의 skeleton code 작성

2d8b947

Test: 주문메뉴 하나(order)에 대한 테스트코드 작성

f316079

Feat: orderItem, orders 구현, menu를 menuItem, menuCategory로 쪼개서 로직 수행이 …

2051bd8

…쉽도록 변경

Refactor: 주문메뉴(orders)에 대한 검증을 ordersValidationUtils에서 처리하도록 함

34278c2

Refactor: validationUtils로 검증로직 분리

d635d5a

Test: priceService 테스트코드 작성

f6128a0

Test: 혜택내역 처리하는 로직(eventService) 테스트코드 작성

2a2b914

Test: 이벤트 배지 생성하는 서비스(badgeService) 테스트코드 작성

5964d2a

Feat(eventCondition, eventNames, discountAmount): 이벤트 적용 조건, 혜택을 enum…

b846c84

… 으로 만들어 관리하도록 함

Refactor(discountAmount): 하드코딩된 부분 리팩토링

8a5315f

Rename: domain 패키지의 하위패키지 생성

8d32d1b

Feat (badgeCondition): 이벤트배지 관련 enum 생성

c72259d

Refactor (eventDetail): 이벤트 이름, 적용조건, 혜택을 eventDetail에서 관리하도록 리팩토링

28e6c7b

Feat(eventService): 적용되는 이벤트 찾는 기능 구현

4dd2354

Rename: dto 패키지의 위치 변경

6db824c

Refactor: eventService의 책임을 나눠갖는 eventFinder, eventBenefitCalculator …

2aa1540

…생성, 이를 총괄하는 christmasEventManager 생성

Feat(christmasController): 컨트롤러의 스켈레톤 코드 작성

8040843

Feat: inputView, outputView 스켈레톤코드 생성, outputView로 전달하는 resultDto 생성

4cbd860

Feat(badgeCondition, badgeGenerator): 이벤트 배지 정보를 반환하는 기능 구현

7b72f51

Feat: resultDto를 어떻게 생성할지 고민하며 구조 변경 시도

c843042

Feat(visitingDateInputHandler): 방문날짜를 입력받고 생성하는 기능 구현

d7ee9a5

Feat(ordersInputHandler, orderItemDto, inputView): 메뉴와 개수를 입력받고 예외처리하…

7478ce7

…는 기능 구현

Move: parser 위치 변경, 불필요한 import 제거

7104f84

Feat: 결과 시작 메세지를 출력하는 기능 구현

1980077

jisu-om added 13 commits November 15, 2023 02:30

Style: 코드 포맷 최적화

fd02ff8

Style(matchingEvents): 메서드 순서 변경

0673753

Refactor(matchingEvent): enum을 static import 하여 사용

e1cea10

Refactor(orders#provideOrderItems): 메서드 이름 수정, 깊은 복사로 수정

d2cb171

Refactor(orderItem): 인자명 수정

6b59e94

Refactor(errorMessage): 사용 안하는 에러메세지 삭제

34149ba

Test(ordersInputHandlerTest): 테스트코드 작성

e4bf21e

Test(visitingDateInputHandlerTest): 테스트코드 작성

5fcf9f6

Style: 불필요한 import 제거

2c82df4

Test: ui 로직 관련된 테스트 삭제

aa69771

Rename(orders): menuCategory, menuItem을 관련성이 높은 orders 하위로 옮김

6b09fbc

Refactor(visitingDate#provideDate): getter 말고 다른 이름으로 변경

40e798c

Refactor(matchingEvent#provideEventName,provideBenefitAmount): getter…

2c85a8f

… 말고 다른 이름으로 변경

jisu-om mentioned this pull request Nov 16, 2023

[크리스마스 프로모션] 전호건 리뷰부탁드립니다. Arachneee/java-christmas-6-Arachneee#1

Open

Arachneee reviewed Nov 16, 2023

View reviewed changes

junslog reviewed Nov 17, 2023

View reviewed changes

kkonii reviewed Nov 17, 2023

View reviewed changes

h-beeen reviewed Nov 17, 2023

View reviewed changes

jisu-om added 7 commits November 17, 2023 21:05

Refactor: inputHandler 삭제하고 supplier 사용해서 입력값 받아옴

3ed8499

Refactor(badgeCondition): maxCondition 필드 삭제, minCondition으로만 해당하는 배지…

7d4b1f6

… 찾도록 변경

Refactor(visitingDate, eventDetail): visitingDate가 객체로서 일하도록 리팩토링, 평일…

2516698

…할인 적용기간 변경

Refactor(benefitConstants): discountConstants -> benefitConstants로 이름…

bf119a1

… 변경, 증정이벤트 상품 개수 하드코딩 변경

Rename: constants 패키지를 domain 하위 패키지로 변경

917f3ee

Refactor: 에러 수정, resultDto의 필드 변경(할인후예상결제금액 추가)

8fdae14

Rename(utils): validationUtils에서 utils 로 패키지명 변경

4b9c148

Comments

Conversation

jisu-om commented Nov 16, 2023

Uh oh!

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

jisu-om Nov 16, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

h-beeen left a comment

Choose a reason for hiding this comment

Uh oh!

jisu-om Nov 16, 2023 •

edited

Loading