[크리스마스 프로모션] 코드리뷰용 Pull Request by junslog · Pull Request #1 · junslog/java-christmas-6-junslog

junslog · 2023-11-16T08:26:45Z

안녕하세요 여러분들 !

이 글을 읽으실 분들은 저와 함께 4주간 달려온 우테코 6기 예비 크루분들이시겠죠?!

4주간 다들 고생 많으셨단 말부터 드리고 싶어요, 다 같이 몰입하며 크게 성장하는 1달 아니었나 싶습니다.

그 과정에서는 매주마다 코드리뷰를 통한 시너지 효과가 있었다고 생각해요!

다들 감사합니다. 서로 1주간 고민한 코드 공유하며 다같이 더 좋은 개발자로 성장해나가면 좋겠습니다 😊

이번 미션에 대해서는 다음과 같은 고민을 위주로 했어요.

1) `Orders` 와 `ReservationDay` 의 협업을 어떻게 처리하지?

크리스마스 프로모션 미션의 핵심 기능은 "예약 날짜와 주문 내역을 입력받으면 적용 가능한 이벤트 목록을 제시한다" 라 생각했어요. 예약날짜는 ReservationDay 라는 도메인 객체로, 주문 내역은 Orders 도메인 객체로 나눴고 그 안에서 각각의 도메인 로직을 구현했습니다.
다만, 이벤트를 적용하는 부분은 물음표더라구요. 이건 누가 하는 일일까? 고민을 했었습니다.
결론적으로 ReservationDay와 Orders가 협업을 통해 해결해야하는 문제 영역이라 생각하게 되었고
어떠한 방식으로 이 문제를 해결해 나갈지에 대한 방법을 계속 생각했었습니다.

처음에는 Orders 객체에 ReservationDay 를 인자로 받아 Orders가 일하게 하거나,
ReservationDay 객체가 Orders를 인자로 받아 ReservationDay 가 일을 하게 하는 방식으로 구상을 했는데,
두 도메인 객체 중 하나가 너무 복잡해진다는 생각을 했어요 ( 이벤트가 워낙 많으니..ㅎㅎ )
또한 변경 가능성과 유지보수성에서도 유리하지 않은 코드 구성양식이더라구요.

그래서 EventService 라는 것을 두어, 두 객체를 꺼내와서 이벤트 로직을 처리하는 방향으로 구성했다가,
Service 단 코드가 너무 무거워진다는 생각을 했습니다. EventService가 이벤트 로직을 다 처리하기에, 코드의 응집성은 높지만, 결합도 또한 높았고 결국 모든 로직은 ReservationDay와 Orders에게서 오기에 변경시 그 전파 범위가 작지 않다 생각했어요.

마지막으로 결과를 낸 것은
EventManager 라는, Orders와 ReservationDay 객체를 인스턴스 변수로 갖는 도메인을 하나 추가하는 것이었습니다.
이걸 하고 나서 너무 놀랐던게, EventService에서는 이벤트 로직 처리를 위해서 Orders와 ReservationDay객체들을 다 파라미터로 넘겨주는 방식이어야 했는데, EventManager는 자기 안에 Orders와 ReservationDay를 들고 있으니, 코드가 매우 간결해지고 도메인 객체 자체가 일을 하는 형식으로 코드를 구성할 수 있더라구요.
그래서 EventService ( <- EventManager 도메인 객체 생성 후 EventManagerService로 이름을 변경 ) 코드가 매우 간결해진다는 느낌을 받았어요.
이래서, 객체를 꺼내 쓰지 말고 객체에 메시지를 보내서 일을 하게 하라는 거구나를 깨달았던 소중한 경험이었습니다.

2) 리드미를 어떻게 작성해야할까? ( `Code` 탭의 가서 `docs` 참조해주시면 감사하겠습니다! )

도대체 리드미, 즉 ‘나를 읽어라’ 라는 뜻의 문서는 누가 읽는 문서일까? 하는 생각이 들었어요.
글을 쓸 때도 일기장이 아니고서야 독자가 관심을 가질만한 것을 이야기해야 그 글의 가치가 있기에
읽을 가치 있는 README를 쓰고 싶었습니다.

왜 리드미를 쓸까를 고민해본 결과, 코드를 처음 보고 파악하기에는 너무 오래걸리거나(개발자), 못 보는 (기획자, 비즈니스팀 인원) 사람들을 위해 빠르게 그들이 프로젝트에 관심있는 부분만 보여주는 것이 리드미라는 결론을 내렸어요.

이번 4주차 제 리드미의 독자는 비즈니스팀과 개발팀으로 잡았습니다.
그리고 그들이 뭘 원할지를 고민해보았어요.

비즈니스팀은 코드나 아키텍처는 관심이 없을 것이고, 단지 실행결과, 동작방식등 인터페이스 부분만을 원할 것이라 생각하여 그 부분만 정리해서 노출했습니다.

저와 함께 협업하는 개발팀의 제 프로젝트를 처음 보는 동료에게는 대략적인 프로젝트의 구조를 디렉토리 구조 또는 다이어그램으로 표현해줘서, 그것을 보고 빠르게 이해하고 협업할 수 있게 만드는 것이 목표였어요.

추후에 제 프로젝트를 유지보수하거나, 제 코드를 재사용하고 싶은 개발자를 대상으로는 어떻게 하면 12월 이벤트 용도로만 어플리케이션을 이용하는 것이 아닌, 1월이나 2월 등 다른 달의 이벤트에 활용하는 방안에 대해 짚어주는 식으로 리드미를 썼습니다.

또한, 기능 요구사항 메일에서 1주 뒤에 이벤트에 대해 회의를 한다는 이야기가 있어, 개발을 하면서 느낀 비즈니스 관점에서 더 나을 것 같은 방안을 구현 가능한 범위 내에서 제시해보려고 했습니다.

꼭.. Pull requests 탭 말고, 제 Code로 가서 docs에 한번 들어가주세요! 제가 리드미에 부린 재롱..한번 구경해주시면 감사하겠습니다..ㅎㅎ

3) 불변 컬렉션, `final`활용

Collections.unmodifiablList() / Collections.unmodifiableMap() 과 같은 수정 연산을 제한해주는 불변 컬렉션을 만들어서 생각지 못했던 변경을 막아 버그 가능성을 줄이고자 했습니다.
또한 원시값이나 String값에 변경이 일어나지 않는게 확실하고, 앞으로도 그럴 가능성이 없을거라 생각했던 부분에는 다 final 처리를 해주었습니다.

4) 예외의 커스텀화 및 메서드가 예외를 `throw`하는 것을 명시화

코드의 가독성, 처음 코드를 보는 개발자를 위해 해당 메서드가 어떤 오류를 던지는지를 명시하려고 노력했어요.
또한, 추후 확장성 및 유지보수를 위해서 어떤 종류의 예외이고, 어떻게 다뤄지는지를 명시하고자 했어요.

5) 도메인별 역할 분리

Orders와 ReservationDay는 사용자 입력을 받기에, 각 이름에 맞게 중계해주는 Controller를 두었어요.
EventManager는 입력받은 Orders와 ReservationDay를 기반으로 일을 하는 객체, 즉 기능만을 제공해주는 친구라
해당 도메인 로직을 조합해서 가공해줄 수 있는 EventManagerService만 두었습니다.

6) 1차 검증은 `InputView` 에서, 2차 이후 검증은 도메인에서

제가 검증 로직을 나눈 기준은
InputView에서 다루는 검증은 "변경 가능성이 적은 검증 로직",
도메인에서 다루는 검증은 "비즈니스 정책에 따라 변경 가능성이 있는 검증 로직" 이었습니다.

예를 들어,
비어있는 입력값.
공백을 포함해서 2000글자 이상의 입력이나,
메뉴의 개수가 숫자형식이 아닌 경우
InputView 단에서 예외를 발생시키고

날짜가 1~31사이 값이어야 한다거나,
주문 개수가 20개가 넘어가면 안되거나,
음료수만 시키면 안되는 경우
메뉴판에 없는 주문 같은 경우는
도메인 단에서 예외 처리해주려고 했어요.

휴, 적다보니 참 길어졌네요. 미리 제 코드를 봐주시는 여러분에게 감사인사 드릴게요 🙇‍♂️

프리코스가 끝나도, 앞으로 여러분들과 함께한 몰입 기억은 여운이 남을 거 같아요.

다시한번 다들 정말 고생하셨고, 앞으로도 커리어나 하시는 일들 응원할게요. :) 감사합니다.

…ropriate range

… not more than 2 without blank

…tible to numeric type

…ve integer

…exception

…range

…, even considering that it contains blanks

…'-')

…propriate range

…t what content user input missed

…more than 2

…list

…inal keyword

imbesky

