-
Notifications
You must be signed in to change notification settings - Fork 0
[크리스마스 프로모션] 코드리뷰용 Pull Request #1
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?
Changes from all commits
cfb3c72
d7f9b86
61aa904
ba4d32b
31990b9
c3cafdf
851a890
f941f33
ef06b19
221e83b
0e47ef0
5fbe307
a9aca2e
d864f3c
00f7558
c904585
4d89124
47ee9c2
863c565
49830ad
b1616ac
245b009
6e68e48
55ba4f0
937363f
5745b52
4d81ca6
fa00100
f011be9
8f75c94
add32f1
c2e1fe8
71152b8
87333c1
143d6da
1e257cd
a381715
c218e24
60c4395
2430ccf
87a82ae
7b7d2fa
e2b3449
106f652
8eb9fca
80b5feb
948ccbd
4c03076
9b76e0d
f043836
1113983
16d37bd
1da8219
b37453a
8e776f6
b05d593
a5f6302
b991761
dbd05d4
0ad1b1a
6fe1558
2dde13c
3fd1824
dbc6031
3c7847b
b17acb1
f587dcf
8608a60
81231c9
c01933d
44b0835
336dd75
a257e23
75fd7dc
e7944ad
e46096b
c4805f3
039a1b1
1899ecf
f48428a
0fd035a
d97bae5
ce6e107
3f98a76
c8ba2ef
22557d6
067626c
3571d80
6acfacf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,11 @@ | ||
| package christmas; | ||
|
|
||
| import christmas.view.input.InputView; | ||
| import christmas.view.output.OutputView; | ||
|
|
||
| public class Application { | ||
| public static void main(String[] args) { | ||
| // TODO: 프로그램 구현 | ||
| EventPlanner eventPlanner = new EventPlanner(new InputView(), new OutputView()); | ||
| eventPlanner.execute(); | ||
|
Comment on lines
+8
to
+9
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. EventPlanner 객체를 컨트롤러 성향이 아닌, main과 같은 레벨로 두신 점이 인상깊었습니다. Application.main과 EventPlanner를 구분하신 이유가 있을까요?
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. Application은 최상단의 클래스로, EventPlanner는 말 그대로 플래너 서비스 그 자체입니다. 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. @junslog 추후에 어떤 서비스가 어떻게 추가되는 걸 상정하셨나요?
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.
이벤트 결과를 다 보여주고 난 뒤에 주문 데이터를 받아와서 주문으로 연결해주는 서비스를 예시로 들 수 있을 거 같습니다만 말씀 듣고 고민해보니 '확장성'이라는 가능성을 열어두면서 구현하는건 좋으나, 확장성을 핑계로 구조를 틀어버린 느낌..?? 사실 누군가 제 코드에 대한 질문을 할 때, 좋은 개발은 뭘까?를 생각을 더 해보게 되는 말씀이었습니다. 감사합니다! |
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| package christmas; | ||
|
|
||
| import camp.nextstep.edu.missionutils.Console; | ||
| import christmas.controller.OrdersController; | ||
| import christmas.controller.ReservationDayController; | ||
| import christmas.domain.EventManager; | ||
| import christmas.domain.Orders; | ||
| import christmas.domain.ReservationDay; | ||
| import christmas.service.EventManagerService; | ||
| import christmas.view.input.InputView; | ||
| import christmas.view.output.OutputView; | ||
|
|
||
| public class EventPlanner { | ||
| private final OutputView outputView; | ||
| private final ReservationDayController reservationDayController; | ||
| private final OrdersController ordersController; | ||
| private final EventManagerService eventManagerService; | ||
|
|
||
| public EventPlanner(InputView inputView, OutputView outputView) { | ||
|
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.
inputView, outtputView는 생성비용이 많이 든다 생각하여 싱글톤으로 구현해야한다 생각했고, |
||
| this.outputView = outputView; | ||
| reservationDayController = new ReservationDayController(inputView, outputView); | ||
| ordersController = new OrdersController(inputView, outputView); | ||
|
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. 컨트롤러를 두 개를 쓰신 이유가 궁금합니다. 그리고 두 개의 컨트롤러가 하나의 인풋뷰를 관리하는 것이 조금 낯선데, 이렇게 구성하신 이유가 있나요? 그리고 제 개인적인 생각으로는 이 EventPlanner가 제일 컨트롤러 역할에 가까운 느낌입니다.
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. 저는 말씀해주신대로 EventPlanner 라는 클래스가 메인 컨트롤러의 역할을 하고, 물론 이 흐름대로면, 하나의 InputView를 관리하는 것이 아닌 각 도메인마다 뷰가 달라야하겠지만, 입력받는 Input값의 수가 많은 편이 아니고, 보여줘야할 데이터는 많지만 각 서브 도메인들의 협업으로 처리한 결과물 ( 이벤트 결과 )을 보여주는 로직이 많다 생각해서 |
||
| eventManagerService = new EventManagerService(); | ||
| outputView.printGreetingMessage(); | ||
|
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. execute() 메서드에서 바로 예약날짜와 주문을 입력받는 코드를 보여줘서 가독성을 높이는 것이 의도였는데, 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. 굿굿 생성자에서 빼주시는게 더 좋을 것 같습니다. |
||
| } | ||
|
Comment on lines
+19
to
+25
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. execute() 부분의 코드를 간결하게 짜고 싶은 욕심에 또한, 의존성 주입 방법 정말 괜찮은 것 같습니다. |
||
|
|
||
| public void execute() { | ||
| ReservationDay reservationDay = reservationDayController.insertReservationDay(); | ||
| Orders orders = ordersController.insertOrders(); | ||
| EventManager eventManager = EventManager.of(reservationDay, orders); | ||
| printResult(reservationDay, orders, eventManager); | ||
| terminatePlanner(); | ||
|
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. execute 메서드 안에 terminatePlanner() 로직이 들어있는게 이해가 가는데
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 void printResult(ReservationDay reservationDay, Orders orders, EventManager eventManager) { | ||
| printIntroMessage(reservationDay); | ||
| printOrderedMenus(orders); | ||
| printTotalAmountWithNoDiscount(orders); | ||
| printGiftMenu(orders); | ||
| printBenefitsDetails(eventManager); | ||
| printTotalBenefitedAmount(eventManager); | ||
| printEstimatedAmountWithDiscount(eventManager); | ||
| printEventBadge(eventManager); | ||
|
Comment on lines
+36
to
+43
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. 이걸 OutputView 에서 printResult() 라는 통합 메서드가 관리하도록 하고 컨트롤러는 outputView.printResult() 를 호출하도록 했어도 좋았을 것 같아요.(이렇게 바꿀 경우 파라미터도 수정을 해야하고 그렇지만...) 뷰의 출력 순서가 컨트롤러에 노출된 느낌이 들어서요. 어떻게 하는게 더 좋을지는 모르겠습니당ㅎㅎ
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. 저는 사실 이 부분에 대한 트레이드 오프가 있다고 생각해요. 컨트롤러가 뷰의 출력 순서를 안다는 것은, 자칫하면 컨트롤러가 복잡해질 수도 있지만, 세밀한 제어 및 유지보수가 가능하다는 장점이 있다 생각했습니다. 또한 뷰를 아예 다 갈아엎는 것보다, '특정' 뷰의 조각에 대한 요구사항 변화가 잦기에, 뷰를 만드는 로직은 쪼개져야한다 생각했습니다. 또한, SOLID 원칙의 ISP 원칙으로 보았을 때, OutputView의 클라이언트는 Controller이고, Controlller에게 뷰에 대해 독립된 인터페이스를 제공하는 것이 추후 유지보수를 위해 좋다 판단하여 이렇게 구현하였습니다! 물론 어떤 원칙이 답이니까 그렇게 해야해! 는 아니지만, 아직까지는 그래도 분리함으로써 얻는 이익이 코스트보다 크다고 생각하고 있습니다. 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.
인터페이스란 결국 '내가 제공하는 기능들의 사용자 그룹들에게 제공하는 기능 목록' 이라 생각합니다. 일단 느끼는 것은, |
||
| } | ||
|
|
||
| private void printIntroMessage(ReservationDay reservationDay) { | ||
| outputView.printIntroMessage(reservationDayController.createEventBenefitsPreviousDto(reservationDay)); | ||
| } | ||
|
|
||
| private void printOrderedMenus(Orders orders) { | ||
| outputView.printOrderedMenus(ordersController.createOrderedMenusDto(orders)); | ||
| } | ||
|
|
||
| private void printTotalAmountWithNoDiscount(Orders orders) { | ||
| outputView.printTotalAmountWithNoDiscount(ordersController.createTotalAmountWithNoDiscountDto(orders)); | ||
| } | ||
|
|
||
| private void printGiftMenu(Orders orders) { | ||
| outputView.printGiftMenu(eventManagerService.createGiftDto(orders)); | ||
| } | ||
|
|
||
| private void printBenefitsDetails(EventManager eventManager) { | ||
| outputView.printBenefitsDetails(eventManagerService.createBenefitsDetailsDto(eventManager)); | ||
| } | ||
|
|
||
| private void printTotalBenefitedAmount(EventManager eventManager) { | ||
| outputView.printTotalBenefitedAmount(eventManagerService.createTotalBenefitedAmountDto(eventManager)); | ||
| } | ||
|
|
||
| private void printEstimatedAmountWithDiscount(EventManager eventManager) { | ||
| outputView.printEstimatedAmountWithDiscount( | ||
| eventManagerService.createEstimatedAmountWithDiscountDto(eventManager)); | ||
| } | ||
|
|
||
| private void printEventBadge(EventManager eventManager) { | ||
| outputView.printEventBadge(eventManagerService.createEventBadgeDto(eventManager)); | ||
| } | ||
|
Comment on lines
+46
to
+77
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. OutputView 가 필요로 하는 정보를 DTO로 잘게 쪼개서 전달하고 있는데,
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 void terminatePlanner() { | ||
|
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. 프로그램의 종료를 명시해주니 더 깔끔한 느낌입니다. |
||
| Console.close(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| package christmas.controller; | ||
|
|
||
| import christmas.domain.Orders; | ||
| import christmas.domain.exception.InvalidOrdersException; | ||
| import christmas.dto.OrderedMenusDto; | ||
| import christmas.dto.TotalAmountWithNoDiscountDto; | ||
| import christmas.service.OrdersService; | ||
| import christmas.view.input.InputView; | ||
| import christmas.view.input.exception.BasicInputException; | ||
| import christmas.view.input.exception.OrdersInputException; | ||
| import christmas.view.output.OutputView; | ||
| import java.util.Map; | ||
|
|
||
| public class OrdersController { | ||
| private final InputView inputView; | ||
| private final OutputView outputView; | ||
| private final OrdersService ordersService; | ||
|
|
||
| public OrdersController(InputView inputView, OutputView outputView) { | ||
| this.inputView = inputView; | ||
| this.outputView = outputView; | ||
| ordersService = new OrdersService(); | ||
| } | ||
|
|
||
| public Orders insertOrders() { | ||
| try { | ||
| Map<String, Integer> orders = askToInsertOrders(); | ||
| return ordersService.createOrders(orders); | ||
| } catch (BasicInputException | OrdersInputException | InvalidOrdersException e) { | ||
|
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. 람다문 내부 변수에 대해서도 e가 아니라 예컨데 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. 명시적으로 어떤 예외가 발생할 수 있는지에 대해 기술하려 하였습니다! |
||
| outputView.printErrorMessage(e.getMessage()); | ||
| return insertOrders(); | ||
|
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. 네 저도 이 부분이.. 스택이 계속 쌓여서 터질 수 있겠다 생각했었습니다. 또한, 공통된 함수 형식인 부분을 함수형 인터페이스로 따로 뺄 수 있더라고요! 이와 같이 람다를 사용할 수 있더군요!
Comment on lines
+25
to
+31
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. while - try - catch 구문이 날짜를 입력 받을때도 쓰이고 있는데, 이 중복되는 부분을 함수형 인터페이스를 이용해서 템플릿 형태로 사용하시면 어떨까요?? 이런식으로 템플릿은 함수를 매개변수로 받아서 해당 함수를 실행하게끔 하면 됩니다!
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 Map<String, Integer> askToInsertOrders() throws BasicInputException, OrdersInputException { | ||
|
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. 헉 지금까지 Input Data로 여러 개의 데이터를 받아오는 경우가 없다보니 Dto로 만들면 여러 개의 input값을 한번에 담을 수 있고, |
||
| outputView.askToInsertOrders(); | ||
| return inputView.getOrders(); | ||
| } | ||
|
|
||
| public OrderedMenusDto createOrderedMenusDto(Orders orders) { | ||
| return ordersService.createOrdersHistoryDto(orders); | ||
| } | ||
|
|
||
| public TotalAmountWithNoDiscountDto createTotalAmountWithNoDiscountDto(Orders orders) { | ||
| return ordersService.createTotalAmountWithNoDiscountDto(orders); | ||
| } | ||
|
Comment on lines
+40
to
+46
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. 요런 DTO 로의 변환 로직을 컨트롤러에 추가한 이유가 궁금합니당 :)
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. DTO 내부에서 도메인 객체에 대한 변환을 하게 되면, |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| package christmas.controller; | ||
|
|
||
| import christmas.domain.ReservationDay; | ||
| import christmas.domain.exception.InvalidReservationDayException; | ||
| import christmas.dto.EventBenefitsPreviewDto; | ||
| import christmas.service.ReservationDayService; | ||
| import christmas.view.input.InputView; | ||
| import christmas.view.input.exception.BasicInputException; | ||
| import christmas.view.input.exception.DayInputException; | ||
| import christmas.view.output.OutputView; | ||
|
|
||
| public class ReservationDayController { | ||
| private final InputView inputView; | ||
| private final OutputView outputView; | ||
| private final ReservationDayService reservationDayService; | ||
|
|
||
| public ReservationDayController(InputView inputView, OutputView outputView) { | ||
| this.inputView = inputView; | ||
| this.outputView = outputView; | ||
| reservationDayService = new ReservationDayService(); | ||
| } | ||
|
|
||
| public ReservationDay insertReservationDay() { | ||
| try { | ||
| int reservationDay = askToInsertReservationDay(); | ||
| return reservationDayService.createReservationDay(reservationDay); | ||
| } catch (BasicInputException | DayInputException | InvalidReservationDayException e) { | ||
|
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. 예외를 이렇게 나눠서 딱딱 적어주니 나중에 추적하기도 편할 것 같아요 |
||
| outputView.printErrorMessage(e.getMessage()); | ||
| return insertReservationDay(); | ||
| } | ||
| } | ||
|
Comment on lines
+23
to
+31
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 int askToInsertReservationDay() throws BasicInputException, DayInputException { | ||
| outputView.askToInsertReservationDay(); | ||
| return inputView.getReservationDay(); | ||
| } | ||
|
|
||
| public EventBenefitsPreviewDto createEventBenefitsPreviousDto(ReservationDay reservationDay) { | ||
| return reservationDayService.createEventBenefitsPreviewDto(reservationDay); | ||
| } | ||
|
Comment on lines
+38
to
+40
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. 도메인을 DTO로 변환하는 건 서비스에만 놔둬도 괜찮았을 것 같기도 해요 :)
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. EventPlanner가 알아야하는 인터페이스의 범위를 컨트롤러단으로 한정시키고 싶었습니다. 물론 Controller에서의 메서드와 Service의 메서드 이름이 같고, 하는 일이 동일하지만 추후에 서비스에 다양한 기능이 생기고, 그 기능을 묶어서 한번에 제공하는 기능을 컨트롤러가 제공할 때, 메인 컨트롤러는 서브 컨트롤러의 API만 참고한다는 생각으로 해당 코드를 추가했습니다! |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| package christmas.domain; | ||
|
|
||
| import static christmas.domain.constant.reservationday.ReservationDayConstant.DECEMBER_LAST_DAY; | ||
| import static christmas.domain.constant.reservationday.ReservationDayConstant.DEFAULT_FIRST_DAY; | ||
| import static christmas.domain.exception.message.InvalidReservationDayExceptionMessage.NOT_IN_APPROPRIATE_RANGE; | ||
|
|
||
| import christmas.domain.exception.InvalidReservationDayException; | ||
|
|
||
| public abstract class DayPerMonth { | ||
|
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. abtsract class 를 사용하신 이유가 궁금합니다.
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. 추후 1월, 2월 등 차후 이벤트 날짜에 대한 제약사항의 틀을 제시하기 위함입니다! |
||
| public DayPerMonth() { | ||
| } | ||
|
Comment on lines
+10
to
+11
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. 앗 이 부분은 원래 생성자 내에서 검증 로직을 처리하는 것을 넣어두었다가 |
||
|
|
||
| protected void validate(final int day) { | ||
|
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.
|
||
| if (!isInAppropriateRange(day)) { | ||
| throw InvalidReservationDayException.of(NOT_IN_APPROPRIATE_RANGE.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| protected boolean isInAppropriateRange(final int day) { | ||
| return day >= DEFAULT_FIRST_DAY.getValue() && day <= DECEMBER_LAST_DAY.getValue(); | ||
| } | ||
|
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. 표기법상 부정접두사 In-이 아닌 전치사 In으로 쓰인 것 같은데
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.
음.. 다시 보니 좀 실수할 수 있겠다는 생각이 드네요, 어떻게 이름을 지어야할지 고민되네요. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| package christmas.domain; | ||
|
|
||
| import static christmas.domain.constant.event.ChristmasPromotion.CHRISTMAS_D_DAY_PROMOTION; | ||
| import static christmas.domain.constant.event.EventNumberConstant.NONE_PROMOTION_APPLIED_AMOUNT; | ||
| import static christmas.domain.constant.event.Promotion.GIFT_PROMOTION; | ||
| import static christmas.domain.constant.event.Promotion.SPECIAL_PROMOTION; | ||
| import static christmas.domain.constant.event.Promotion.WEEKDAY_PROMOTION; | ||
| import static christmas.domain.constant.event.Promotion.WEEKEND_PROMOTION; | ||
|
|
||
| import christmas.domain.constant.event.EventBadge; | ||
| import java.util.Collections; | ||
| import java.util.LinkedHashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class EventManager { | ||
|
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. 네, 객체에게 메시지를 보내서 일을 다 시킨다는 것만 생각했어서 객체에게 책임을 너무 무겁게 준 것 같습니다. |
||
| private final ReservationDay reservationDay; | ||
| private final Orders orders; | ||
|
|
||
| private EventManager(ReservationDay reservationDay, Orders orders) { | ||
| this.reservationDay = reservationDay; | ||
| this.orders = orders; | ||
| } | ||
|
|
||
| public static EventManager of(ReservationDay reservationDay, Orders orders) { | ||
|
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. 이 부분 고민해보았는데,
이 두가지인데, 말씀해주신 코드에서 제가 쓴 방식은 그저 |
||
| return new EventManager(reservationDay, orders); | ||
| } | ||
|
|
||
| public Map<String, Integer> createBenefitsDetails() { | ||
| Map<String, Integer> benefitsDetails = new LinkedHashMap<>(); | ||
|
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. LinkedHashMap을 사용한 이유가 궁금합니당~!
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. 혜택 출력 순서를 제가 의도한 순서로 유지해주기 위해 사용하였습니다! |
||
| if (orders.isEventApplicable()) { | ||
| return makeBenefitsDetails(benefitsDetails); | ||
| } | ||
| return Collections.unmodifiableMap(benefitsDetails); | ||
| } | ||
|
|
||
| private Map<String, Integer> makeBenefitsDetails(Map<String, Integer> benefitsDetails) { | ||
|
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. 도메인 내에서 DTO를 생성하는 것에 대한 고민이 있었습니다. 실제 다양한 요구사항이 남발하여 1주마다 새로운 버전이 릴리즈 되는 현장이라면, 코어 로직에 해당하는 도메인의 변화보다, 뷰에 대한 요구사항 변화가 다양할 것이라 생각했습니다. 뷰에 대한 요구사항이 변한다는 것은, 그에 해당하는 출력 DTO의 스펙이 변한다는 것이고, 도메인 객체가 DTO의 생성에 관여하여 결합되는 순간 도메인 객체의 수정이 잦게 일어날 것이고 이는 또다시 이 도메인으로부터 파생된 DTO의 스펙을 변화시키며 코드를 수정할 것이 매우 많아질 것이라 판단했습니다. 그렇기에 도메인은 DTO의 생성에 대해 몰라야한다 생각했습니다. 하지만, 그럼 도메인의 모든 상태변수에 대해 getter를 두어, 서비스 단에서 DTO를 조립하게 하는 것은 좋지 않은 설계라 생각했습니다. 그러면, 위 두가지 문제의 타협점으로 최소한 도메인이 제공할 수 있는 데이터의 스펙을 정해서 서비스에 제공한 뒤, 서비스에서 이를 재조립하는 식으로 구현하면 된다 생각하여, 이와 같이 Map을 반환하도록 구현하였습니다! 혹시 @jisu-om 님의 생각은 어떠실까요? |
||
| addChristmasPromotionDetails(benefitsDetails); | ||
| addWeekdayPromotionDetails(benefitsDetails); | ||
| addWeekendPromotionDetails(benefitsDetails); | ||
| addSpecialPromotionDetails(benefitsDetails); | ||
| addGiftPromotionDetails(benefitsDetails); | ||
| return Collections.unmodifiableMap(benefitsDetails); | ||
| } | ||
|
|
||
| private void addChristmasPromotionDetails(Map<String, Integer> benefitsDetails) { | ||
| if (reservationDay.isChristmasPromotionApplicable()) { | ||
| benefitsDetails.put(CHRISTMAS_D_DAY_PROMOTION.getName(), | ||
| calculateChristmasPromotionBenefitAmount()); | ||
| } | ||
| } | ||
|
|
||
| private int calculateChristmasPromotionBenefitAmount() { | ||
| return CHRISTMAS_D_DAY_PROMOTION.getBenefitAmount(reservationDay.getDay()); | ||
| } | ||
|
|
||
| private void addWeekdayPromotionDetails(Map<String, Integer> benefitsDetails) { | ||
| if (canAddWeekdayPromotion()) { | ||
| benefitsDetails.put(WEEKDAY_PROMOTION.getName(), | ||
| calculateWeekdayPromotionBenefitAmount()); | ||
| } | ||
| } | ||
|
|
||
| private boolean canAddWeekdayPromotion() { | ||
| return reservationDay.isWeekdayPromotionApplicable() | ||
| && calculateWeekdayPromotionBenefitAmount() | ||
| > NONE_PROMOTION_APPLIED_AMOUNT.getValue(); | ||
| } | ||
|
|
||
| private int calculateWeekdayPromotionBenefitAmount() { | ||
| int count = orders.getOrders().stream() | ||
| .filter(Order::isWeekdayPromotionApplicable) | ||
| .mapToInt(Order::getMenuCount) | ||
| .sum(); | ||
| return WEEKDAY_PROMOTION.getBenefitAmount() * count; | ||
| } | ||
|
|
||
| private void addWeekendPromotionDetails(Map<String, Integer> benefitsDetails) { | ||
| if (canAddWeekendPromotion()) { | ||
| benefitsDetails.put(WEEKEND_PROMOTION.getName(), | ||
| calculateWeekendPromotionBenefitAmount()); | ||
| } | ||
| } | ||
|
|
||
| private boolean canAddWeekendPromotion() { | ||
| return reservationDay.isWeekendPromotionApplicable() | ||
| && calculateWeekendPromotionBenefitAmount() | ||
| > NONE_PROMOTION_APPLIED_AMOUNT.getValue(); | ||
| } | ||
|
|
||
| private int calculateWeekendPromotionBenefitAmount() { | ||
| int count = orders.getOrders().stream() | ||
| .filter(Order::isWeekendPromotionApplicable) | ||
| .mapToInt(Order::getMenuCount) | ||
| .sum(); | ||
| return WEEKEND_PROMOTION.getBenefitAmount() * count; | ||
| } | ||
|
|
||
| private void addSpecialPromotionDetails(Map<String, Integer> benefitsDetails) { | ||
| if (reservationDay.isSpecialPromotionApplicable()) { | ||
| benefitsDetails.put(SPECIAL_PROMOTION.getName(), calculateSpecialPromotionBenefitAmount()); | ||
| } | ||
| } | ||
|
|
||
| private int calculateSpecialPromotionBenefitAmount() { | ||
| return SPECIAL_PROMOTION.getBenefitAmount(); | ||
| } | ||
|
|
||
| private void addGiftPromotionDetails(Map<String, Integer> benefitsDetails) { | ||
| if (orders.isGiftEventApplicable()) { | ||
| benefitsDetails.put(GIFT_PROMOTION.getName(), calculateGiftPromotionBenefitAmount()); | ||
| } | ||
| } | ||
|
|
||
| private int calculateGiftPromotionBenefitAmount() { | ||
| return GIFT_PROMOTION.getBenefitAmount(); | ||
| } | ||
|
|
||
| public int calculateTotalBenefitedAmount() { | ||
| return createBenefitsDetails().values().stream() | ||
| .mapToInt(Integer::intValue) | ||
| .sum(); | ||
| } | ||
|
|
||
| public int calculateEstimatedOrdersAmountWithDiscount() { | ||
| return orders.calculateTotalAmountWithNoDiscount() - calculateTotalDiscountedAmount(); | ||
| } | ||
|
|
||
| private int calculateTotalDiscountedAmount() { | ||
| if (orders.isGiftEventApplicable()) { | ||
| return calculateTotalBenefitedAmount() - calculateGiftPromotionBenefitAmount(); | ||
| } | ||
| return calculateTotalBenefitedAmount(); | ||
| } | ||
|
|
||
| public String issueEventBadge() { | ||
| return EventBadge.getBadgeNameByBenefitedPrice(calculateTotalBenefitedAmount()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| package christmas.domain; | ||
|
|
||
| import christmas.domain.constant.orders.FoodType; | ||
| import christmas.domain.constant.orders.Menu; | ||
| import christmas.domain.exception.InvalidOrdersException; | ||
|
|
||
| public class Order { | ||
| private final Menu menu; | ||
| private final int count; | ||
|
|
||
| private Order(Menu menu, final int count) { | ||
| this.menu = menu; | ||
| this.count = count; | ||
| } | ||
|
|
||
| public static Order of(final String menuName, final int count) throws InvalidOrdersException { | ||
| return new Order(Menu.searchByName(menuName), count); | ||
| } | ||
|
|
||
| public String getMenuName() { | ||
| return menu.getName(); | ||
| } | ||
|
|
||
| public int getMenuCount() { | ||
| return count; | ||
| } | ||
|
|
||
| public FoodType getFoodType() { | ||
| return menu.getFoodType(); | ||
| } | ||
|
|
||
| public boolean isWeekdayPromotionApplicable() { | ||
| return menu.isWeekDayPromotionApplicable(); | ||
| } | ||
|
|
||
| public boolean isWeekendPromotionApplicable() { | ||
| return menu.isWeekendPromotionApplicable(); | ||
| } | ||
|
|
||
| public int getTotalPrice() { | ||
| return menu.getPrice() * count; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 본 리드미 중에 가장 완성형에 가깝습니다.
독자가 비개발자, 개발자인걸 고려해서 윗단은 비즈니스팀 아랫단은 개발자를 위한 내용을 잘 작성하셨습니다.
순서 또한 to가 비즈니스팀인걸 생각하셔서 제일 윗단에 작성하신것도 정말 좋네요
그리고 목차까지 있어서 원하는 내용도 바로 숏컷으로 볼 수 있어서 참 편한 것 같습니다.
결론은 전체적으로 잘 작성된 리드미 입니다! 👍