[크리스마스] 코드 리뷰 부탁드립니다! #1

youngsu5582 · 2023-11-16T05:48:43Z

🎄 Java-Christmas

4주차 까지 모든 과제 마무리 하신다고 다들 정말 수고하셨습니다!!
모두가 얼마 안 남은 크리스마스 까지 , 행복하시면 좋겠습니다.🥰🥰

이번 과제는 , 특히나 객체 분리와 완벽한 코드를 구현하기 더욱 어려웠던 거 같습니다.
부족한 부분을 마구마구 꾹꾹 쑤셔 주시면 좋겠습니다! 감사합니다!!
#무조건 반사 #욕설 빼고 다가능

고민한 부분

1. UI 로직은 철저하게 분리해야한다.

MVC 패턴에서는 Controller가 View와 Model을 호출하여 관리하는 방법으로 구현해야 합니다.

2. Event 를 묶어서 관리하자.

Event 를 모두 공통적으로 묶어서 관리를 하고 싶었습니다.
-> Event 는 혜택 확인 , 혜택 타입이 제각각
=> 제네릭 타입 과 extends 를 통해 해결하자!
DiscountEvent 는 무조건 날짜를 통해 검증 <-> PresentEvent 는 금액을 통해 검증
�DIscountEvent 는 금액을 리턴 <-> PresentEvent 는 메뉴를 리턴

폴더 구조

christmas
├── constant  상수들을 모아놓은 디렉토리
├── controller  입출력,서비스 호출을 담당하는 컨트롤러 디렉토리 
├── domain 도메인 관련 디렉토리
│   ├── badge  
│   ├── category 
│   ├── date  
│   ├── event  
│   │   ├── discount 
│   │   └── present 
│   ├── menu 
│   ├── order 
│   └── reward
├── dto DTO를 모은 디렉토리
├── exception  예외를 모은 디렉토리
│   └── message  예외 메시지 모은 디렉토리
├── factory  팩토리를 모은 디렉토리
├── lib  상속 관련 객체 모은 디렉토리
│   ├── event 
│   └── exception
├── service  도메인 로직을 담당하는 서비스 디렉토리
├── util  유틸 클래스 모은 유틸 디렉토리
└── view  입출력 담당하는 뷰 디렉토리
    └── message 입출력 메시지 모은 디렉토리

Test Coverage

Test Case

입력 담당 하는 InputView 출력 담당 하는 OutputView

Parser 내 , 문자열 을 숫자로 변환 해주는 parseInfoToNumber 추가 모든 Error 의 상위 Class 인 CustomException 추가 이에 따른 , 테스트 코드 추가 - 숫자 문자열 을 입력 하는 경우 - 숫자가 아닌 문자열 을 입력 하는 경우

Calendar 내 , 날짜에 따른 해당 요일을 반환 해주는 calculateDayOfWeek 추가 요일을 결정 하는 DayOfWeek enum 추가 DayOfWeek 의 에러를 담당 하는 DayOfWeekException 추가 이에 따른 , 테스트 코드 추가 - 첫 날이 금요일 이고 , 7일이 목요일 인 경우 - 첫 날이 월요일 이고 , 22일이 월요일 인 경우

Date Domain 생성 부가 적인 DateConstant , DateException , DateExceptionMessage 같이 추가 이에 따른 , 테스트 케이스 구현 - 정상 적인 날짜를 입력 하는 케이스 - 범위 밖의 날짜를 입력 하는 케이스

Category , Menu Domain 생성 Category 에 해당 하는 Drink , Dessert , MainDish , Appetizer 추가 이에 따른 , 테스트 케이스 구현 - 카테고리 에 해당 하는 클래스 의 요소 받는 케이스

MenuCatalog 내 , 이름에 따라 , Menu 를 반환 해주는 searchFromMenuName 기능 추가 이에 따른 , 테스트 케이스 구현 - 음식명을 통해 메뉴를 받는 케이스 - 없는 음식명을 입력 하는 케이스

RequestOrder Domain 생성 Parser 에 parseInfoWithSeparator 에 함수 추가 이에 따른 , 테스트 케이스 구현 - 정해진 형식에 맞게 입력 하는 케이스 - 정해진 형식에 맞지 않는 문자열 입력하는 케이스 - 1보다 작은 값을 입력하는 케이스

Order Domain 생성 OrderExceptionMessage 에 DUPLICATE_MENU 추가 이에 따른 , 테스트 케이스 구현 - 정해진 형식에 맞게 입력 하는 케이스 - 정해진 형식에 맞지 않는 문자열 입력 하는 케이스 - 중복된 메뉴명 을 입력 하는 케이스

OrderResult,OrderInfo,MenuInfo Domain 생성 Category 식별 위해 MenuCatalogTest 함수 리턴 타입 변경 이에 따른 , 테스트 케이스 구현 - 메뉴명 과 개수를 입력 하는 케이스 - 없는 메뉴명 을 입력 하는 케이스

Bill 도메인 생성 OrderConstant , OrderExceptionMessage 에 관련 값 추가 이에 따른 , 테스트 케이스 구현 - 주문 리스트 입력 하는 케이스 - 주문 개수가 최대 주문 개수를 초과 하는 케이스 - 음료 메뉴만 주문 하는 케이스