코드 잘 봤습니다! 4주차 미션 수고하셨습니다.

imbesky · 2023-11-16T11:06:16Z

src/main/java/christmas/EventPlanner.java

+        outputView.printEventBadge(eventManagerService.createEventBadgeDto(eventManager));
+    }
+
+    private void terminatePlanner() {


프로그램의 종료를 명시해주니 더 깔끔한 느낌입니다.

imbesky · 2023-11-16T11:09:03Z

src/main/java/christmas/controller/OrdersController.java

+            return ordersService.createOrders(orders);
+        } catch (BasicInputException | OrdersInputException | InvalidOrdersException e) {
+            outputView.printErrorMessage(e.getMessage());
+            return insertOrders();


재입력 받는 로직을 재귀로 처리하는 것도 좋다고 생각합니다. 다만 가능성이 낮다고는 해도 스택 오버플로우에 대한 대책이 있어야 할 것 같아요.

네 저도 이 부분이.. 스택이 계속 쌓여서 터질 수 있겠다 생각했었습니다.
이 부분을 while 문을 통한 반복으로 구현하는 것이 좋을 것 같다 생각합니다.

또한, 공통된 함수 형식인 부분을 함수형 인터페이스로 따로 뺄 수 있더라고요!

private <T> T readUserInput(Supplier<T> supplier) { while (true) { try { return supplier.get(); } catch (IllegalArgumentException e) { outputView.printError(e.getMessage()); } } }

return readUserInput(() -> { int userInput = ~ ... return reservationday... });

이와 같이 람다를 사용할 수 있더군요!

imbesky · 2023-11-16T11:11:18Z

src/main/java/christmas/domain/EventManager.java

+import java.util.LinkedHashMap;
+import java.util.Map;
+
+public class EventManager {


이 클래스에서 하는 일을 조금 분리해서 계산같은 로직은 다른 클래스를 이용해도 좋았을 것 같습니다.

네, 객체에게 메시지를 보내서 일을 다 시킨다는 것만 생각했어서 객체에게 책임을 너무 무겁게 준 것 같습니다.
계산이나 할인 로직을 util 형식으로 빼서 구현을 해도 좋았겠다는 생각이 드네요 ㅠ

imbesky · 2023-11-16T11:12:27Z

src/main/java/christmas/domain/ReservationDay.java

+        return isWeekend();
+    }
+
+    private boolean isWeekend() {


나머지를 이용하는 방식도 간편하고 좋은 것 같네요!

imbesky · 2023-11-16T11:16:12Z

src/main/java/christmas/view/output/constant/OutputMessageConstant.java

+import static christmas.global.ApplicationConstant.CURRENT_PROMOTION_MONTH;
+
+public enum OutputMessageConstant {
+    GREETING("안녕하세요! 우테코 식당 " + CURRENT_PROMOTION_MONTH + "월 이벤트 플래너입니다."),


+ 연산자보다는 String.concat()이나 StringBuilder 사용을 추천드리고 싶습니다.

public static void main(String[] args) { String result = ""; long start = System.currentTimeMillis(); for(int i = 0; i < 500000; i++){ result += "test"; } long end = System.currentTimeMillis(); System.out.println("String exec time : " + (end - start)); StringBuilder resultBuilder = new StringBuilder(); start = System.currentTimeMillis(); for(int i = 0; i < 500000; i++){ resultBuilder.append("test"); } end = System.currentTimeMillis(); System.out.println("String Builder Exec Time : " + (end - start)); }

// 실행 결과 String exec time : 21145 String Builder Exec Time : 6

예전에 어느 글에서 JDK 1.5 버전 이후에 String의 + 연산은 컴파일 과정에서 StringBuilder를 통한 연산으로 바뀐다고 들었었는데..
실험을 해보니 확실히 많은 차이가 나네요..! 조언 감사합니다!!!

imbesky · 2023-11-16T11:17:50Z

src/main/java/christmas/view/input/validator/OrderInputValidator.java

+    }
+
+    private void validateContainingDelimiter(final String orderInput) {
+        if (!containsDelimiter(orderInput)) {


containsDelimiter정도는 별개의 메서드로 빼지 않고 내부 로직으로 작성해도 괜찮지 않을까요?

물론 메서드 이름 자체가 어떤 일을 할지 예측이 되지만,

Delimiter 포함 여부를 판단하는 로직이 다양해질 수 있는 점

If문을 캡슐화하는 방향으로 다른 코드를 구현해서, 코드의 통일성을 위함

이 두가지를 고려해보면 묶는 것도 좋을 거 같단 생각입니다.

@imbesky 님의 이에 대한 생각이 궁금해요!

gomudayya · 2023-11-16T13:36:51Z

src/main/java/christmas/EventPlanner.java

+    public EventPlanner(InputView inputView, OutputView outputView) {
+        this.outputView = outputView;
+        reservationDayController = new ReservationDayController(inputView, outputView);
+        ordersController = new OrdersController(inputView, outputView);


컨트롤러를 두 개를 쓰신 이유가 궁금합니다. 그리고 두 개의 컨트롤러가 하나의 인풋뷰를 관리하는 것이 조금 낯선데, 이렇게 구성하신 이유가 있나요?

그리고 제 개인적인 생각으로는 이 EventPlanner가 제일 컨트롤러 역할에 가까운 느낌입니다.
(뷰와 서비스 계층을 이어준다는 느낌에서)

저는 Controller 란, UI와 도메인을 연결해주는 통로라고 생각했습니다.

말씀해주신대로 EventPlanner 라는 클래스가 메인 컨트롤러의 역할을 하고,
각 UI 별 로직을 담당 컨트롤러를 두어
메인 컨트롤러는 지휘자의 느낌,
서브 컨트롤러들은 각 파트의 장 느낌으로 계층을 구성하려는 의도였어요!

물론 이 흐름대로면, 하나의 InputView를 관리하는 것이 아닌 각 도메인마다 뷰가 달라야하겠지만, 입력받는 Input값의 수가 많은 편이 아니고, 보여줘야할 데이터는 많지만 각 서브 도메인들의 협업으로 처리한 결과물 ( 이벤트 결과 )을 보여주는 로직이 많다 생각해서
뷰를 쪼개지는 않았습니다!

gomudayya · 2023-11-16T13:46:01Z

src/main/java/christmas/service/ReservationDayService.java

OrdersService와 ReservationDayService는 서비스 계층이라고 보기가 조금 어려운 것 같습니다.
서비스 계층은 핵심 비즈니스 로직(할인, 증정)이 전개되어야 하는데, 위의 두 서비스는 입력 데이터를 정제하고, 파싱하는 것에 초점이 맞춰져 있는 것 같습니다.

만약 대부분의 비즈니스 로직이 도메인에서 이루어진다면
컨트롤러와 도메인 사이에서 데이터를 정제해서
도메인에게 넘겨주는 계층을 서비스라고 불러도 괜찮지 않을까요?

OrdersService와 ReservationDayService는 서비스 계층이라고 보기가 조금 어려운 것 같습니다. 서비스 계층은 핵심 비즈니스 로직(할인, 증정)이 전개되어야 하는데, 위의 두 서비스는 입력 데이터를 정제하고, 파싱하는 것에 초점이 맞춰져 있는 것 같습니다.

이 부분에 대해 이번 주차에 고민을 많이 헀습니다.
객체를 객체스럽게 사용한다는 것은 , Getter를 통해 빼오는 것이 아니라 객체에 메시지를 보내서 일을 하게 하는 것이라는 피드백을 듣고 모든 도메인의 로직을 도메인 객체에게 위임하는 식으로 코드를 짜려 했었어요.

그렇게 되니 서비스 단은 도메인에 요청을 하고 결과를 받아 포장하는 역할만을 하는 식으로 구성이 되었습니다.
컨트롤러가 서비스간의 로직을 조합해서 UI로 결과를 보내주는 것처럼,
서비스는 도메인간의 로직을 조합하여 컨트롤러로 결과를 보내주는 것이라 생각했습니다.
이런 이유로 서비스를 구성하는 방식을 getter로 도메인 객체를 가져와 그 안의 상태를 기반으로 한 로직처리보단, 로직 처리된 결과물을 조합하는 것으로 방향성을 잡았습니다!

gomudayya · 2023-11-16T13:47:08Z

src/main/java/christmas/domain/ReservationDay.java

+        return day % WEEKDAY_LENGTH.getValue() == FRIDAY_VALUE.getValue()
+                || day % WEEKDAY_LENGTH.getValue() == SATURDAY_VALUE.getValue();
+    }
+


자바의 LocalDate 클래스를 활용하시면 요일 계산이 좀 더 편해질 수 있을 것 같습니다!

오 날짜와 시간을 처리하는 API가 잘 구현되어있었군요!
알려주셔서 감사합니다 👍 👍 👍

gomudayya · 2023-11-16T13:53:51Z

src/main/java/christmas/domain/exception/InvalidReservationDayException.java

예외나 검증 파트는 정말 꼼꼼하게 잘하신 것 같네요!

gomudayya · 2023-11-16T13:54:56Z

src/main/java/christmas/domain/constant/orders/Menu.java

+    ZERO_COKE("제로콜라", 3_000, BEVERAGE),
+    RED_WINE("레드와인", 60_000, BEVERAGE),
+    CHAMPAGNE("샴페인", 25_000, BEVERAGE);
+


개행으로 카테코리별로 끊어주시니 보기 좋은 것 같습니다
요런 것들 하나하나가 클린 코드로 가는 첫걸음이라고 생각합니다.

gomudayya · 2023-11-16T13:58:48Z

src/main/java/christmas/dto/BenefitsDetailsDto.java

Dto 계층의 Map이 대부분 String이신데 Enum으로 관리하시면 보는사람도 더 명시적이여서 좋고, 코딩할때도 훨씬 편해질 것 같습니다.

말씀하신대로 EnumMap을 활용하여 Dto를 구성하는 것이 이름을 명시적으로 줄 수 있어서 좋은 것 같습니다!!
배워갑니다 :)

jisu-om

정성스러운 리드미, PR 메세지 감명깊게 읽었습니다 !! 흑 제 리드미 눈감아....
4주동안 고생 많으셨고 앞으로도 열심히 달려보아요 :)

jisu-om · 2023-11-16T13:01:55Z

src/main/java/christmas/EventPlanner.java

+        reservationDayController = new ReservationDayController(inputView, outputView);
+        ordersController = new OrdersController(inputView, outputView);
+        eventManagerService = new EventManagerService();
+        outputView.printGreetingMessage();


생성 시 메세지가 출력되도록 한 것 신선해요 ㅎㅎㅎ

execute() 메서드에서 바로 예약날짜와 주문을 입력받는 코드를 보여줘서 가독성을 높이는 것이 의도였는데,
생성자에서 할만한 일인가? 하는 생각이 들더라고요.
가독성을 챙기려다 확장성에서 좀 아쉬운 판단을 한거같다는 갠적인 생각합니다..ㅎㅎ

굿굿 생성자에서 빼주시는게 더 좋을 것 같습니다.

jisu-om · 2023-11-16T13:04:26Z

src/main/java/christmas/EventPlanner.java

+        Orders orders = ordersController.insertOrders();
+        EventManager eventManager = EventManager.of(reservationDay, orders);
+        printResult(reservationDay, orders, eventManager);
+        terminatePlanner();


execute 메서드 안에 terminatePlanner() 로직이 들어있는게 이해가 가는데
(저도 이렇게 했다가 이 클래스 외부 - Application.main() 에서 처리하자로 바꿨어서요)
자원 정리 느낌이라 어디에 두어도 상관은 없을 것 같기는 합니당!

말씀대로 돌아보니,
Application에서 EventPlanner 시스템만 시행할 것이라는 전제가 깔린 코드라는 생각이 듭니다.
혹여 다른 시스템이 추가될 미래를 생각한다면, Application 내에 종료될 때 자원을 회수하는 것이 좋겠어요!

jisu-om · 2023-11-16T13:05:16Z

src/main/java/christmas/controller/ReservationDayController.java

+        try {
+            int reservationDay = askToInsertReservationDay();
+            return reservationDayService.createReservationDay(reservationDay);
+        } catch (BasicInputException | DayInputException | InvalidReservationDayException e) {


예외를 이렇게 나눠서 딱딱 적어주니 나중에 추적하기도 편할 것 같아요

jisu-om · 2023-11-16T13:08:33Z

src/main/java/christmas/domain/EventManager.java

+    }
+
+    public Map<String, Integer> createBenefitsDetails() {
+        Map<String, Integer> benefitsDetails = new LinkedHashMap<>();


LinkedHashMap을 사용한 이유가 궁금합니당~!

혜택 출력 순서를 제가 의도한 순서로 유지해주기 위해 사용하였습니다!
물론 요구사항에서는 출력순서는 상관 없었지만, 제 욕심이었어요..!

jisu-om · 2023-11-16T13:10:35Z

src/main/java/christmas/domain/constant/event/ChristmasPromotion.java

+
+import static christmas.domain.constant.event.EventNumberConstant.CHRISTMAS_PROMOTION_INCREASING_AMOUNT;
+
+public enum ChristmasPromotion {


ChristmasPromotion 과 Promotion 은 연관성이 크다고 생각되어서 혹시 한번에 관리하는 건 어떻게 생각하세요?!

그리고 ChristmasPromotion, Promotion 의 enum 클래스 안에 계산로직을 넣는 것도 좋을 것 같아용! 그러면 뭔가 서비스단이 가벼워질 것 같습니당 :)

enum 활용법의 최고봉 자료 놓고 갑니당
https://techblog.woowahan.com/2527/

Enum 에 할인, 이벤트 등의 로직을 몰아넣어 도메인 객체로서 사용할 생각은 전혀 하지 못했었네요.
그렇게 하면 기존에 너무 무거웠던 EventManager 객체의 역할이 분리되어 SRP 를 잘 지킬 수 있겠어요.
좋은 자료 감사합니다 ☺️

ChristmasPromotion 과 Promotion 은 연관성이 크다고 생각되어서 혹시 한번에 관리하는 건 어떻게 생각하세요?!

맞습니다, 한번에 관리하는게 좋을 것 같은데
할인 로직이 고정되어있지 않은 점이 구현할 때 문제였었습니다.
이 부분에 대해 다른 분들 코드를 보니 함수형 인터페이스를 적재적소에 활용하여 해결하셨더라구요..!
저도 한번 리팩토링하면서 적용해봐야겠습니다 :)

jisu-om · 2023-11-16T13:52:12Z

src/main/java/christmas/controller/OrdersController.java

+    public OrderedMenusDto createOrderedMenusDto(Orders orders) {
+        return ordersService.createOrdersHistoryDto(orders);
+    }
+
+    public TotalAmountWithNoDiscountDto createTotalAmountWithNoDiscountDto(Orders orders) {
+        return ordersService.createTotalAmountWithNoDiscountDto(orders);
+    }


요런 DTO 로의 변환 로직을 컨트롤러에 추가한 이유가 궁금합니당 :)
(ReservationDayController 의 createEventBenefitsPreviousDto() 와 동일하게요)

DTO 내부에서 도메인 객체에 대한 변환을 하게 되면,
도메인 객체가 하나 변하는 순간, 그로부터 파생되는 DTO는 다양할 것이기에 변경의 전파범위가 작지 않다 생각했습니다.
서비스 단에서 결국 DTO들을 생성하기에, 서비스단 에 변환로직을 둔다면 추후 유지보수성에서 유리하다 판단했습니다.

jisu-om · 2023-11-16T13:54:31Z

src/main/java/christmas/EventPlanner.java

+    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));
+    }


