-
Notifications
You must be signed in to change notification settings - Fork 0
[재구현] PR #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: review
Are you sure you want to change the base?
[재구현] PR #4
Changes from all commits
8190efa
5c93916
1a7271e
d2b8c48
7227d56
b78cc7f
c6b758a
007277f
f33cfc4
9753ee2
6d563d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| # 구현할 기능 목록 | ||
|
|
||
| - [X] 방문날짜를 입력받고 생성한다. | ||
| - [X] 1 이상 31 이하 여야 한다. | ||
| - [X] 아닌 경우 IllegalArgumentException 발생시키고 입력 다시 받음 | ||
| - [X] 예외 메세지 : "[ERROR] 유효하지 않은 날짜입니다. 다시 입력해 주세요." | ||
|
|
||
|
|
||
| - [X] 주문 메뉴와 개수를 입력받고 해당 객체를 생성한다. | ||
| - [X] 예외 상황 - 예외 메세지 출력 "[ERROR] 유효하지 않은 주문입니다. 다시 입력해 주세요." | ||
| - [X] 고객이 메뉴판에 없는 메뉴를 입력하는 경우 | ||
| - [X] 메뉴의 개수는 1 이상의 숫자만 입력되도록 한다. | ||
| - [X] 메뉴 형식이 예시와 다른 경우 | ||
| - [X] 중복 메뉴를 입력한 경우(e.g. 시저샐러드-1,시저샐러드-1) | ||
| - [X] 음료만 주문하는 경우 | ||
| - [X] 메뉴 개수 총합 : 최대 20개 까지 가능. | ||
|
|
||
|
|
||
| - [X] 주문 메뉴 출력 | ||
|
|
||
|
|
||
| - [X] 이벤트 적용 | ||
| - [X] 총주문 금액 10000원 이상부터 적용 | ||
|
|
||
| - [X] 이벤트 적용 기간 | ||
| - 크리스마스 디데이 할인 12/1 ~ 12/25 | ||
| - 이외 이벤트 12/1 ~ 12/31 | ||
|
|
||
| - [X] 크리스마스 디데이 할인 | ||
| - 1,000원으로 시작하여 크리스마스가 다가올수록 날마다 할인 금액이 100원씩 증가 | ||
| - 총주문 금액에서 해당 금액만큼 할인 | ||
| (e.g. 시작일인 12월 1일에 1,000원, 2일에 1,100원, ..., 25일엔 3,400원 할인) | ||
|
|
||
| - [X] 평일 할인 | ||
| - 일~목 / 디저트 메뉴를 메뉴 1개당 2,023원 할인 | ||
|
|
||
| - [X] 주말 할인 | ||
| - 금,토 / 메인 메뉴를 메뉴 1개당 2,023원 할인 | ||
|
|
||
| - [X] 특별 할인 | ||
| - 3, 10, 17, 24, 25, 31 | ||
| - 총주문 금액 에서 1,000원 할인 | ||
|
|
||
| - [X] 증정 이벤트 | ||
| - 할인 전 총주문 금액이 12만 원 이상일 때, 샴페인 1개 증정 | ||
|
|
||
|
|
||
| - [X] 배지 계산 | ||
| - [X] 총혜택금액 기준 | ||
| - 산타 : 5_000원 이상 | ||
| - 트리 : 10_000원 이상 | ||
| - 별 : 20_000원 이상 | ||
|
|
||
|
|
||
| - [X] 아래 내용 출력 | ||
| - [X] 할인 전 총주문 금액 | ||
| - 형식 : "8,500원" | ||
| - [X] 증정 메뉴 | ||
| - "없음" || "샴페인 1개" | ||
| - [X] 혜택 내역 | ||
| - 형식 : "없음" || "크리스마스 디데이 할인: -1,200원" | ||
| - [X] 총혜택 금액 = 할인 금액의 합계 + 증정 메뉴의 가격 | ||
| - 형식 : "0원" || "-31,246원" | ||
| - [X] 할인 후 예상 결제 금액 = 할인 전 총주문 금액 - 할인 금액 | ||
| - 형식 : "8,500원" | ||
| - [X] 배지 출력 | ||
| - 형식 : "없음" || "산타" | ||
|
|
||
|
|
||
|
|
||
| --- | ||
| ### 전체 흐름 | ||
| ``` | ||
| 안녕하세요! 우테코 식당 12월 이벤트 플래너입니다. | ||
| 12월 중 식당 예상 방문 날짜는 언제인가요? (숫자만 입력해 주세요!) | ||
| 26 | ||
| 주문하실 메뉴를 메뉴와 개수를 알려 주세요. (e.g. 해산물파스타-2,레드와인-1,초코케이크-1) | ||
| 타파스-1,제로콜라-1 | ||
| 12월 26일에 우테코 식당에서 받을 이벤트 혜택 미리 보기! | ||
|
|
||
| <주문 메뉴> | ||
| 타파스 1개 | ||
| 제로콜라 1개 | ||
|
|
||
| <할인 전 총주문 금액> | ||
| 8,500원 | ||
|
|
||
| <증정 메뉴> | ||
| 없음 | ||
|
|
||
| <혜택 내역> | ||
| 없음 | ||
|
|
||
| <총혜택 금액> | ||
| 0원 | ||
|
|
||
| <할인 후 예상 결제 금액> | ||
| 8,500원 | ||
|
|
||
| <12월 이벤트 배지> | ||
| 없음 | ||
| ``` | ||
|
|
||
| ``` | ||
| 안녕하세요! 우테코 식당 12월 이벤트 플래너입니다. | ||
| 12월 중 식당 예상 방문 날짜는 언제인가요? (숫자만 입력해 주세요!) | ||
| 3 | ||
| 주문하실 메뉴를 메뉴와 개수를 알려 주세요. (e.g. 해산물파스타-2,레드와인-1,초코케이크-1) | ||
| 티본스테이크-1,바비큐립-1,초코케이크-2,제로콜라-1 | ||
| 12월 3일에 우테코 식당에서 받을 이벤트 혜택 미리 보기! | ||
|
|
||
| <주문 메뉴> | ||
| 티본스테이크 1개 | ||
| 바비큐립 1개 | ||
| 초코케이크 2개 | ||
| 제로콜라 1개 | ||
|
|
||
| <할인 전 총주문 금액> | ||
| 142,000원 | ||
|
|
||
| <증정 메뉴> | ||
| 샴페인 1개 | ||
|
|
||
| <혜택 내역> | ||
| 크리스마스 디데이 할인: -1,200원 | ||
| 평일 할인: -4,046원 | ||
| 특별 할인: -1,000원 | ||
| 증정 이벤트: -25,000원 | ||
|
|
||
| <총혜택 금액> | ||
| -31,246원 | ||
|
|
||
| <할인 후 예상 결제 금액> | ||
| 135,754원 | ||
|
|
||
| <12월 이벤트 배지> | ||
| 산타 | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,12 @@ | ||
| package christmas; | ||
|
|
||
| import camp.nextstep.edu.missionutils.Console; | ||
| import christmas.controller.MainController; | ||
|
|
||
| public class Application { | ||
| public static void main(String[] args) { | ||
| // TODO: 프로그램 구현 | ||
| MainController mainController = MainController.create(); | ||
| mainController.run(); | ||
| Console.close(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package christmas.constants; | ||
|
|
||
| import christmas.domain.menu.Menu; | ||
|
|
||
| public class BenefitConstants { | ||
jisu-om marked this conversation as resolved.
Show resolved
Hide resolved
jisu-om marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| public static final long ORDER_MINIMUM = 10000; | ||
| public static final long GIVE_AWAY_MINIMUM = 120000; | ||
| public static final long CHRISTMAS_D_DAY_BASE_BENEFIT = 1000; | ||
| public static final long CHRISTMAS_D_DAY_DAILY_BENEFIT = 100; | ||
| public static final long WEEKDAY_BENEFIT_UNIT = 2023; | ||
| public static final long WEEKEND_BENEFIT_UNIT = 2023; | ||
| public static final long SPECIAL_BENEFIT = 1000; | ||
| public static final Menu GIVE_AWAY_PRODUCT = Menu.샴페인; | ||
| public static final String GIVE_AWAY_PRODUCT_NAME = Menu.샴페인.name(); | ||
| public static final long GIVE_AWAY_BENEFIT_AMOUNT = GIVE_AWAY_PRODUCT.getPrice(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| package christmas.controller; | ||
|
|
||
| import christmas.domain.EventFinder; | ||
| import christmas.domain.event.MatchingEvents; | ||
| import christmas.dto.*; | ||
| import christmas.domain.orders.OrderItem; | ||
| import christmas.domain.orders.Orders; | ||
| import christmas.domain.visitingDate.VisitingDate; | ||
| import christmas.view.InputView; | ||
| import christmas.view.OutputView; | ||
|
|
||
| import java.util.List; | ||
| import java.util.function.Supplier; | ||
|
|
||
| public class MainController { | ||
| private final InputView inputView; | ||
| private final OutputView outputView; | ||
|
|
||
| private MainController(InputView inputView, OutputView outputView) { | ||
| this.inputView = inputView; | ||
| this.outputView = outputView; | ||
| } | ||
|
|
||
| public static MainController create() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 정적 팩터리 메서드를 쓰셨군요? 이렇게 구현했을 때, 어떤 장점이 있어서 이렇게 구현해주셨나요?
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 뷰는 필요한 데이터만 받아서 생성해주는 역할이라 객체가 매번 생성될 필요가 없다고 생각해서 싱글톤으로 만들었고, 어차피 외부에서 받아오는 매개변수가 필요하지 않은 지금과 같은 상황에서는 정적 팩터리 메서드를 사용하지 않아도 되었을 것 같다는 생각을 지금... 했습니다. 제 생각이 어떤지요..?? 준팍님(@junpakPark)의 생각이 궁금합니다!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋은 답변입니당 👍👍 |
||
| return new MainController(InputView.getInstance(), OutputView.getInstance()); | ||
| } | ||
|
|
||
| public void run() { | ||
| outputView.printStart(); | ||
| VisitingDate date = createVisitingDate(); | ||
| Orders orders = createOrders(); | ||
| outputView.printResultStart(); | ||
| OrdersDto ordersDto = OrdersDto.from(orders); | ||
| outputView.printOrderDetail(ordersDto); | ||
| MatchingEvents matchingEvents = EventFinder.findMatchingEvents(date, orders); | ||
| ResultDto resultDto = ResultDto.of(orders, matchingEvents); | ||
| outputView.printResult(resultDto); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. view 메시지가 출력될 떄마다 개행을 해주면 좀 더 읽기 쉬워질 것 같아요, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 개행해주셔두 좋구요, 아니면 View와 Model을 유의미한 단위로 묶어서 메서드 추출해주셔도 좋을 것 같네요 👍👍
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
시간이 없어서 이부분 리팩터링은 생각도 못하고 구현에만 집중했던 것 같습니다! |
||
| } | ||
|
|
||
| private VisitingDate createVisitingDate() { | ||
| return readUserInput(() -> { | ||
| int input = inputView.readVisitingDate(); | ||
| return VisitingDate.from(input); | ||
| }); | ||
| } | ||
|
|
||
| private Orders createOrders() { | ||
| return readUserInput(() -> { | ||
| List<OrderItemDto> orderItemDtos = inputView.readOrderItemDtos(); | ||
| List<OrderItem> orderItems = orderItemDtos.stream() | ||
| .map(dto -> OrderItem.of(dto.getName(), dto.getQuantity())) | ||
| .toList(); | ||
| return Orders.from(orderItems); | ||
| }); | ||
| } | ||
|
|
||
| private <T> T readUserInput(Supplier<T> supplier) { | ||
| while (true) { | ||
| try { | ||
| return supplier.get(); | ||
| } catch (IllegalArgumentException e) { | ||
| outputView.printError(e.getMessage()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| package christmas.domain; | ||
|
|
||
| import java.util.Arrays; | ||
|
|
||
| public enum Badge { | ||
| NONE(null, 0L, 5_000), | ||
jisu-om marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| STAR("별", 5_000, 10_000), | ||
| TREE("트리", 10_000, 20_000), | ||
| SANTA("산타", 20_000, Long.MAX_VALUE); | ||
|
|
||
| private final String badgeName; | ||
| private final long minimumBenefitAmount; | ||
| private final long maximumBenefitAmount; | ||
|
|
||
| Badge(String badgeName, long minimumBenefitAmount, long maximumBenefitAmount) { | ||
| this.badgeName = badgeName; | ||
| this.minimumBenefitAmount = minimumBenefitAmount; | ||
| this.maximumBenefitAmount = maximumBenefitAmount; | ||
| } | ||
|
|
||
| public static Badge findBadgeByCondition(long totalBenefitAmount) { | ||
| return Arrays.stream(Badge.values()) | ||
| .filter(badge -> totalBenefitAmount >= badge.getMinimumBenefitAmount() | ||
| && totalBenefitAmount < badge.getMaximumBenefitAmount()) | ||
| .findFirst() | ||
| .get(); | ||
jisu-om marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| public long getMinimumBenefitAmount() { | ||
| return minimumBenefitAmount; | ||
| } | ||
|
|
||
| public long getMaximumBenefitAmount() { | ||
| return maximumBenefitAmount; | ||
| } | ||
|
|
||
| public String getBadgeName() { | ||
| return badgeName; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package christmas.domain; | ||
|
|
||
| import christmas.domain.event.EventDetail; | ||
| import christmas.domain.event.MatchingEvent; | ||
| import christmas.domain.event.MatchingEvents; | ||
| import christmas.domain.orders.Orders; | ||
| import christmas.domain.visitingDate.VisitingDate; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| public class EventFinder { | ||
| private EventFinder() { | ||
| } | ||
| public static MatchingEvents findMatchingEvents(VisitingDate date, Orders orders) { | ||
| List<EventDetail> events = EventDetail.findEventByCondition(date, orders); | ||
| List<MatchingEvent> matchingEvents = events.stream() | ||
| .map(event -> MatchingEvent.of(event, event.calculateBenefitAmount(date, orders))) | ||
| .toList(); | ||
| return MatchingEvents.from(matchingEvents); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package christmas.domain.event; | ||
|
|
||
| import christmas.domain.orders.Orders; | ||
| import christmas.domain.visitingDate.VisitingDate; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.function.BiFunction; | ||
| import java.util.function.Predicate; | ||
|
|
||
| import static christmas.constants.BenefitConstants.*; | ||
| import static christmas.domain.menu.MenuType.DESSERT; | ||
| import static christmas.domain.menu.MenuType.MAIN; | ||
|
|
||
| public enum EventDetail { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 음... 솔직하게 말하면 이런 코드가 리뷰해드리기 제일 어려운 것 같습니다. 분명 Java8에 대해 학습을 많이 하셨고, 객체지향적인 설계가 잘 되었냐고 물으신다면 객체란 무엇일까요? 하지만 EventDetaild을 Enum으로 만들어주시면서 가장 중요한 비즈니스 로직이 객체로 만들어지지 못했습니다. 그 증거로 이런 경우엔 VisitingDate date, Orders orders 이 두 값을 인스턴스 변수로 가졌다면, SOLID 원칙 중엔 OCP (Open Closed Principle, 개방 폐쇄 원칙)이라는 것이 있습니다. 인터페이스를 통해 구현체를 만들어 주는 방식은 기존의 구현한 코드들은 수정하지 않고 유지 보수 측면에서도 모든 정책을 한꺼번에 파악해서 수정하는 경우보다는 사실 가독성 측면에서도 좋은 평가를 드리기 어려울 것 같습니다. 처음 해당 코드를 보는 사람은 쉽사리 이해하기 힘들 것 같습니다. 제한된 시간내에 구현하느라 어쩔 수 없으셨겠지만, 다형성을 고민하면서 설계하시면 더 좋은 코드를 쓸 수 있으실 것 같습니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jisu-om 저도 다른 PR에서 Enum 으로 비즈니스 로직을 고려한 코드를 리뷰할 때 @junpakPark 님과 같은 의견을 냈었습니다.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enum의 활용을 극대화해보자는 데에만 집중하여 본질(객체지향 설계)을 놓친 것 같습니다.. 정말 감사합니다!! 여러번 곱씹고 다시 구현해보겠습니다 |
||
| NONE(null, | ||
| date -> true, | ||
| orders -> orders.calculateTotalPrice() < ORDER_MINIMUM, | ||
| (date, orders) -> 0L), | ||
| CHRISTMAS_D_DAY("크리스마스 디데이 할인", | ||
| VisitingDate::isBeforeChristmas, | ||
| orders -> orders.calculateTotalPrice() >= ORDER_MINIMUM, | ||
| (date, orders) -> CHRISTMAS_D_DAY_BASE_BENEFIT | ||
| + date.getChristmasDDayBenefitDate() * CHRISTMAS_D_DAY_DAILY_BENEFIT), | ||
| WEEKDAY("평일 할인", | ||
| VisitingDate::isWeekday, | ||
| orders -> orders.calculateTotalPrice() >= ORDER_MINIMUM && orders.containsMenuType(DESSERT), | ||
| (date, orders) -> orders.countMenuType(DESSERT) * WEEKDAY_BENEFIT_UNIT), | ||
| WEEKEND("주말 할인", | ||
| VisitingDate::isWeekend, | ||
| orders -> orders.calculateTotalPrice() >= ORDER_MINIMUM && orders.containsMenuType(MAIN), | ||
| (date, orders) -> orders.countMenuType(MAIN) * WEEKEND_BENEFIT_UNIT), | ||
| SPECIAL("특별 할인", | ||
| VisitingDate::isSpecial, | ||
| orders -> orders.calculateTotalPrice() >= ORDER_MINIMUM, | ||
| (date, orders) -> SPECIAL_BENEFIT), | ||
| GIVE_AWAY("증정 이벤트", | ||
| date -> true, | ||
| orders -> orders.calculateTotalPrice() >= GIVE_AWAY_MINIMUM, | ||
| (date, orders) -> GIVE_AWAY_BENEFIT_AMOUNT); | ||
|
|
||
| private final String eventName; | ||
| private final Predicate<VisitingDate> dateCondition; | ||
| private final Predicate<Orders> ordersCondition; | ||
| private final BiFunction<VisitingDate, Orders, Long> benefitAmount; | ||
jisu-om marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| EventDetail(String eventName, Predicate<VisitingDate> dateCondition, Predicate<Orders> ordersCondition, | ||
| BiFunction<VisitingDate, Orders, Long> benefitAmount) { | ||
| this.eventName = eventName; | ||
| this.dateCondition = dateCondition; | ||
| this.ordersCondition = ordersCondition; | ||
| this.benefitAmount = benefitAmount; | ||
| } | ||
|
|
||
| public static List<EventDetail> findEventByCondition(VisitingDate date, Orders orders) { | ||
| return Arrays.stream(EventDetail.values()) | ||
| .filter(eventDetail -> eventDetail.dateCondition.test(date) && eventDetail.ordersCondition.test(orders)) | ||
| .toList(); | ||
| } | ||
|
|
||
| public long calculateBenefitAmount(VisitingDate date, Orders orders) { | ||
| return benefitAmount.apply(date, orders); | ||
| } | ||
jisu-om marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| public boolean isEqual(EventDetail event) { | ||
| return this == event; | ||
| } | ||
|
|
||
| public String getEventName() { | ||
| return eventName; | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.