해당 요일이 주말 인지 확인 하는 isWeekend 함수 추가 해당 요일이 평일 인지 확인 하는 isWeekday 함수 추가 이에 따른 , 테스트 케이스 구현 - 값을 넣어 요일을 받는 케이스 - 금요일 과 토요일 이 주말 인지 확인 하는 케이스 - 월요일 , 화요일 , 수요일 ,목요일 , 일요일 이 평일 인지 확인 하는 케이스

이벤트 공통 관리를 위한 추상 Class 인 Event 추가 증정 이벤트 를 위한 PresentEvent 추가 할인 이벤트를 위한 DiscountEvent 추가

Object 의 타입을 하위 클래스 에서 지정 하기 위해 제네릭 타입으로 변경

EventConstant 추가 MenuCatalog 내 , 이름 비교 API 수정 이에 따른 , 테스트 케이스 구현 - 평일인 케이스 - 주말인 케이스 - 주말 이지만 메인 디쉬가 없는 케이스

EventConstant 추가 이에 따른 , 테스트 케이스 구현 ChristmasDiscountEvent - 25일 이전 케이스 - 25일 이후 케이스 WeekdayDiscountEvent - 주말인 케이스 - 평일인 케이스 - 평일 이지만 디저트 가 없는 케이스

EventConstant 추가 이에 따른 , 테스트 케이스 구현 SpecialDiscountEvent - 특정 일인 케이스 - 특정 일이 아닌 케이스 ChampagnePresentEvent - 총 주문 금액이 지정 금액을 넘는 케이스 - 총 주문 금액이 지정 금액 넘지 못하는 케이스

DiscountEventReward,PresentEventReward Domain 추가 Event 내 리턴 타입도 유동적 사용 위해 제네릭 타입 으로 변경 DiscountEvent,PresentEvent 리턴 타입도 변경

DiscountEventReward,PresentEventReward 로 변경 이에 따른 , 테스트 케이스 리턴 타입도 수정

공통 적으로 사용 위한 상위 클래스 EventReward 추가 DiscountEventReward, PresentEventReward 들이 상속 상속을 위해서 record -> class 로 변경

Reward , RewardDto 추가 이에 따른 , 테스트 케이스 구현 - Reward 를 만드는 케이스 - 보상이 분류 되어 들어간 지 확인 하는 케이스 - 총 할인 금액 계산이 일치 한지 확인 하는 케이스

총 혜택 금액 따라 제공 뱃지 결정 하는 Badge Domain 추가 상수 관리 하는 BadgeConstant 추가 이에 따른 , 테스트 케이스 구현 - 산타 뱃지 기준치 를 만족 하는 케이스 - 트리 뱃지 기준치 를 만족 하는 케이스 - 별 뱃지 기준치 를 만족 하는 케이스 - 기준치 를 만족 못하는 케이스

모든 기능 관리 하는 Gamecontroller 생성 Date 생성을 담당 하는 DateController , DateService 생성 InputView 와 OutputView 에 함수 추가 OutputViewMessage , InputViewMessage 에 상수 추가

Order,Bill 생성을 담당 하는 OrderController , OrderService 생성 InputView 와 OutputView 에 함수 추가 OutputViewMessage , InputViewMessage 에 상수 추가

OrderService 누락 되어 파일 추가

숫자를 세 자리 마다 , 로 포맷 해주는 formatNumber 추가 숫자를 포맷 후 , - 를 붙히는 formatNegativeNumber 추가

Reward 생성을 담당 하는 EventController , EventService 생성 OutputView 에 함수 추가 , OutputViewMessage 에 상수 추가 Reward 에 0원이면 , 추가 되지 못하게 로직 추가

TAPAS 가격 5000 -> 5500 수정 EVENT_NAME 수정 DUPLICATE_MEMU -> NOT_EXIST_MENU 변경

Badge 생성 담당 하는 BadgeController , BadgeService 생성 OutputView 에 함수 추가 , OutputViewMessage 에 상수 추가

숫자가 0 이상 일시 , - 부착 숫자가 0일 시 , - 부착 하지 않음

ORDER_SEPARATOR , REQUEST_SEPARATOR 상수 파일 분리 메뉴 입력중 발생 예외 사항 요구사항 맞게 메시지 변경 변경에 따라 , 테스트 파일도 수정

confirmRequestOrders , confirmOrders 중 생성 부분 함수 생성 하여 분리

Discount 를 통해 , 할인된 값만 모아서 반환하는 getter 추가

Parser 중 에러 발생 시 , INVALID_DAY 발생 하게 변경

함수명 변경 함수 로직 중 구분 위한 공백 추가 에러 메시지 공백 추가

Calendar 의 setStartDate 함수 제거 - 테스트 케이스 에서도 제거 MenuItem 의 getPrice 함수 제거 - 상속 하는 enum 에서도 제거

스택 오버플로우 & 최적화 위한 재귀 함수 제거 Exception Chaining 위해 발생 Exception 포함해 새로 생성 - CustomException 에도 Exception 포함해 생성 하는 메소드 기능 추가

스택 오버플로우 & 최적화 위한 재귀 함수 제거 Exception Chaining 위해 발생 Exception 포함해 새로 생성

더 보기 쉽게 하기 위해 폴더별 이벤트 분리

이벤트 명도 상수 파일에서 관리

기존 event 폴더 reward 로 이름 변경 밖에 있는 event 폴더 domain 내 event 로 이동

1. 표준 Java 라이브러리 import 2. 서드 파티 라이브러리 import 3. 다른 패키지 import 4. 정적 import

present 부분 상수 처리 누락된 import 문 순서 변경 TODO 제거