OutputView 가 필요로 하는 정보를 DTO로 잘게 쪼개서 전달하고 있는데,
이 DTO 들을 묶는 큰 DTO가 있어도 좋을 것 같아요.

#1 (comment)

해당 의견에 대해 언급드린 이 부분의 의견과 동일합니다!

jisu-om · 2023-11-16T14:19:10Z

src/main/java/christmas/dto/OrderedMenusDto.java

+    public static OrderedMenusDto from(Map<String, Integer> orders) {
+        return new OrderedMenusDto(orders);
+    }


Map<String, Integer> orders 로 인자를 받는게 아니라 Orders orders 로 받아도 좋을 것 같아요!

public static OrderedMenusDto from(Orders orders) { return orders.stream() .collect(Collectors.toUnmodifiableMap( Order::getMenuName, Order::getMenuCount )); }

그러면 OrdersService의 makeOrdersHistory() 를 없애도 될 것 같아보여요
OrderedMenusDto 가 생성 로직을 스스로 갖고 있어도 좋을 것 같아서요!

public OrderedMenusDto createOrdersHistoryDto(Orders orders) { return OrderedMenusDto.from(makeOrdersHistory(orders)); //return OrderedMenusDto.from(orders); 로 변경 } private Map<String, Integer> makeOrdersHistory(Orders orders) { //삭제 가능 return orders.getOrders().stream() .collect(Collectors.toUnmodifiableMap( Order::getMenuName, Order::getMenuCount )); }

그리고 추가로 orders 필드의 자료구조가 Map<String, Integer> 인데
OrderedMenuDto 라는 객체를 새로 만들어서 List<OrderedMenuDto> orders 로 바꾸면 생성도 더 깔끔해질 것 같아요

OrderedMenusDto.getOrders() 내부에서 Collections.unmodifiableMap()을 사용하지 않고 그냥 return orders; 해도 되구요
이렇게 바꾸면 OutputView 의 printOrderedMenu() 로직이 살짝 수정될 것 같아요!

#1 (comment)

이 의견과 동일한 의견입니다!

List orders 로 가져가는 것 되게 좋은 것 같습니다! 무의식적으로 Dto 내부에는 Dto가 있으면 안된다 생각했는데, 외부에 대한 인터페이스만 디미터의 법칙을 지키는 식으로 잘 구현하면, 꼭 그럴필요도 없을 것 같습니다! 좋은 의견 감사합니다 :)

DTO, 자료구조와 같은 경우에는 내부 구조를 외부에 노출하는것이 당연하므로
디미터 법칙을 적용하지 않습니다.

jisu-om · 2023-11-16T14:20:08Z

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

+    public void printOrderedMenus(OrderedMenusDto orderedMenusDto) {
+        print(ORDERED_MENUS.getSymbol());
+        printLine();
+        orderedMenusDto.getOrders().forEach(this::printOrderedMenu);


요거 어떻게 Map 의 key, value가 알아서 딱딱 들어가게 되는거예요? @.@? 디버깅해도 모르겠어요;;;

forEach 문에서 Map이 바로 Stream으로 변환되어,
printOrderedMenu 의 메서드 시그니처에 맞게
( orderedMenu, count ) -> printOrderedMenu( orderedMenu, count );
와 같이 동작합니다.

jisu-om · 2023-11-16T14:26:29Z

src/main/java/christmas/dto/BenefitsDetailsDto.java

+    public static BenefitsDetailsDto from(Map<String, Integer> benefitsDetails) {
+        return new BenefitsDetailsDto(benefitsDetails);
+    }


DTO를 생성할 때 도메인 객체를 파라미터로 받아서 생성 메서드 안에서 로직을 돌려도 될 것 같아요!
전반적으로 DTO 객체들의 생성 로직이 서비스단에 노출된 느낌이 들었습니당!!
그리고 이렇게 로직을 작성하신 이유가 궁금해요!!! ◡̈

#1 (comment)

이 의견과 내용이 동일합니다!

개인적으로는 @jisu-om 님 처럼 하지 않는다면 DTO에 굳이 정적 팩터리 메서드가 필요한가 싶네요 ㅎㅎ
생성자랑 하는일이 동일하고 from이란 네이밍도 딱히 더 유의미한 정보를 주지 않군요

gomudayya · 2023-11-16T14:34:12Z

4주 동안 정말 고생 많으셨습니다!! 저도 앞으로 하시는 일 잘되길 응원할게요. 감사합니다!!

phk1128 · 2023-11-16T23:50:42Z

docs/README.md

+```bash
+안녕하세요! 우테코 식당 12월 이벤트 플래너입니다.
+12월 중 식당 방문 날짜는 언제인가요? (숫자만 입력해 주세요!)
+> 
+[ERROR] 유효하지 않은 날짜입니다. 다시 입력해 주세요.
+12월 중 식당 방문 날짜는 언제인가요? (숫자만 입력해 주세요!)
+>a
+[ERROR] 유효하지 않은 날짜입니다. 다시 입력해 주세요.
+12월 중 식당 방문 날짜는 언제인가요? (숫자만 입력해 주세요!)
+>40
+[ERROR] 유효하지 않은 날짜입니다. 다시 입력해 주세요.
+12월 중 식당 방문 날짜는 언제인가요? (숫자만 입력해 주세요!)
+>-5
+[ERROR] 유효하지 않은 날짜입니다. 다시 입력해 주세요.
+12월 중 식당 방문 날짜는 언제인가요? (숫자만 입력해 주세요!)
+```
+
+#### 유효하지 않은 예약날짜 입력 기준
+
+- 예약 날짜 입력에 다음과 같은 값을 입력할 시,
+  <br> 에러가 발생하고 예약 날짜를 다시 입력받습니다.
+    - 비어있는 값
+    - 숫자 형식이 아닌 입력 값
+    - 1부터 31 사이가 아닌 값
+    - 0보다 작은 숫자 값
+    - 공백 포함 2000글자 이상인 값
+    - 공백 제거 후 3글자 이상인 값


예외상황의 정의와 예시를 들어주니까 훨씬 이해하기 편한것같습니다!

phk1128 · 2023-11-16T23:54:29Z

docs/README.md

+- 주문 내역에 다음과 같은 값을 입력할 시,
+  <br> 에러가 발생하고 주문 내역을 다시 입력받습니다.
+    - 비어있는 값
+    - 메뉴 이름이 비어있는 값
+    - 메뉴 개수가 비어있는 값
+    - 메뉴 개수가 숫자 형식이 아닌 값
+    - 메뉴 이름이 30글자 초과인 값
+    - 메뉴 개수가 3글자 초과인 값
+    - 판매중인 메뉴가 아닌 메뉴 이름을 포함한 값
+    - 각 주문의 메뉴 개수 합이 20개를 초과하는 값
+    - 메뉴 이름이 음료만으로 이루어진 값
+    - 메뉴 이름과 메뉴 개수를 나누는 구분자(-)가 포함되지 않는 값
+    - 구분자(-)로 끝나는 값
+    - 공백 포함 2000글자 초과인 값
+    - 공백 제거 후 1000글자 초과인 값


글자수에 대한 꼼꼼한 예외처리가 돋보입니다. 혹시 글자수 제한은 어떤 기준으로 정하신걸까요??

하나의 메뉴가 가장 긴 이름을 가진다면 얼마만큼의 길이일까를 고민해봤어요.
우리나라 사람들이 접할 수 있는 대중적인 음식 중 그래도 이름이 가장 긴 건 프랑스 음식이 아닐까? 싶었습니다.
프랑스 음식 이름 중 가장 긴 음식을 한국어로 번역해보니 '에크레비스와 낭튀아 크림 소스의 닭 요리' 더라구요
대략 20글자였고, 그래도 혹시 모르니 새로운 메뉴를 만든다면 30글자는 안넘겠지 하는 생각으로 만들었습니다.

그리고, 메뉴 개수가 3글자를 초과한다는 것은, 100개 이상을 주문했다는 것인데
뷔페가 아닌 레스토랑에 100개 이상의 주문이 한번에 들어올 수 있을까? 하는 생각에 저는 아니라고 생각했습니다.
그래서 최대 99개의 수량으로 한정짓고, 장난/공격성을 띄는 입력값에 대한 방어로 3글자를 길이 제한으로 두었습니다.

공백 포함 2000글자, 공백 제외 1000자 초과의 경우,
서버의 자원을 낭비시키기 위한 공격으로 매우 긴 String을 보낼 수 있다 생각해서 추가한 제한사항입니다.
이에 대해, 메뉴 개수 최대 길이 : 30 + '-' + 개수 = 33, 33 * 20개가 사실 공백 제외 660글자이지만
동료 개발자들이 이런 상세사항에 대해 아는건 중요치 않다 생각했고, 맥시멈 입력값을 넉넉하게 통용적인 1000글자로 잡아놨습니다.
또한, 공백을 너무 과도하게 포함하여 2000글자 이상이면 다시 입력하게 구현했습니다.

phk1128 · 2023-11-17T00:02:28Z

docs/README.md

제가 본 리드미 중에 가장 완성형에 가깝습니다.

독자가 비개발자, 개발자인걸 고려해서 윗단은 비즈니스팀 아랫단은 개발자를 위한 내용을 잘 작성하셨습니다.

순서 또한 to가 비즈니스팀인걸 생각하셔서 제일 윗단에 작성하신것도 정말 좋네요

그리고 목차까지 있어서 원하는 내용도 바로 숏컷으로 볼 수 있어서 참 편한 것 같습니다.

결론은 전체적으로 잘 작성된 리드미 입니다! 👍

phk1128 · 2023-11-17T00:07:27Z

src/main/java/christmas/controller/OrdersController.java

+    public Orders insertOrders() {
+        try {
+            Map<String, Integer> orders = askToInsertOrders();
+            return ordersService.createOrders(orders);
+        } catch (BasicInputException | OrdersInputException | InvalidOrdersException e) {
+            outputView.printErrorMessage(e.getMessage());
+            return insertOrders();


while - try - catch 구문이 날짜를 입력 받을때도 쓰이고 있는데, 이 중복되는 부분을 함수형 인터페이스를 이용해서 템플릿 형태로 사용하시면 어떨까요??

@FunctionalInterface public interface InputCallback<T> { T run() throws IllegalArgumentException; }

public class InputTemplate { public <T> T execute(InputCallback<T> callback) { while (true) { try { return callback.run(); } catch (IllegalArgumentException e) { System.out.println(e.getMessage()); } } } }

이런식으로 템플릿은 함수를 매개변수로 받아서 해당 함수를 실행하게끔 하면 됩니다!

함수형 인터페이스를 정말 잘 활용하시네요!

입력부분에서 반복되는 코드를 짜는게 참 기분이 안좋았는데

이런 좋은 방법이 있다는걸 알았다면..ㅎ 지금부터라도 적용해봐야겠습니다. 감사합니다!!!

phk1128 · 2023-11-17T00:08:54Z

src/main/java/christmas/domain/constant/event/EventBadge.java

+    public static String getBadgeNameByBenefitedPrice(final int totalBenefitedPrice) {
+        if (isStar(totalBenefitedPrice)) {
+            return STAR.name;
+        }
+        if (isTree(totalBenefitedPrice)) {
+            return TREE.name;
+        }
+        if (isSanta(totalBenefitedPrice)) {
+            return SANTA.name;
+        }
+        return NO_BADGE_SYMBOL.getSymbol();
+    }


이 부분을 stream을 이용해서 처리해보시는게 어떠실까요?? ㅎㅎ

같은 의견입니다.
만약 뱃지가 100개라면? 새로운 배지가 추가된다면?
배지마다 지정하는 기간이 부여된다면?
이 모든 것을 하드코딩된다면 비즈니스 로직이 변경에 취약하고, 휴먼 에러를 낳을 가능성이 높을 것 같아요.

@phk1128 @h-beeen
좋은 의견 감사합니다!
스트림이 단순히 가독성이 좋은 코드를 생성한다 생각했는데
비즈니스 로직의 변화에 대응 및 확장에도 좋겠단 생각을 처음 하게 되었습니다!
배워갑니다 ☺️

phk1128 · 2023-11-17T00:12:08Z

src/main/java/christmas/dto/BenefitsDetailsDto.java

개인적인 의견으로는 웹계층, DB계층이 없으면 DTO는 불필요하다고 생각하는데, 어떻게 생각하시나요??

혹시 다른 이유가 있으신건지 궁금합니다! ㅎㅎ

DTO의 본질은 데이터를 담는 그릇과 같다 생각하기에,
웹/DB의 환경에 사용이 종속되지 않는다 생각합니다!

DTO를 도입하면
입력값과 출력값에 의도를 담은 네이밍을 담을 수 있는 점이 좋은 것 같습니다.!! ( 마치 일급컬렉션의 장점과 같이 )

phk1128 · 2023-11-17T00:14:37Z

src/main/java/christmas/controller/ReservationDayController.java

+    public ReservationDay insertReservationDay() {
+        try {
+            int reservationDay = askToInsertReservationDay();
+            return reservationDayService.createReservationDay(reservationDay);
+        } catch (BasicInputException | DayInputException | InvalidReservationDayException e) {
+            outputView.printErrorMessage(e.getMessage());
+            return insertReservationDay();
+        }
+    }


윗분이 말씀하신대로 예외가 상황별로 분류되어 있어 추후 문제가 발생했을때 트러블슈팅하기 편할것같네요 ㅎㅎ 이 부분은 메모해두었다가 저도 사용해보겠습니다!

phk1128 · 2023-11-17T00:25:31Z

src/test/java/christmas/domain/EventBadgeTest.java

+    @DisplayName("혜택 금액에 따라 이벤트 배지를 발급한다.")
+    @ParameterizedTest
+    @CsvSource(value = {
+            "1000:''", "4999:''", "5000:별", "8230:별", "9999:별", "10000:트리", "13200:트리",
+            "14999:트리", "15000:트리", "19999:트리", "20000:산타", "20001:산타", "83000:산타"
+    }, delimiter = ':')


키-값 형태로 테스트 소스를 넣어주는 방법이 있었군요! 하나 배워갑니다 ㅎㅎ

phk1128 · 2023-11-17T00:29:09Z

src/test/java/christmas/domain/EventManagerTest.java

+        EventManager eventManager = createEventManager(day, menuAndCounts);
+        // when
+        Map<String, Integer> benefitsDetails = eventManager.createBenefitsDetails();
+        // then
+        for (String benefit : expectedBenefitDetails.split("\\|")) {
+            String[] parts = benefit.split(",");
+            String benefitName = parts[0];
+            int expectedAmount = Integer.parseInt(parts[1]);
+            assertThat(benefitsDetails.get(benefitName)).isEqualTo(expectedAmount);
+        }
+    }


테스트 코드에 반복문이 있어서 이해하기 조금 어려운것 같습니다. assertAll()을 활용해보시는걸 추천드립니다!

오 assertAll() 이란 기능이 있군요!!
확실히 for문을 하나하나 돌면서 assert문을 읽는거보다
한번에 assertAll()로 처리하는게 더 이해하기 쉬운 코드 같습니다.
좋은 조언 감사합니다!

inti0

예외처리와 ui로직도 계층을 나눈 부분이 인상깊네요 코드 잘 봤습니다!

inti0 · 2023-11-17T19:23:33Z

src/main/java/christmas/service/ReservationDayService.java

만약 대부분의 비즈니스 로직이 도메인에서 이루어진다면
컨트롤러와 도메인 사이에서 데이터를 정제해서
도메인에게 넘겨주는 계층을 서비스라고 불러도 괜찮지 않을까요?

inti0 · 2023-11-17T19:29:46Z

src/main/java/christmas/domain/Orders.java

+        if (sumOfMenuCountsExceedUpperLimit(orders)) {
+            throw InvalidOrdersException.of(EXCEED_MENU_COUNTS_UPPER_LIMIT.getMessage());
+        }
+    }


작성자분의 스타일이겠지만 메소드 명이 너무 긴 것 같습니다.
validateSum() -> 합계를 유효성검사 하는구나. 합계의 상한선이 있겠네 정도는
코드를 읽는 사람에게 충분히 기대해도 될 수준이라고 생각합니다.

말씀하신대로, 너무 긴 메소드도 가독성에 좋지 않을 것 같습니다.
축약하지 않고 메서드 이름에 모든 정보를 담고자 해서, 너무 길어진 감이 있는 것 같습니다.
다만 줄일 수 있되, 나타내는 의미까지 줄여버리는 것을 지양해야할 것 같아
네이밍에 대한 고민을 더 많이 해봐야곘다는 생각이 듭니다.
해당 메서드는 다시 읽어보니
menusExceedUpperLimit(orders) 으로도 의도가 잘 드러날 것 같네요..!

inti0 · 2023-11-17T19:38:19Z

src/main/java/christmas/domain/Orders.java

+        return orders.stream()
+                .mapToInt(Order::getTotalPrice)
+                .sum();
+    }