CaseTest 내 , 다른 케이스 추가 Application 내, 예외 케이스 추가 - 음료 단독 주문 케이스 - 메뉴 초과 주문 케이스

Controller 주입 해주는 ControllerFactory Service 주입 해주는 ServiceFactory EventList 주입 해주는 EventFactory 이에 따라 , 컨트롤러 서비스 단 수정

기존 : EventList 변경 : InProgressEventList EventList 는 Event 의 배열명 으로도 사용 가능 - 이벤트 목록 분리 하는 케이스 추가

임시 이름인 Temp 로 선언한 케이스 명 변경 - ApplicationTest 에서 메뉴 0 시키는 케이스 추가

가독성 위해 , Test/Domain 내 파일들 폴더로 이동

고민한 부분에 대한 내용 정리 폴더 구조 이미지 첨부 테스트 커비리지 결과 첨부 입출력 , 에러 부분 상세 설명 추가

aammddkkzxc · 2023-11-19T05:11:37Z

src/main/java/christmas/controller/GameController.java

+    public void run() {
+        printWelcomeMessage();
+
+        Date date = dateController.acceptVisitDate();
+        Bill bill = orderController.acceptOrder();
+        printOrderResult(date, bill);
+
+        Reward reward = eventController.confirmReward(date, bill);
+        RewardDto rewardDto = reward.toDto();
+
+        printRewardResult(rewardDto);
+        printFinalCheckoutPrice(bill, rewardDto);
+
+        printBadgeResult(badgeController.grantBadge(rewardDto));
+    }


취향 차이 라고 생각하긴 하지만, 객체를 생성해주는 라인들을 하나로 모으고 출력print메서드들을 전부 모아주는것도 하나의 방법일 것 같아요

printWelcomeMessage(); Date date = dateController.acceptVisitDate(); Bill bill = orderController.acceptOrder(); Reward reward = eventController.confirmReward(date, bill); RewardDto rewardDto = reward.toDto(); printRewardResult(rewardDto); printOrderResult(date, bill); printFinalCheckoutPrice(bill, rewardDto); printBadgeResult(badgeController.grantBadge(rewardDto));

이런식으로요

요구사항을 생각해서 ,
생성하고 같이 출력하는 식으로 생각했는데 ,
이렇게 print 구문 모아놓은거도 괜찮네요!!

aammddkkzxc · 2023-11-19T05:18:33Z

src/main/java/christmas/domain/category/Appetizer.java

+import christmas.domain.menu.Menu;
+import christmas.domain.menu.MenuItem;
+
+public enum Appetizer implements MenuItem {


인터페이스를 사용하셨군요!! 이렇게 되면 유지 보수에 더 좋을거라는 생각이 드네요 멋져요

enum 인터페이스 사용은 생각 못했네요! 저는 Category를 enum으로 하나 더 만들었는데, 인터페이스 사용이 더 깔끔해 보여서 좋은 것 같습니다!

aammddkkzxc · 2023-11-19T05:21:15Z

src/main/java/christmas/domain/date/Date.java

+    }
+
+    private static void validateDayRange(int day) {
+        if (day < START_DAY || day > END_DAY) {


저도 리뷰하다가 배운 내용인데 localDate.of() 를 사용하는 방법도 있어서 추천 드려요!!

저도 보고 신기해서 놀랬습니다..

aammddkkzxc · 2023-11-19T05:23:47Z

src/main/java/christmas/domain/event/discount/ChristmasDiscountEvent.java

+import christmas.domain.date.Date;
+import christmas.domain.reward.DiscountEventReward;
+import christmas.lib.event.DiscountEvent;
+
+import static christmas.constant.EventConstant.CHRISTMAS_DAY;
+import static christmas.constant.EventConstant.D_DAY_DISCOUNT_UNIT;
+import static christmas.constant.EventConstant.CHRISTMAS_EVENT_MESSAGE;
+import static christmas.constant.EventConstant.D_DAY_START_PRICE;


static import문들이 위에 위치하는 것이 자바 컨벤션에 부합하는 것으로 알고 있어요

자동 정렬이 , 밑으로 내려가서 변경한다고 신경썼는데
해당 부분은 놓쳤네요 ㅠㅠ

aammddkkzxc · 2023-11-19T05:31:49Z

src/main/java/christmas/domain/order/OrderInfo.java

+
+import christmas.domain.menu.Menu;
+
+public record OrderInfo(Menu menu, int amount) {


메일로 온 1주차 피드백에 '이름을 통해 의도를 드러낸다' 부분에 info를 불용어 라고 규정하고 쓰는 걸 지양한다고 되어있어서 다른 네이밍을 고려해보시는게 좋을 것 같아요

해당 부분은 ,
마땅한 단어가 생각이 안나서 Info 를 넣었는데
더 구체적인 단어를 생각하는것도 괜찮겠네요!

aammddkkzxc · 2023-11-19T05:37:04Z

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

+        printNewLine();
+    }
+
+    private static void printRewardMessage(RewardDto rewardDto) {


메서드가 15라인을 넘어가요 분리 해주는게 필요해 보여요

aammddkkzxc · 2023-11-19T05:39:05Z

src/main/java/christmas/view/message/OutputViewMessage.java

@@ -0,0 +1,33 @@
+package christmas.view.message;
+
+public enum OutputViewMessage {


저도 이렇게 출력을 열거로 관리한 부분이 있는데요, 한 리뷰어 분이 한번만 사용하는 문자열을 굳이 enum으로 관리 하지 않아도 될 것 같다는 말씀을 주셨었어요. @youngsu5582님의 생각은 어떤가요?

한번만 사용하긴 하나 ,
저는 출력 메시지를 한 곳에 다 모아서 관리하는 의미로는 enum 이 괜찮다고 생각해서 사용했습니다!
특히 , String.format 을 통해 , 출력을 동적으로 변화 시키므로 enum 으로 관리를 하고 싶었습니다!

src/main/java/christmas/controller/EventController.java

Cheonwooo · 2023-11-19T06:58:21Z

src/main/java/christmas/domain/event/discount/SpecialDiscountEvent.java

+import static christmas.constant.EventConstant.SPECIAL_DAY_PRICE;
+
+public class SpecialDiscountEvent extends DiscountEvent<Void> {
+    private final List<Integer> SPECIAL_DAY = List.of(3, 10, 17, 24, 25, 31);


특별 할인이 적용되는 날이 매주 일요일, 크리스마스 날이라는 점을 DayOfWeek를 활용해서 표현하는 것도 좋은 방법일거 같아요!

저는 이 부분에 대해서는 다른 의견입니다!
별 이 달려있는 날이 이벤트 날로 적용한다는 조건이었기 때문에,
이 부분은 List로 정적으로 할당해주는게 오히려 더 적절한 설계였다고 생각해요!

저도 해당 부분은 고민 했으나 , 해당 날짜에서 기존 규칙에서 벗어난 특별한 날이 생기면
전부 수정할 꺼 같아서 배열로 만들어서 관리 하는식으로 결정했습니다!

Cheonwooo · 2023-11-19T07:11:23Z

src/main/java/christmas/domain/order/Bill.java

+
+import static christmas.constant.OrderConstant.MAX_TOTAL_ORDER_COUNT;
+
+public record Bill(int totalPrice, EnumMap<Category, List<OrderInfo>> orderDetail) {


record를 활용해야겠다고 생각한 이유가 있을까요?

자동으로 제공해주는 getter 와 private final 생략등
편의성 때문에 사용했는데 클래스로 관리해서 필요한 부분만 하는게 더 깔끔하다는 생각도 드네요!!

Cheonwooo · 2023-11-19T08:01:55Z

src/main/java/christmas/domain/category/Dessert.java

+import christmas.domain.menu.Menu;
+import christmas.domain.menu.MenuItem;
+
+public enum Dessert implements MenuItem {


메뉴 카테고리마다 enum 클래스를 나눌 생각은 못했는데 이렇게 하면 유지보수가 더 쉽겠네요!
좋은 아이디어인거 같아요!

h-beeen · 2023-11-19T12:10:38Z

src/main/java/christmas/constant/DateConstant.java

+public interface DateConstant {
+    public static int START_DAY = 1;
+    public static int END_DAY = 31;
+}


이 부분도 월 수가 바뀐다면?
12월에서 1월로 바뀔 때, 2월로 바뀔 때 일수에 대한 제약조건을 수동으로 수정해주어야 할 것으로 예견됩니다.
전역에서 설정하는 연/월 설정을 바탕으로 StartDay와 EndDay를 동적으로 판단하면 좋을 것 같아요!

해당 부분은 생각했는데 ,
해당 과제를 위해 1월부터12월 배열을 다 만드는건 불필요한 코드라 생각해서 포기했는데,
LocalDate 와 전역을 사용하면 충분히 가능하겠네요,, 감사합니다!

h-beeen · 2023-11-19T12:11:09Z

src/main/java/christmas/constant/OrderConstant.java

+    public static final int MIN_AMOUNT = 1;
+    public static final int MAX_TOTAL_ORDER_COUNT = 20;


이 부분도 제약조건으로 분리해서 설정한 부분 너무 좋습니다 👍

h-beeen · 2023-11-19T12:12:25Z

src/main/java/christmas/domain/category/Drink.java

+    ZERO_COKE("제로콜라", 3000),
+    RED_WINE("레드와인", 60000),
+    CHAMPAGNE("샴페인", 25000);


이 부분도 25_000과 같이 일관적인 형태로 사용하면 좋을 것 같아요!

자동 정렬 에만 의존하고 , 직접 더 깔끔하게 해야 하는 부분에는 하지 못했던 거 같네요!

h-beeen · 2023-11-19T12:14:38Z

src/main/java/christmas/domain/order/Bill.java

+        int totalPrice = 0;
+        for (List<OrderInfo> orderInfos : orderBoard.values()) {
+            for (OrderInfo orderInfo : orderInfos) {
+                int price = orderInfo.menu().price();
+                totalPrice += orderInfo.amount() * price;
+            }
+        }


지역변수 재할당은 자바가 싫어하는 문법이라고 알고 있어요!
스트림 문법을 통해 필터링해서 더하는 로직을 설계한다면,
불필요한 지역변수 재할당을 피할 수 있을 것 같아요!

예전부터 , 함수형은 불필요한 겉멋이라 생각했는데,
우테코를 하며 깔끔하게 짜고 재할당 방지등의 장점을 보고 생각이 바뀐거 같아요!
이번에 리팩토링 하면 함수형을 적극 사용해볼꺼 같습니다! 감사합니다!!

h-beeen · 2023-11-19T12:15:08Z

src/main/java/christmas/domain/order/Bill.java

+
+        validate(orderMenuBoard);
+
+        int totalPrice = calculateTotalPrice(orderMenuBoard);


calculateTotalPrice의 리턴형은 Integer인데, 반환 변수는 int네요!
해당 메소드를 Integer로 반환하도록 설계하신 이유가 있을까요?

마지막에 , 급하게 짠다고 타입 일치를 못했네요... 🥲🥲

h-beeen

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

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

감사합니다 :)

ffolabear

고생하셨습니다! 많이 배워갑니다 👍

ffolabear · 2023-11-20T00:47:02Z

src/main/java/christmas/constant/BadgeConstant.java

+    public static final int STAR_THRESHOLD = 5000;
+    public static final int NON_THRESHOLD = 0;
+
+}


현재 구현하신 코드에서 BadgeConstant 는 Badge enum 에서 상수로 밖에 사용되지 않은 것으로 조금 불필요한 인터페이스지 않나 생각이듭니다. Badge enum 에서 이 부분은 함께 구현할 수 있다고 생각하는데 어떻게 생각하시나요?

해당 부분에서는 , 저도 생각을 되게 많이 했어요!
오히려 불필요한 파일 과 더 혼란을 주지 않을까 고민도 했지만 ,
Constant 파일 들에서 값만 조작하면 로직을 보지 않고 원하는 값을 손쉽게 수정할 수 있다고 생각해서
이렇게 코드를 짰습니다. @ffolabear 🐻‍❄️ 님처럼 enum 내에서 하는 방법도 괜찮은 거 같아요!!

ffolabear · 2023-11-20T00:48:04Z

src/main/java/christmas/constant/EventConstant.java

+
+    public static final Integer CHAMPAGNE_LIMIT_PRICE = 120000;
+    public static final Menu CHAMPAGNE_PRESENT = Drink.CHAMPAGNE.get();
+}


이벤트 관련 상수를 저는 enum 으로 구현했습니다. 인터페이스로 상수들을 정의하신 이유가 궁금합니다!

ffolabear · 2023-11-20T00:55:26Z

src/main/java/christmas/domain/date/Date.java

+        return Calendar.calculateDayOfWeek(day);
+    }
+
+}


날짜 정보를 담은 불변객체를 이용하셨군요 👍 다만 현재 Date 객체는 날짜 정보와 요일 정보만 가지고 있는것으로 보이는데 그렇다면 이미 자바에서 제공하는 LocalDate 를 사용하는게 어떨까 하는 생각이 듭니다! 1주차 피드백에도 나오는 내용이니까 고려해보시면 좋을것 같습니다 😁

다른 분들 코드 보면서 LocalDate 라는게 있는걸 알았네요 ㅠㅠ😭
자세히 찾아보지 못한 제 잘못인거 같습니다!
코드 리팩토링 때는 적극 활용 해볼 생각입니다! 감사합니다!!

ffolabear · 2023-11-20T01:11:08Z

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

+            if (event instanceof PresentEvent) {
+                presentEventList.add((PresentEvent) event);
+            }
+        }


이 부분은 약간 개선할 수도 있을것 같습니다! 이 클래스의 of 메소드는 EventSercice 에서 EventFactory 의 static 변수인 EVENT_LIST 를 파라미터로 넣어서 생성하는 부분에서만 쓰이는데 EVENT_LIST 는 항상 같은 값을 가지는 변수로 예상됩니다. 그렇다면 처음부터 할인 이벤트와 증정 이벤트를 따로 관리하는 방법은 어떨까요?

해당 코드는 이벤트가 할인 과 증정 이벤트 뿐 아니라 ,
특가 등 다른 종류의 이벤트가 생겨도 추가하기 쉽게 만들려고 했는데
생각해보니 , 그냥 DISCOUNT_EVENT_LIST , PRESENT_EVENT_LIST 처럼 나누는게 더 깔끔하겠네요!!
감사합니다! 👍

ffolabear · 2023-11-20T01:19:53Z

src/main/java/christmas/domain/event/discount/WeekdayDiscountEvent.java

+            count += orderInfo.amount();
+        }
+        return count;
+    }