계산과 주문이라는 이 프로그램에서 큰 비즈니스 로직이 한 곳에서 수행되는 느낌을 받는 것 같아요.
이 객체가 주문을 다루는 만큼 할인 전 합계를 충분히 구할 수 있지만,
getter가 있는만큼 밖으로 주문을 빼낼 수 있기도 하고,
다른 계산하는 메소드들과 같이 있는게 응집성면에서도 좋을 것 같습니다.

inti0 · 2023-11-17T19:47:54Z

src/main/java/christmas/domain/ReservationDay.java

+    private ReservationDay(final int day) throws InvalidReservationDayException {
+        validate(day);
+        this.day = day;
+    }


정말 사소한 거지만 validate를 팩토리 메소드로 옮기면 생성자의 throws는 뺄 수 있을 것 같습니다.

사실 이 부분은 고민입니다.

날짜에 대한 validation은 어떠한 팩토리 메서드로 생성하더라도 수행되어야만 하는 공통 검증 로직인데

팩토리 메서드로 빼는 것이 나을까? 고민이 되네요. 🤔

다른 종류의 팩토리 메서드를 만들 때 실수할 수 있는 가능성의 여지를 둘 수 있지 않을까? 하는 생각입니다.

@inti0 님의 의견이 궁금합니다!

inti0 · 2023-11-17T19:59:13Z

src/main/java/christmas/domain/DayPerMonth.java

+
+    protected boolean isInAppropriateRange(final int day) {
+        return day >= DEFAULT_FIRST_DAY.getValue() && day <= DECEMBER_LAST_DAY.getValue();
+    }


표기법상 부정접두사 In-이 아닌 전치사 In으로 쓰인 것 같은데
!와 더해져서 혼동을 주는 것 같습니다.

!isInAppropriateRange()

음.. 다시 보니 좀 실수할 수 있겠다는 생각이 드네요, 어떻게 이름을 지어야할지 고민되네요.

inti0 · 2023-11-17T20:09:19Z

src/main/java/christmas/domain/constant/reservationday/ReservationDayConstant.java

+    public int getValue() {
+        return value;
+    }
+}


enum타입들이 constants라는 패키지 안에 들어있는데 어디 패키지에서 쓰이는 enum인지 다시 분류되고 있습니다.
각각의 패키지에 각각의 enum타입을 두는 것도 좋은 방법이라고 생각해요.
또 이런식으로 패키지 분리가 된다면 접근제어자도 선택할 수 있다는 장점도 있을 것 같습니다.

오, 말씀해주신 패키지 구성 방법 되게 좋은 것 같습니다.
좀 더 연관된 것을 묶은 느낌이 들어 직관적으로 보이네요.
좋은 의견 감사합니다!

inti0 · 2023-11-17T20:23:46Z

src/main/java/christmas/dto/TotalBenefitedAmountDto.java

+    public int getAmount() {
+        return amount;
+    }
+}


모든 dto들이 기본생성자와 똑같은 일을 하는 정적팩토리메소드를 사용하고 있는데
이는 단순 new가아닌 from으로 객체를 생성하고 싶기 때문인가요?
몇몇 객체는 기본 생성자가 private이고 몇몇은 public인데 특별한 이유가 있는지 궁금합니다.

네, of 나 from으로 생성하는 것이 좀 더 가독성이 좋다 판단하여 정적팩토리 메서드를 구현하고 있습니다.

다만, InputView와 OutputView, EventPlanner 같은 경우는 new로 생성하는게 너무 익숙해져서 따로 생각을 안했긴 합니다.

통일성에 대한 고민이 필요하다 생각이 드네요.. 기준을 한번 세워봐야겠습니다.

inti0 · 2023-11-17T20:31:33Z

src/main/java/christmas/view/output/constant/OutputMessageConstant.java

+    GREETING("안녕하세요! 우테코 식당 " + CURRENT_PROMOTION_MONTH + "월 이벤트 플래너입니다."),
+    INSERT_RESERVATION_DAY(CURRENT_PROMOTION_MONTH + "월 중 식당 방문 날짜는 언제인가요? (숫자만 입력해 주세요!)"),
+    INSERT_ORDERS("주문하실 메뉴를 메뉴와 개수를 알려 주세요. (e.g. 해산물파스타-2,레드와인-1,초코케이크-1)");
+


String.format("안녕하세요! 우테코 식당 %d월 이벤트 플래너입니다.",CURRENT_PROMOTION_MONTH);

이것도 고려해볼 수 있겠네요.

갠적으로 말씀해주신 String.format을 활용하는 것이 더 잘 읽히네요!
좋은 의견 감사합니다 :)

inti0 · 2023-11-17T20:41:56Z

src/test/java/christmas/parser/InputParserTest.java

+                .hasMessageContaining(
+                        String.format(BasicInputExceptionMessageFormat.TOO_LONG_WITH_BLANKS_FORMAT.getFormat(),
+                                ORDER_SYMBOL.getSymbol()));
+    }


이런건 전혀 생각 못했는데 신기하네요. parseOrders의 어떤부분 때문에 글자수 제한이 생기는 건가요?

#1 (comment)

이 부분의 의견과 동일합니다!

chosunghyun18

미션 고생하셨습니다.
다른분들이 이미 많이 리뷰를 남기시어 중복된 리뷰는 피하고자 했습니다.

고생하셨습니다

chosunghyun18 · 2023-11-18T02:18:19Z

src/main/java/christmas/domain/DayPerMonth.java