이 부분은 stream 을 사용해 이렇게 구현할 수도 있습니다 😁

public class WeekdayDiscountEvent extends DiscountEvent<Bill> { //... private int countOrderMenu(List<OrderInfo> orderInfos) { return orderInfos.stream() .mapToInt.(OrderInfo::amount) .sum(); } //... }

해당 과제를 하며 , 함수형이 참 매력 적인게 느껴지네요!!

ffolabear · 2023-11-20T01:25:38Z

src/main/java/christmas/domain/order/Bill.java

+        }
+        return totalPrice;
+    }
+}


저는 검증로직은 분리하는것을 선호하는데 이 부분에 대해서 어떻게 생각하시는지 궁금합니다!

검증 로직을 Validator 에 분리하는 방식 과 도메인에 넣어서 스스로 검증 하는 방식에 대해 저도 되게 고민을 많이 했습니다!
제 생각으로는 도메인이 스스로 값에 대한 검증을 하는게 더 깔끔하다고 생각했습니다!
중복 적으로 사용할 수 있는 코드가 아니라면 , 해당 클래스 내에서 하는게 좋다고 생각한 이유도 있습니다!
어떻게 생각하시나요?? 더 의견을 들려주셔도 감사할 거 같아요!!

ffolabear · 2023-11-20T01:30:46Z

src/main/java/christmas/util/Calendar.java