+
+import christmas.domain.exception.InvalidReservationDayException;
+
+public abstract class DayPerMonth {


abtsract class 를 사용하신 이유가 궁금합니다.

추후 1월, 2월 등 차후 이벤트 날짜에 대한 제약사항의 틀을 제시하기 위함입니다!

h-beeen · 2023-11-21T07:46:56Z

src/main/java/christmas/Application.java

+        EventPlanner eventPlanner = new EventPlanner(new InputView(), new OutputView());
+        eventPlanner.execute();


EventPlanner 객체를 컨트롤러 성향이 아닌, main과 같은 레벨로 두신 점이 인상깊었습니다.

Application.main과 EventPlanner를 구분하신 이유가 있을까요?
어떤 장점을 얻을 수 있을 지 궁금합니다.

Application은 최상단의 클래스로,
EventPlanner의 생성과 실행시키는 것을 담당합니다.
자원의 생성 및 실행, 관리 및 설정을 총괄한다 생각합니다. ( 추후 Application에서 모든 클래스에 대한 의존성 주입을 해주는 것도 고려하고 있습니다. )

EventPlanner는 말 그대로 플래너 서비스 그 자체입니다.
이 서비스가 단일로 존재하면 Application과 큰 차이가 없겠지만
추후 다른 서비스가 추가되는 경우를 생각하면 Application과 EventPlanner를 구분해놓은 것이 의미가 있을 것이라 생각합니다!

@junslog 추후에 어떤 서비스가 어떻게 추가되는 걸 상정하셨나요?

@junslog 추후에 어떤 서비스가 어떻게 추가되는 걸 상정하셨나요?

이벤트 결과를 다 보여주고 난 뒤에 주문 데이터를 받아와서 주문으로 연결해주는 서비스를 예시로 들 수 있을 거 같습니다만
이렇게 얘기하는건, 미리 상정했다고 말씀 못드리겠습니다.

말씀 듣고 고민해보니 '확장성'이라는 가능성을 열어두면서 구현하는건 좋으나,
확장성 자체에 초점을 맞춰서 구현을 하는건 그리 좋은 방식이 아니라 생각이 듭니다.

확장성을 핑계로 구조를 틀어버린 느낌..?? 사실 누군가 제 코드에 대한 질문을 할 때,
"확장성을 고려했어요~ "하면 되게 뭉뚱그릴 수 있는 답변으로 무언가 중요한걸 회피하는거 아닐까? 하는 생각이 듭니다.
개발 시 확장성을 우선순위가 1순위로 두는게 아니라, 2~3순위로 고려해야하는 것 같다는 느낌이 드네요.

좋은 개발은 뭘까?를 생각을 더 해보게 되는 말씀이었습니다. 감사합니다!

h-beeen · 2023-11-21T07:47:46Z

src/main/java/christmas/controller/OrdersController.java

+        try {
+            Map<String, Integer> orders = askToInsertOrders();
+            return ordersService.createOrders(orders);
+        } catch (BasicInputException | OrdersInputException | InvalidOrdersException e) {


람다문 내부 변수에 대해서도 e가 아니라 예컨데 exception과 같이 구체적인 변수 명으로 선언하면 더 좋을 것 같아요!

h-beeen · 2023-11-21T07:50:29Z

src/main/java/christmas/domain/ReservationDay.java

+    private final int day;
+
+    private ReservationDay(final int day) throws InvalidReservationDayException {
+        validate(day);


지금까지 줄곧 직관적인 메소드 네이밍을 작성하려고자 많이 노력하신 흔적이 엿보였습니다! 이런 검증메소드의 경우도 validateDayRange와 같이 조금 더 직관적으로 네이밍 하면 가독성이 올라갈 것 같아요!

h-beeen · 2023-11-21T07:53:13Z

src/main/java/christmas/domain/constant/orders/Menu.java

+    public String getName() {
+        return name;
+    }
+
+    public int getPrice() {
+        return price;
+    }


@Getter는 통상 코드의 가장 밑부분에 위치하는 것이 일반적이라고 알고 있어요.
비즈니스 로직을 위로 올리고, Getter로직을 내리는 방법이 가독성을 조금 더 높여줄 것이라 생각해요.

확실히 이 부분은 컨벤션에 가깝다 생각합니다.
갑자기 getter가 나오면 오잉? 싶을 것 같아요.

h-beeen · 2023-11-21T07:54:03Z

src/main/java/christmas/domain/constant/orders/Menu.java

+        return foodType.isWeekendPromotionApplicable();
+    }
+
+    public static Menu searchByName(final String name) {


String이라는 객체에 final을 파라미터로 적용해주셨어요.
final을 붙여 객체의 주솟값을 고정하는 행위가, 예컨데 의도하신 바와 같이 해당 String의 내용의 불변성을 확보할 수 있을까요?

어차피 새로운 지역변수로 초기화되어 원본 객체와 독립된 스택에 존재했다 사라질 변수인데,
굳이 final을 사용할 필요 없겠다는 생각이 드네요.
물론 메서드 내에서 파라미터 값의 재할당을 금지하긴 하지만, 그야 String의 메서드를 통해 변환되어 새로 생성된 String 객체를 재할당하면 되는 문제이니.. 굳이 final을 사용할 필요 없겠네요! 좋은 지적 감사합니다 :)

h-beeen

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

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

감사합니다 :)

kmw2378

4주차 과제하시느라 수고 많으셨습니다!

kmw2378 · 2023-11-28T13:06:33Z

src/main/java/christmas/EventPlanner.java

+    public EventPlanner(InputView inputView, OutputView outputView) {
+        this.outputView = outputView;
+        reservationDayController = new ReservationDayController(inputView, outputView);
+        ordersController = new OrdersController(inputView, outputView);
+        eventManagerService = new EventManagerService();
+        outputView.printGreetingMessage();
+    }


생성자에서 많은 일을 하는 것 같습니다! 출력같은 부분은 다른 메서드에서 하는것도 좋을 것 같아요!

추가로 다른 필드값을 파라미터로 받아 외부에서 주입하는 방식은 어떨까요?

execute() 부분의 코드를 간결하게 짜고 싶은 욕심에
생성자에 출력 로직을 부여하였습니다. 이 부분은 생성자의 역할이 아니라 생각이 드네요.

또한, 의존성 주입 방법 정말 괜찮은 것 같습니다.
new 로 생성되는 것이 클래스 군데군데 퍼져있으면 관리하기 어려울 것 같아요.
좋은 의견 감사합니다!

kmw2378 · 2023-11-28T13:08:33Z

src/main/java/christmas/controller/OrdersController.java

+        try {
+            Map<String, Integer> orders = askToInsertOrders();
+            return ordersService.createOrders(orders);
+        } catch (BasicInputException | OrdersInputException | InvalidOrdersException e) {


커스텀 예외 클래스 모두 IllegalArgumentException 을 상속 받으면 catch 문이 더 간단해 질 것 같아요! �이후 다른 커스텀 예외가 추가되는 상황에 대비하기도 좋을 것 같습니다!

명시적으로 어떤 예외가 발생할 수 있는지에 대해 기술하려 하였습니다!
물론.. 다른 커스텀 예외가 발생하면, 좀 일이 커질 것 같네요.
그걸 생각해보니... 확실히 IllegalArgumentException으로 통일해서 받는게 좋은 방법 같습니다 :)
감사합니다!

kmw2378 · 2023-11-28T13:09:33Z

src/main/java/christmas/controller/OrdersController.java

+        }
+    }
+
+    private Map<String, Integer> askToInsertOrders() throws BasicInputException, OrdersInputException {


메뉴 입력 시 이름, 수량 외에 다른 값이 추가된다면 Map<K, V> 사용이 힘들 거 같습니다! DTO 클래스는 어떨까요?

헉 지금까지 Input Data로 여러 개의 데이터를 받아오는 경우가 없다보니
싹 다 원시값 또는 컬렉션 자료구조로 커버했었습니다.

Dto로 만들면 여러 개의 input값을 한번에 담을 수 있고,
input값에 의미있는 이름을 명시할 수 있어서 좋을 것 같네요. 좋은 의견 감사합니다 :>

kmw2378 · 2023-11-28T13:10:30Z

src/main/java/christmas/domain/DayPerMonth.java

+    public DayPerMonth() {
+    }


기본 생성자를 별도로 만드신 이유가 있을까요? 생성자가 없으면 컴파일러가 기본 생성자를 만들어줍니다!

앗 이 부분은 원래 생성자 내에서 검증 로직을 처리하는 것을 넣어두었다가
리팩터링하면서 바깥으로 뻈었는데, 그 이후로 잔재로 남아있던 것 같습니다.
짚어주셔서 감사합니다!!

kmw2378 · 2023-11-28T13:10:51Z

src/main/java/christmas/domain/DayPerMonth.java

+    public DayPerMonth() {
+    }
+
+    protected void validate(final int day) {


어떤것을 검증하는지 메서드명으로 표현하면 좋을 것 같아요!

validateDayRange() 와 같은 이름으로 지을 수 있었겠네요.
좋은 조언 감사합니다!!!

kmw2378 · 2023-11-28T13:12:57Z

src/main/java/christmas/domain/Orders.java

+    public List<Order> getOrders() {
+        return Collections.unmodifiableList(orders);
+    }


Collections.unmodifiableList() 도 완전한 불변성을 보장하진 않습니다. getter 를 제공하지 않는 방법은 어떨까요?

결국 원본객체가 변경되면, 파생된 모든 객체가 영향을 받으니.. 완전한 불변성은 아니겠네요!

getter를 최대한 지양하려 했지만, 이 부분이 아직 쉽진 않은 것 같습니다.

한번 없애는 방향으로 리팩터링 시도해보겠습니다! 감사합니다.

junpakPark

안녕하세요 준준님! 5기 준팍입니다.
코드 잘 봤습니다 ㅎㅎ

기능명세서에서 전체적인 흐름을 잘 표현해주셔서 이해하기 편했습니다.👍👍
PR 본문에는 미션 간 고민한 부분도 잘 남겨주셨구요 ㅎㅎ

다른 분들이 다양하게 리뷰를 남겨주셨는데요,
제 의견도 그런 의견들 중 하나일 뿐이니 너무 휩쓸리지 않으셔도 될 것 같습니다 ㅎㅎ

전체적으로 오버엔지니어링이라는 생각이 많이 들었습니다.
EventPlanner와 두개의 컨트롤러라던가,
수없이 많은 DTO,
생성자와 완전히 동일한 일을 하는 정적 팩터리 메서드 등등...

Console 프로그래밍을 하는데 Spring WebMVC의 구조를 따라하려다 보니
어색한 지점이 더 많아졌다는 생각이 드네요.

MVC 패턴의 Controller와 레이어드 아키텍처에서 말하는 Controller는
정말 같은 걸까요?

적정한 수준의 개념을 도입하여 적정한 수준으로 구현하는 것이
오히려 더 깔끔한 코드를 작성하는 일이 될 수 있을 것 같습니다.

여러 6기 지원자분들께서 이미 리뷰를 많이 남겨주셔서
코멘트가 많지는 않은데요,
기존의 피드백에 좋은 점이 많으니까 다방면으로 흡수해보시는 것도 좋을 것 같습니다 ㅎㅎ

수고 많으셨습니다!

junpakPark · 2023-12-02T15:03:41Z

docs/README.md

@@ -0,0 +1,614 @@
+# 🎅 크리스마스 프로모션


PR 본문에 이번 미션에서 고민한 점도 명시해주시고
기능 명세도 깔끔하게 작성해주셨네요 👍👍👍

junpakPark · 2023-12-02T15:05:19Z

src/main/java/christmas/Application.java

+        EventPlanner eventPlanner = new EventPlanner(new InputView(), new OutputView());
+        eventPlanner.execute();


@junslog 추후에 어떤 서비스가 어떻게 추가되는 걸 상정하셨나요?

junpakPark · 2023-12-02T15:06:08Z

src/main/java/christmas/EventPlanner.java

+        reservationDayController = new ReservationDayController(inputView, outputView);
+        ordersController = new OrdersController(inputView, outputView);
+        eventManagerService = new EventManagerService();
+        outputView.printGreetingMessage();


굿굿 생성자에서 빼주시는게 더 좋을 것 같습니다.

junpakPark · 2023-12-02T15:07:29Z

src/main/java/christmas/EventPlanner.java

+    private final OrdersController ordersController;
+    private final EventManagerService eventManagerService;
+
+    public EventPlanner(InputView inputView, OutputView outputView) {


어떤 인스턴스 변수는 파라미터로 받고 있고,
어떤 인스턴스 변수는 생성자 내부에서 객체를 생성하고 있네요
어떤 기준으로 이렇게 나누셨나요?

inputView와 outputView는 최초 한번 생성되게 하고
제어의 흐름에 따라 미리 생성된 InputView, outputView를 사용하게 하려 하였습니다.
그 외의 인스턴스 변수는 객체가 생성될 때 새로 생성하려 하였습니다.

inputView, outtputView는 생성비용이 많이 든다 생각하여 싱글톤으로 구현해야한다 생각했고,
오버엔지니어링을 지적해주셔서 살짝 말씀드리기 고민되지만
멀티 쓰레드 환경에서 여러 사용자가 사용할 때, 해당 사용자를 위한 서비스, 컨트롤러들을 생성하려는 생각을 가지고 구현하였습니다.

junpakPark · 2023-12-02T15:20:05Z

src/main/java/christmas/EventPlanner.java

+        printIntroMessage(reservationDay);
+        printOrderedMenus(orders);
+        printTotalAmountWithNoDiscount(orders);
+        printGiftMenu(orders);
+        printBenefitsDetails(eventManager);
+        printTotalBenefitedAmount(eventManager);
+        printEstimatedAmountWithDiscount(eventManager);
+        printEventBadge(eventManager);


해당 메서드가 controller에 있어서 어떤 식으로 세밀한 제어 및 유지보수가 가능할까요?

해당 메서드를 OutputView에 public으로 선언하고,
내부 메서드들은 private으로 만들었을 때는 어떤 문제가 있을까요?

ISP 원칙에서 말하는 인터페이스란 무엇인가요?

junpakPark · 2023-12-02T15:23:14Z

src/main/java/christmas/domain/EventManager.java

+        this.orders = orders;
+    }
+
+    public static EventManager of(ReservationDay reservationDay, Orders orders) {


생성자랑 하는 일이 동일한데 굳이 정적 팩터리 메서드를 써야할까요?

이 부분 고민해보았는데,
정적 팩터리 메서드를 쓰는 의미가 없는 것 같습니다.
지금까지 써보고, 공부하며 느꼈던 정적 팩터리 메서드는

메서드 이름을 통해 생성의 로직에 대한 직관적 유추 가능

생성 로직, 방식에 따라 다른 이름을 제공해서 생성자 오버로딩 / 빌더 패턴보다 나은 객체 생성에 대한 분리 및 명확성

이 두가지인데, 말씀해주신 코드에서 제가 쓴 방식은 그저 of라는 이름을 써서 생성하는 것 이외에 다른 의미가 없는 것 같습니다.

junpakPark · 2023-12-02T15:32:21Z

src/main/java/christmas/domain/constant/event/EventNumberConstant.java

@@ -0,0 +1,19 @@
+package christmas.domain.constant.event;
+
+public enum EventNumberConstant {


연관성이 있는 상수는 static final 대신 enum을 활용한다
이 문구 때문에 만든 Enum인가요?

해당 Enum의 상수들은 각기 다른 클래스에서 쓰고 있고
한 상수를 여러 클래스에서 사용하는 경우도 별로 없군요...

정말 연관성이 높은거 맞을까요?

연관성이 없는 것 같습니다.

차라리 EventPlanner 와 같은 객체 내에서
private static final로 명시해놓는게 훨씬 더 명확하고 유지보수하기 쉬울 것 같습니다.
다시 생각해보니 어플리케이션을 위한 코드라기 보다는, 코드를 쓰기위해 어플리케이션을 끼워맞춘 느낌이네요..
지적해주셔서 감사합니다!

junpakPark · 2023-12-02T15:37:09Z

src/main/java/christmas/domain/constant/orders/OrdersConstant.java

+package christmas.domain.constant.orders;
+
+public enum OrdersConstant {
+    MAX_MENU_COUNTS(20);


굳이 Enum으로 분리할 필요가 있을까요?

없습니다!

그냥 Orders 에 private static final 상수로 관리하는게 훨씬 낫겠습니다..ㅎㅎ

junpakPark · 2023-12-02T15:39:35Z

src/main/java/christmas/dto/BenefitsDetailsDto.java

+    public static BenefitsDetailsDto from(Map<String, Integer> benefitsDetails) {
+        return new BenefitsDetailsDto(benefitsDetails);
+    }


개인적으로는 @jisu-om 님 처럼 하지 않는다면 DTO에 굳이 정적 팩터리 메서드가 필요한가 싶네요 ㅎㅎ
생성자랑 하는일이 동일하고 from이란 네이밍도 딱히 더 유의미한 정보를 주지 않군요

junpakPark · 2023-12-02T15:41:18Z

src/main/java/christmas/dto/OrderedMenusDto.java

+    public static OrderedMenusDto from(Map<String, Integer> orders) {
+        return new OrderedMenusDto(orders);
+    }


DTO, 자료구조와 같은 경우에는 내부 구조를 외부에 노출하는것이 당연하므로
디미터 법칙을 적용하지 않습니다.

junslog added 30 commits November 12, 2023 21:05

docs: add feature list

cfb3c72

feat: add skeleton code for application

d7f9b86

feat: add feature to print statement for user to insert reservation day

61aa904

feat: add feature to check user input about reservation day is in app…

ba4d32b

…ropriate range

feat: add feature for user to insert reservation day

31990b9

feat: add feature to check user input about reservation day is not empty

c3cafdf

fix: fix error when invalid input about reservation day is inserted

851a890

feat: add feature to remove blank in user input about reservation day

f941f33

feat: add feature to check user input length about reservation day is…

ef06b19

… not more than 2 without blank

feat: add feature to check user input about reservation day is conver…

221e83b

…tible to numeric type

feat: add feature to check user input about reservation day is positi…

0e47ef0

…ve integer

docs: rearrange feature list

5fbe307

feat: add feature to print statement for user to insert orders

a9aca2e

refactor: indicate method handling exception throws specific type of …

d864f3c

…exception

feat: add feature for user to insert orders

00f7558

docs: add constraint to user input about orders

c904585

feat: add feature to check user input about orders is in appropriate …

4d89124

…range

docs: add constraint to user input about orders

47ee9c2

feat: add feature to remove blank in user input about orders

863c565

feat: add feature to check user input about order is not empty

49830ad

docs: update feature list

b1616ac

feat: add feature to check all user input length is under limit(2000)…

245b009

…, even considering that it contains blanks

feat: add feature to check user input about order contains delimiter(…

6e68e48

…'-')

feat: add feature to check user input about menu name length is in ap…

55ba4f0

…propriate range

refactor: change input constant values to be enum

937363f

refactor: change empty input notification to contain information abou…

5745b52

…t what content user input missed

feat: add feature to check user input about menu count is not empty

4d81ca6

feat: add feature to check user input about menu count length is not …

fa00100

…more than 2

feat: add feature to check user input about menu name exists in menu …

f011be9

…list

docs: change constraint about error message format to meet requirement

8f75c94

junslog added 15 commits November 15, 2023 13:52

refactor: make ApplicationConstant finalized and remove unnecessary f…

e7944ad

…inal keyword

refactor: remove redundunt ERROR_PREFIX addition in exception messages

e46096b

test: add test to check EventManager domain logic

c4805f3

test: add test to check Menu domain logic

039a1b1

test: add test to check EventBadge domain logic

1899ecf

test: add test to check ChristmasPromotion domain logic

f48428a

refactor: extract duplicated part to method in EventManagerTest

0fd035a

test: add test code for InputParser

d97bae5

test: add test code for DayInputValidator

ce6e107

test: add test code for OrdersInputValidator

3f98a76

test: add test code for MenuNameInputValidator

c8ba2ef

test: add test code for MenuCountInputValidator

22557d6

test: add test code for OrderInputValidator

067626c

refactor: remove magic literal in test code

3571d80

docs: update README to explain about application

6acfacf

imbesky reviewed Nov 16, 2023

View reviewed changes

gomudayya reviewed Nov 16, 2023

View reviewed changes

jisu-om reviewed Nov 16, 2023

View reviewed changes

phk1128 reviewed Nov 17, 2023

View reviewed changes

inti0 reviewed Nov 17, 2023

View reviewed changes

chosunghyun18 reviewed Nov 18, 2023

View reviewed changes

h-beeen reviewed Nov 21, 2023

View reviewed changes

kmw2378 reviewed Nov 28, 2023

View reviewed changes

junpakPark reviewed Dec 2, 2023

View reviewed changes


		import static christmas.domain.constant.event.EventNumberConstant.CHRISTMAS_PROMOTION_INCREASING_AMOUNT;

		public enum ChristmasPromotion {


		import christmas.domain.exception.InvalidReservationDayException;

		public abstract class DayPerMonth {

		EventPlanner eventPlanner = new EventPlanner(new InputView(), new OutputView());
		eventPlanner.execute();

		@@ -0,0 +1,19 @@
		package christmas.domain.constant.event;

		public enum EventNumberConstant {

Conversation

junslog commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

이번 미션에 대해서는 다음과 같은 고민을 위주로 했어요.

1) Orders 와 ReservationDay 의 협업을 어떻게 처리하지?

2) 리드미를 어떻게 작성해야할까? ( Code 탭의 가서 docs 참조해주시면 감사하겠습니다! )

3) 불변 컬렉션, final활용

4) 예외의 커스텀화 및 메서드가 예외를 throw하는 것을 명시화

5) 도메인별 역할 분리

6) 1차 검증은 InputView 에서, 2차 이후 검증은 도메인에서

Uh oh!

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

junslog Nov 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jisu-om 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!

junslog Nov 29, 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

junslog commented Nov 16, 2023 •

edited

Loading

1) `Orders` 와 `ReservationDay` 의 협업을 어떻게 처리하지?

2) 리드미를 어떻게 작성해야할까? ( `Code` 탭의 가서 `docs` 참조해주시면 감사하겠습니다! )

3) 불변 컬렉션, `final`활용

4) 예외의 커스텀화 및 메서드가 예외를 `throw`하는 것을 명시화

6) 1차 검증은 `InputView` 에서, 2차 이후 검증은 도메인에서

junslog Nov 29, 2023 •

edited

Loading

junslog Nov 29, 2023 •

edited

Loading

junslog Nov 29, 2023 •

edited

Loading

junslog Nov 29, 2023 •

edited

Loading