+        int weekValue = (startDate.getValue() + day - 1) % 7;
+        return DayOfWeek.fromValue(weekValue);
+    }
+}


달력 정보를 새로 정의 하셨군요! 다만 이미 java,util 패키지 아래에 동일한 이름으로 구현되어 있는 클래스가 있어서 혼동할 여지가 있기 때문에 이름은 변경하는 것이 좋아보입니다.

저도 해당 부분에대해서 생각했으나 ,
마땅한 클래스 명이 생각안나서 Date 로 썼습니다,,
하지만 CustomDate 명을 사용하는 있더라도 기존 존재하는 클래스명을 사용하는건 안 좋았던 거 같습니다!!

ffolabear · 2023-11-20T01:46:13Z

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

+            }
+        }
+        printNewLine();
+    }


전체적으로 static 메소드가 많은것 같습니다. static 메소드는 클래스의 인스턴스 생성을 하지않아도 사용할 수 있다는 장점이 있지만 소멸 시점이 GC 에 의한 소멸 시점이 아닌 어플리케이션 종료시 이기 때문에 리소스를 많이 잡아 먹는다고 생각합니다.

그래서 저는 어플리케이션에 전반적으로 사용되어야할 메소드 및 변수만 static 으로 선언하려고 했는데 이 부분에 대해 어떻게 생각하시는지 궁금합니다 🤔

저도 static 메소드를 최소화할 필요가 있다고 생각합니다!

다른 분들 코드를 보면서 같은 이유로 InputView 와 OutputView 의 메소드들은 non-static 이 맞는거 같습니다!
View 를 Controller 단에서 분리하기 위해 사용하려 했으나 ,
오히려 불필요한 메모리 사용을 유발한 거 같네요!! 감사합니다!!!
코드에 대해 전체적으로 자세히 훑어주신거 같아서 너무 감사했습니다🙏🙏
최종때 까지 준비 잘하셨으면 좋겠습니다

hwangjeyeon

좋은 코드 잘 보고 갑니다!! static 메소드 선언만 줄인다면 더 좋은 코드가 될 것 같아요!! 4주동안 고생 많으셨습니다~

hwangjeyeon · 2023-11-20T02:25:11Z

src/main/java/christmas/factory/ControllerFactory.java

+
+import christmas.controller.*;
+
+public class ControllerFactory {


ControllerFactory로 유사한 기능을 하는 컨트롤러를 모아두고 static으로 선언하는 방식은 새롭네요!! 생각 못한 방법인데, 이것도 좋은 방법인 것 같아요! 혹시 이 부분을 생각하게 된 계기가 있을까요?
그리고 저는 이 부분을 인터페이스화 하거나 추상 클래스로 만들어보는 것만 생각했었는데, 혹시 이 부분에 대해서는 어떻게 생각하시나요?

해당 부분은 Factory Pattern 과 Sigleton Pattern 을 접목해서 사용했습니다!
Gamecontroller 에서 다른 Controller 를 사용하는 만큼 생성자에 다 넣으면
가독성이 떨어지고 매개변수가 너무 많아지는 점 때문에 해당 방법을 생각했습니다!!
도움이 되셨다면 다행입니다

hwangjeyeon · 2023-11-20T02:26:46Z

src/main/java/christmas/controller/BadgeController.java

+public class BadgeController {
+    private BadgeService badgeService = ServiceFactory.getBadgeService();
+
+    public Badge grantBadge(RewardDto rewardDto) {


grantBadge! 클래스명에서 센스가 넘치네요 ㅋㅋ 그런데 혹시 get 대신에 grant를 메소드명으로 선언한 이유가 있을까요?

Badge 를 단순히 , get 하는게 아니라
금액에 따라 부여 해준다고 생각해서 grant 를 썼습니다!! 😄😄

hwangjeyeon · 2023-11-20T02:27:26Z

src/main/java/christmas/controller/EventController.java

+import christmas.service.EventService;
+
+public class EventController {
+    private final EventService eventService = ServiceFactory.getEventService();


이 부분을 생성자가 아닌 필드에서 초기화한 이유가 있을까요?

저도 생성자 에 필드를 초기화 하려고 했으나
현재 Factory 단에서 주입을 해주지만
서비스가 전부 생성 된 후에 컨트롤러가 생성 되야 하지만 다 한번에 생성 되면서 에러가 발생했습니다!
해당 부분에서 생성자에 주입하려면 프레임워크의 위상 정렬을 비슷하게 나마 구현해야 할 거같습니다... �🥺

hwangjeyeon · 2023-11-20T02:29:12Z

src/main/java/christmas/domain/category/Appetizer.java

+import christmas.domain.menu.Menu;
+import christmas.domain.menu.MenuItem;
+
+public enum Appetizer implements MenuItem {


enum 인터페이스 사용은 생각 못했네요! 저는 Category를 enum으로 하나 더 만들었는데, 인터페이스 사용이 더 깔끔해 보여서 좋은 것 같습니다!

hwangjeyeon · 2023-11-20T02:33:00Z

src/main/java/christmas/domain/event/discount/WeekendDiscountEvent.java

+    }
+
+    private int countOrderMenu(List<OrderInfo> orderInfos) {
+        int count = 0;


이렇게 도메인에 포함시켜도 될만한 지역변수가 많은 것 같은데, 도메인 필드로 추가해보는건 어떨까요?

저도 , 도메인 필드에 값들 추가에 대해서 생각을 했지만
도메인 값 들로 가능한 값들은 최대한 포함을 안 시키려고 노력한 거 같습니다!
공간 과 시간 복잡도의 trade-off 의 느낌인거 같아요!!

hwangjeyeon · 2023-11-20T02:38:07Z

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

+            }
+        }
+        printNewLine();
+    }


저도 static 메소드를 최소화할 필요가 있다고 생각합니다!

youngsu5582 added 30 commits November 11, 2023 14:14

docs : 기능 요구사항 초안 정리

8e1abb6

feat : 입출력 담당 하는 InputView,OutputView 추가

5d4a71e

입력 담당 하는 InputView 출력 담당 하는 OutputView

feat : Date 를 생성 하는 기능 추가

9674ca5

Date Domain 생성 부가 적인 DateConstant , DateException , DateExceptionMessage 같이 추가 이에 따른 , 테스트 케이스 구현 - 정상 적인 날짜를 입력 하는 케이스 - 범위 밖의 날짜를 입력 하는 케이스

feat : Category , Menu , MenuItem 추가

ec5663b

Category , Menu Domain 생성 Category 에 해당 하는 Drink , Dessert , MainDish , Appetizer 추가 이에 따른 , 테스트 케이스 구현 - 카테고리 에 해당 하는 클래스 의 요소 받는 케이스

feat : MenuCatalog 추가

10a9bad

MenuCatalog 내 , 이름에 따라 , Menu 를 반환 해주는 searchFromMenuName 기능 추가 이에 따른 , 테스트 케이스 구현 - 음식명을 통해 메뉴를 받는 케이스 - 없는 음식명을 입력 하는 케이스

feat : Event,DiscountEvent,PresentEvent 추가

24930ab

이벤트 공통 관리를 위한 추상 Class 인 Event 추가 증정 이벤트 를 위한 PresentEvent 추가 할인 이벤트를 위한 DiscountEvent 추가

feat : Event,DiscountEvent,PresentEvent 제네릭 타입 추가

8e2d438

Object 의 타입을 하위 클래스 에서 지정 하기 위해 제네릭 타입으로 변경

feat : WeekendDiscountEvent 기능 추가

0227308

EventConstant 추가 MenuCatalog 내 , 이름 비교 API 수정 이에 따른 , 테스트 케이스 구현 - 평일인 케이스 - 주말인 케이스 - 주말 이지만 메인 디쉬가 없는 케이스

feat : DiscountEventReward , PresentEventReward 추가

7d90095

DiscountEventReward,PresentEventReward Domain 추가 Event 내 리턴 타입도 유동적 사용 위해 제네릭 타입 으로 변경 DiscountEvent,PresentEvent 리턴 타입도 변경

feat : 기존 Event 리턴 타입 변경

8b5dfa8

DiscountEventReward,PresentEventReward 로 변경 이에 따른 , 테스트 케이스 리턴 타입도 수정

feat : EventReward 추가

d9bd891

공통 적으로 사용 위한 상위 클래스 EventReward 추가 DiscountEventReward, PresentEventReward 들이 상속 상속을 위해서 record -> class 로 변경

feat : 혜택을 모오는 Reward 추가

6883c9d

Reward , RewardDto 추가 이에 따른 , 테스트 케이스 구현 - Reward 를 만드는 케이스 - 보상이 분류 되어 들어간 지 확인 하는 케이스 - 총 할인 금액 계산이 일치 한지 확인 하는 케이스

feat : 방문할 날짜를 입력 하는 1번 기능 요구 사항 완료

f5cf59d

모든 기능 관리 하는 Gamecontroller 생성 Date 생성을 담당 하는 DateController , DateService 생성 InputView 와 OutputView 에 함수 추가 OutputViewMessage , InputViewMessage 에 상수 추가

feat : 주문할 메뉴와 메뉴 개수 입력 하는 2번 기능 요구 사항 완료

a23b4a1

Order,Bill 생성을 담당 하는 OrderController , OrderService 생성 InputView 와 OutputView 에 함수 추가 OutputViewMessage , InputViewMessage 에 상수 추가

fix : 누락된 파일 추가

6a1ad99

OrderService 누락 되어 파일 추가

feat : 숫자 변환 해주는 Formatter 추가

5259f91

숫자를 세 자리 마다 , 로 포맷 해주는 formatNumber 추가 숫자를 포맷 후 , - 를 붙히는 formatNegativeNumber 추가

feat : 적용 가능한 이벤트 혜택 적용 하는 3번 기능 요구 사항 완료

a28a077

Reward 생성을 담당 하는 EventController , EventService 생성 OutputView 에 함수 추가 , OutputViewMessage 에 상수 추가 Reward 에 0원이면 , 추가 되지 못하게 로직 추가

fix : 잘못 기입한 상수명 수정

55540fa

TAPAS 가격 5000 -> 5500 수정 EVENT_NAME 수정 DUPLICATE_MEMU -> NOT_EXIST_MENU 변경

feat : 총 혜택 금액 따라 이벤트 뱃지 부여 하는 4번 기능 요구 사항 완료

ed16c00

Badge 생성 담당 하는 BadgeController , BadgeService 생성 OutputView 에 함수 추가 , OutputViewMessage 에 상수 추가

feat : 숫자에 따라 , 포맷 결정 하는 formatBasedOnNumber 추가

163e909

숫자가 0 이상 일시 , - 부착 숫자가 0일 시 , - 부착 하지 않음

youngsu5582 added 19 commits November 14, 2023 12:25

refactor : 에러 메시지 내용 변경 , 구분자 상수 파일로 이동

f33c8bc

ORDER_SEPARATOR , REQUEST_SEPARATOR 상수 파일 분리 메뉴 입력중 발생 예외 사항 요구사항 맞게 메시지 변경 변경에 따라 , 테스트 파일도 수정

refactor : 배열 생성중 , 반복문 구문 분리

4a00ee2

confirmRequestOrders , confirmOrders 중 생성 부분 함수 생성 하여 분리

feat : RewardDto getTotalDiscountReward 추가

d67a2c7

Discount 를 통해 , 할인된 값만 모아서 반환하는 getter 추가

feat : Date Service try - catch 문 추가

54276b0

Parser 중 에러 발생 시 , INVALID_DAY 발생 하게 변경

refactor : 코드 전반 적인 리팩토링

e0d8cf1

함수명 변경 함수 로직 중 구분 위한 공백 추가 에러 메시지 공백 추가

refactor : Coverage 위한 불 필요한 코드 제거

eba4437

Calendar 의 setStartDate 함수 제거 - 테스트 케이스 에서도 제거 MenuItem 의 getPrice 함수 제거 - 상속 하는 enum 에서도 제거

refactor : Date Domain 관련 리팩토링

830c2b7

스택 오버플로우 & 최적화 위한 재귀 함수 제거 Exception Chaining 위해 발생 Exception 포함해 새로 생성 - CustomException 에도 Exception 포함해 생성 하는 메소드 기능 추가

refactor : Order Domain 관련 리팩토링

dbbd4fe

스택 오버플로우 & 최적화 위한 재귀 함수 제거 Exception Chaining 위해 발생 Exception 포함해 새로 생성

refactor : Event 파일들 Discount , Present 로 구분

8e0860e

더 보기 쉽게 하기 위해 폴더별 이벤트 분리

refactor : Event 이름 상수 파일로 이동

a955741

이벤트 명도 상수 파일에서 관리

refactor : Domain 폴더 변경

39ac7eb

기존 event 폴더 reward 로 이름 변경 밖에 있는 event 폴더 domain 내 event 로 이동

refactor : import 구문 순서 변경

fbb42fc

1. 표준 Java 라이브러리 import 2. 서드 파티 라이브러리 import 3. 다른 패키지 import 4. 정적 import

refactor : 제출 전 리팩토링

48f694b

present 부분 상수 처리 누락된 import 문 순서 변경 TODO 제거

test : 테스트 케이스 추가

a5c7c16

CaseTest 내 , 다른 케이스 추가 Application 내, 예외 케이스 추가 - 음료 단독 주문 케이스 - 메뉴 초과 주문 케이스

feat : Event,Service,Controller Factory 추가

0367681

Controller 주입 해주는 ControllerFactory Service 주입 해주는 ServiceFactory EventList 주입 해주는 EventFactory 이에 따라 , 컨트롤러 서비스 단 수정

test : 파일명 변경 및 테스트 코드 추가

562c727

기존 : EventList 변경 : InProgressEventList EventList 는 Event 의 배열명 으로도 사용 가능 - 이벤트 목록 분리 하는 케이스 추가

refactor : 테스트 함수명 변경 , 누락 함수 추가

0a7415e

임시 이름인 Temp 로 선언한 케이스 명 변경 - ApplicationTest 에서 메뉴 0 시키는 케이스 추가

refactor : Test 내 Domain 파일 폴더별 분리

cfce5be

가독성 위해 , Test/Domain 내 파일들 폴더로 이동

docs : 기능 전부 구현 후 README 상세 내용 추가

ba73ffc

고민한 부분에 대한 내용 정리 폴더 구조 이미지 첨부 테스트 커비리지 결과 첨부 입출력 , 에러 부분 상세 설명 추가

aammddkkzxc reviewed Nov 19, 2023

View reviewed changes

src/main/java/christmas/controller/EventController.java Show resolved Hide resolved

Cheonwooo reviewed Nov 19, 2023

View reviewed changes

h-beeen reviewed Nov 19, 2023

View reviewed changes

ffolabear reviewed Nov 20, 2023

View reviewed changes

hwangjeyeon reviewed Nov 20, 2023

View reviewed changes


		import christmas.domain.menu.Menu;

		public record OrderInfo(Menu menu, int amount) {

		@@ -0,0 +1,33 @@
		package christmas.view.message;

		public enum OutputViewMessage {


		import static christmas.constant.OrderConstant.MAX_TOTAL_ORDER_COUNT;

		public record Bill(int totalPrice, EnumMap<Category, List<OrderInfo>> orderDetail) {

		public static final int MIN_AMOUNT = 1;
		public static final int MAX_TOTAL_ORDER_COUNT = 20;


		validate(orderMenuBoard);

		int totalPrice = calculateTotalPrice(orderMenuBoard);


		import christmas.controller.*;

		public class ControllerFactory {

[크리스마스] 코드 리뷰 부탁드립니다! #1

Are you sure you want to change the base?

[크리스마스] 코드 리뷰 부탁드립니다! #1

Conversation

youngsu5582 commented Nov 16, 2023

🎄 Java-Christmas

고민한 부분

폴더 구조

Test Coverage

Test Case

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-beeen Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-beeen left a comment

Choose a reason for hiding this comment

ffolabear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwangjeyeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-beeen Nov 19, 2023 •

edited

Loading