[크리스마스 프로모션] 변해빈 과제 제출합니다. #1
Conversation
| @DisplayName("[requestOrders] 주문할 메뉴, 갯수를 요청하고, 요청이 제약조건에 맞지 않으면 예외를 던진다.") | ||
| class requestOrders extends NsTest { |
There was a problem hiding this comment.
🤔고민🤔
저도 NsTest 를 쓸 때 마다 고민 되는 사항이 몇 있었어요.
NsTest 가 메인 출력을 테스트하는 기본 코드에 포함이 되긴 하지만요..!
첫 번째로 모든 출력 테스트 코드가 외부 라이브러리에 의존한다는 고민도 있었어요.
두 번째로 NsTest 이름만으로 무엇을 상속한다는 건지 꽤 알아차리기 힘들었어요.
세 번째로 메소드 재정의가 불가능해 (runMain() 만 재정의 가능) 제가 원하는 출력 형식을 제대로 테스트하지 못한다는 느낌도 받았어요. 예를 들어 출력에 좌우 공백이 들어가면, 그 공백이 trim() 메소드로 짤리더군요. 그럼 기존 상수 String 과 다른 출력이 이뤄졌어요. 하지만 실제 출력은 공백이 들어가는데, 제대로 된 테스트가 맞나..? 생각이 들더라고요.
그래서 저는 ConsoleOutputCapture, PrintOutputTest 등등으로 조금 더 이름을 구체적으로 붙여서,
프로젝트 자체적으로 새로 만들었는데, 혹시 저와 같은 고민을 해보셨을까요~? 의견이 궁금합니다. :)
There was a problem hiding this comment.
추후 설계에서는 NsTest를 배제하는 방향으로 하려고 합니다!
NsTest 자체가, 표준 입출력에 종속되어있는 설계기도 하고,
컨트롤러 레벨을 테스트하기에 부적합한것은 아니나, 여러가지 커스텀해서 구현하기에는
다소 제한적이라는 생각이 저도 많이 들었습니다!
AssertJ나, JUnit을 적극 활용해 테스트코드를 짜는 것이 좋을 것 같아요!
@wooteco-daram 님께서 말씀 주신 대로,
프로젝트 자체적으로 테스트 할 수 있는 모듈을 만들고
이를 상속받아 사용하는 것도 큰 도움이 될 것 같군요 👍
|
|
||
| //== Constructor ==// | ||
| private VisitDay(final int visitDay) { | ||
| this.visitDay = ExceptionHandler.tryOnParseIntException(() -> convertLocalDate(visitDay)); |
There was a problem hiding this comment.
생성자는 무조건 public으로 해서 다른 곳에서 쓰는건줄 알았는데 혜빈님은 private으로 하셨군요...!
There was a problem hiding this comment.
생성자를 private으로 닫고, 정적 팩토리 메소드로 호출하도록 강제했습니다.
의미를 부여한 정적 팩토리 메소드 호출을 강제해, 무분별한 객체 생성에 대한 휴먼 에러를 막고자 하는 의도입니다 :)
There was a problem hiding this comment.
ExceptionHandler를 VisitDay에서 호출한 이유를 여쭙고 싶습니다.
개인적으로는 ExceptionHandler를 통한 예외 처리는 Controller 단에서 끝내고, VisitDay에선 비즈니스 로직 (1 ~ 31일)에 대한 검증만 해주면 명료해질 것 같아 질문 드립니다!
| } | ||
|
|
||
| public Integer multiplyDate(final int valueToMultiply) { | ||
| return visitDay.getDayOfMonth() * valueToMultiply; |
There was a problem hiding this comment.
getter는 이런 상황에서는 써도 되는걸까요?? 정확히 어느 범위까지 getter를 안 쓰고 메세지를 보내줘야하는지
헷갈리네요ㅜ
There was a problem hiding this comment.
multiplyDate의 경우, 객체 내부에서 필드 변수에 대한 값을 가져와, 값을 곱해 리턴하는 함수입니다.
반대로, 해당 함수를 호출하는 곳에서 getVisitDay() 로 객체 전체를 가져와 값을 비교하는 방법이 있을수 있겠네요.
VisitDay로 포장된 객체를 외부해 노출하지 않고, 해당 도메인 내부에서 위와같이 사용함으로써 캡슐화를 조금 더 지킬 수 있어요!
| public boolean containPeriod( | ||
| LocalDate startDate, | ||
| LocalDate endDate | ||
| ) { | ||
| return !(visitDay.isBefore(startDate) || visitDay.isAfter(endDate)); | ||
| } |
There was a problem hiding this comment.
게터를 쓰는 대신에 이렇게 boolean 함수를 따로 만드셨군요...!
| MUSHROOM_SOUP(APPETIZER, "양송이수프", 6_000), | ||
| TAPAS(APPETIZER, "타파스", 5_500), | ||
| CAESAR_SALAD(APPETIZER, "시저샐러드", 8_000), | ||
|
|
||
| //== MAIN_DISH ==// | ||
| T_BONE_STEAK(MAIN_DISH, "티본스테이크", 55_000), | ||
| BBQ_RIB(MAIN_DISH, "바비큐립", 54_000), | ||
| SEAFOOD_PASTA(MAIN_DISH, "해산물파스타", 35_000), | ||
| CHRISTMAS_PASTA(MAIN_DISH, "크리스마스파스타", 25_000), | ||
|
|
||
| //== DESSERT ==// | ||
| CHOCOLATE_CAKE(DESSERT, "초코케이크", 15_000), | ||
| ICE_CREAM(DESSERT, "아이스크림", 5_000), | ||
|
|
||
| //== BEVERAGE ==// | ||
| ZERO_COLA(BEVERAGE, "제로콜라", 3_000), | ||
| RED_WINE(BEVERAGE, "레드와인", 60_000), | ||
| CHAMPAGNE(BEVERAGE, "샴페인", 25_000); |
There was a problem hiding this comment.
APPETIZER 안에 mushroom, tapas, salad 가 있는게 아니라
각 메뉴 안에 에피타이저를 넣어주셨군요...! 이렇게 구현을 할 수도 있겠네요 배워갑니당!
| public enum MenuCategory { | ||
| APPETIZER, | ||
| MAIN_DISH, | ||
| DESSERT, | ||
| BEVERAGE; | ||
|
|
||
| MenuCategory() { | ||
| } |
There was a problem hiding this comment.
메뉴 카테고리를 따로 만드셨군요..! 이렇게 분리하면 어떤 장점이 있을까요?
There was a problem hiding this comment.
평일엔 DESSERT, 주말엔 MAIN_DISH, BEVERAGE만 주문했을 때 예외처리.
해당 부분을 문자열로 처리하는 것 보다, type-safety 하게 처리할 수 있습니다 :)
| private boolean hasOnlyBeverages(EnumMap<Menu, Integer> menus) { | ||
| return menus.keySet() | ||
| .stream() | ||
| .allMatch(this::isBeverage); | ||
| } |
There was a problem hiding this comment.
메뉴에 대한 오류 상황들을 다른 오류 클래스를 생성하지 않고 바로 Orders 안에서 처리해주셨네요.
이게 훨씬 더 직관적인거 같네요 저도 이렇게 수정해보겠습니다 !
|
|
||
| //== Static Factory Method ==// | ||
| public static Orders create(EnumMap<Menu, Integer> menus) { | ||
| return new Orders(menus); |
There was a problem hiding this comment.
Menu 객체를 따로 만들어서 Menus에 넣었었는데 EnumMap이라는게 존재하는군요
배워갑니다!
| VisitDay visitDay, | ||
| Orders orders, | ||
| BadgePromotion badge | ||
| ) { |
There was a problem hiding this comment.
생성자 안에 이렇게 초기화를 하는게 훨씬 더 효율적인 코드같네요. 저도 수정해보겠습니다 !
| PRODUCT_QUANTITY_RESULT("%s %d개"), | ||
| PRICE_RESULT("%,d원"), | ||
| MINUS_PRICE_RESULT("-%,d원"), | ||
| BENEFIT_PRICE_RESULT("%s: -%,d원"), | ||
| GIFT_RESULT("증정 이벤트: -%,d원"), | ||
| PROMOTION_PREVIEW("%d월 %d일에 우테코 식당에서 받을 이벤트 혜택 미리 보기!"); |
There was a problem hiding this comment.
이 부분을 public static final이 아닌 enum으로 하신 이유가 있을까요??
3주차 피드백에 연관성이 있는 상수는 static final 대신 enum을 활용한다 라고 나와있는데,
위의 부분은 단순한 포맷팅이라 enum을 쓴 이유가 궁금합니다!!
저도 원래는 enum으로 했다가 3주차 피드백을 보고 이 부분은 static final을 썼거든요,,,
과연 어느방식이 좋은 방법일까요?
There was a problem hiding this comment.
| VisitDayController.responseVisitDay(visitDay); | ||
| OrderController.responseOrdersResult(orders); | ||
| OrderController.responseTotalOriginPriceResult(orders); | ||
| PromotionController.responseAppliedBenefitResult(visitDay, orders); |
There was a problem hiding this comment.
하나의 controller에서 전부 처리를 하는게 아니라, controller를 나누셨군요!! 저도 나눠보겠습니다
| BadgePromotion defaultBadge = DEFAULT; | ||
| AppliedDiscountPromotions discountPromotions = AppliedDiscountPromotions.create(visitDay, orders, defaultBadge); | ||
| AppliedGiftPromotions giftPromotions = AppliedGiftPromotions.create(visitDay, orders, defaultBadge); | ||
| BadgePromotion badge = BadgeContext.applyPromotions(visitDay, discountPromotions, giftPromotions); | ||
|
|
||
| final int totalDiscountAmount = discountPromotions.getExpectedPayment(orders); | ||
| final int totalBenefit = giftPromotions.getTotalBenefit(discountPromotions); | ||
|
|
||
| DiscountResponse discountResponse = DiscountResponse.from(discountPromotions); | ||
| GiftResponse giftResponse = GiftResponse.from(giftPromotions); | ||
| BadgeResponse badgeResponse = BadgeResponse.from(badge); | ||
|
|
||
| responseGiftResult(giftResponse); | ||
| responseBenefitResult(discountResponse, giftResponse); | ||
| responseTotalBenefitResult(totalBenefit); | ||
| responseExpectPaymentResult(totalDiscountAmount); | ||
| responseBadgeResult(badgeResponse); | ||
| } |
There was a problem hiding this comment.
[ERROR]
제가 설계했던 부분 중, 가장 고민하고, 고민하다 조금은 망가진 로직입니다 😢
실제 코드라인 수는 개행을 제외하고 15줄 이하이긴 하지만,
큰 리팩토링이 필요한 신호라는 사실은 변함이 없죠.
객체 자체의 설계가 강타입으로 구현된 경향이 있어,
이 부분은 조금 더 머리를 싸매고 뜯어봐야 할 것 같아요 ㅠ
There was a problem hiding this comment.
Promotion의 조건이 더 까다로워진다면 enum으로 정의하는 한줄짜리 조건은 괜찮은 구현방법일까요?
예시) 기존에 산타 뱃지를 받은 고객 중, 특별 할인 기간에 방문하여 메인 디쉬를 두 개 이상 주문한 고객에게 총 금액의 5% 할인 적용
너무 억지스러운 예시일지 모르겠지만, 조건이 조금만 복잡해져도 한 줄에 너무 코드가 길어질 것 같아요. 고민 해보면 좋을 것 같습니다.
There was a problem hiding this comment.
Enum 내부 함수형 인터페이스로 조건문을 분기하면 장단점이 명확하게 갈리는 것 같아요.
- Enum 내부의 정적인 인자들로 동작하는 신규 프로모션 추가의 구현은 정말 간단합니다.
- 특수한 조건으로 동작하는 신규 프로모션의 추가는, 새로운 구체 클래스의 구현이 불가피합니다.
전략패턴과 Enum-함수형 인터페이스의 선택에서, 저는 후자를 선택했고,
후자를 선택하면서 겪는 자연스러운 문제이지 않을까 싶습니다 😢
조금은 강타입으로 구현체(Enum)에 의존하면서 설계하다 보니
말씀주신 부분에 우려가 생긴다는 점 동의합니다!!
조금은 유연하지 못한 설계였다고 반성하며,
더 좋은 방법(최소비용 + 유연함)을 고민해보겠습니다 :)
| private static final int POSITIVE_NUMBER_MINIMUM_RANGE = 1; | ||
| private static final String DELIMITER = ","; | ||
| private static final String HYPHEN = "-"; | ||
| private static final Pattern REGEX_PATTERN = Pattern.compile("^[가-힣]+-\\d{1,20}$"); |
There was a problem hiding this comment.
| private static final Pattern REGEX_PATTERN = Pattern.compile("^[가-힣]+-\\d{1,20}$"); | |
| private static final Pattern REGEX_PATTERN = Pattern.compile("^[가-힣]+-\\d+$"); |
혹시 REGEX의 \d{1, 20}가 1~20의 정수를 받는다는 의도셨다면, 바로잡아드리고싶네요.
다음의 부분은 1에서 20개 사이의 digit 이라는 뜻입니다! 다른 메서드에서 Integer.parseInt 이후에 검증하는게 좋을 것 같아요.
There was a problem hiding this comment.
이번 과제에서 정규표현식을 처음으로 사용해보았는데요..
사용 과정에서 조금 문제가 있었네요!
말씀주신 내용을 바탕으로, 다시 공부해보니
1~20자리의 숫자를 보장하는 정규표현식이라는 것을 확인하게 되었습니다!!
물론, 총 주문횟수에서 이를 검증하면서 오류가 발생하지는 않지만
개별 주문이 20개가 넘으면 에러를 던지려고 기존에 설계했던 로직 자체는
조금 의도하는 바와 다르게 동작했던 것 같습니다!!
날카롭고 예리한 리뷰 감사드립니다 :)
| public int countOrdersByMenuType(MenuCategory menuCategory) { | ||
| return menus.keySet() | ||
| .stream() | ||
| .filter(key -> key.isSameCategory(menuCategory)) |
There was a problem hiding this comment.
- 해당 부분은 , keySet 을 통해 , 순회하기 때문에
key 라는 변수명도 좋을거 같지만 , enumMap 을 사용하기 때문에
바로 알아차리기 쉽게 menu -> menu.isSameCategory 도 괜찮을 거 같아요!
There was a problem hiding this comment.
말씀주신대로 Enum의 Key에 집중하는 것이 아니라, menu와 같이 직관적인 이름이,
직관적인 코드리뷰에 도음이 될 것 같습니다 :)
| package christmas.domain.consumer.constants; | ||
|
|
||
| public enum PlannerConstraint { | ||
| PROMOTION_YEAR(2_023), |
There was a problem hiding this comment.
12월 까지는 생각했으나
다가오는 1월 이벤트 이니까 , 년도가 바뀌는데 이거 까지는 생각 못했네요!
| ); | ||
|
|
||
| private final LocalDate startDate; | ||
| private final LocalDate endDate; |
There was a problem hiding this comment.
맨날 , Date 만 쓰고 LocalDate 에 대해서는
자세히 안봤었는데 사용해서 매우 깔끔하게 코드를 짤 수 있네요!
There was a problem hiding this comment.
LocalDate 문법! 생각보다 편리한 문법이 많습니다!
꼭 사용해보세요 :)
| ) { | ||
| DiscountContext promotionContext = DiscountContext.create(visitDay, orders, badge); |
There was a problem hiding this comment.
Promotion 들을
공통적으로 어떻게 사용할 수 있을까 계속 생각했는데 ,
정말 깔끔하게 되네요... 해당 부분은 리팩토링 할때도 사용할 거 같습니다.👍👍
| return visitDay.getDayOfWeek().getValue() > THURSDAY_VALUE | ||
| && visitDay.getDayOfWeek().getValue() < SUNDAY_VALUE; |
There was a problem hiding this comment.
저도 해당 부분은 어색함이 있는거 같습니다!
억지적인 부분이긴 하나 주말이
금요일 & 토요일 -> 토요일 & 일요일로 바뀔 여지가 있다고 생각하면
배열에 요소를 넣고 contains 로 비교하는 것도 괜찮을 거 같아요!
| (visitDay, orders) -> true, | ||
| always -> true | ||
| ), | ||
| WEEKDAY_PROMOTION_CONDITION( |
There was a problem hiding this comment.
이벤트 처리를 어떻게 하나 계속 생각했는데
처음에 날짜 처리
두번째 조건 처리
구분지어서 하니 되게 깔끔하게 되네요..!
| BadgePromotion badge | ||
| ) { | ||
| return hasApplicableTotalOriginPrice(orders) | ||
| && applicableFunction.test(visitDay, orders) |
There was a problem hiding this comment.
해당 부분에서 test 메소드가 있길래 움찔 했는데 ,
해당 타입을 사용하기 위해선 어쩔수 없는 부분 이군요!
BiPredicate 에 대해서도 , 공부 해봐야 겠네요! 감사합니다!!
There was a problem hiding this comment.
apply() 메소드와 동일한 기능을 하는 메소드라고 생각하시면 이해하기 편할 것 같습니다 :)
| import java.time.LocalDate; | ||
| import java.util.List; | ||
|
|
||
| public enum SpecialPromotionPeriod { |
There was a problem hiding this comment.
Discount 와 Present 로만 나눌 생각을 했는데 ,
종류별로 나눌 수 있는 방법도 괜찮네요!
|
Reference Memo |
| VisitDay visitDay = VisitDayController.requestVisitDay(); | ||
| Orders orders = OrderController.requestOrders(); | ||
|
|
||
| VisitDayController.responseVisitDay(visitDay); | ||
| OrderController.responseOrdersResult(orders); | ||
| OrderController.responseTotalOriginPriceResult(orders); | ||
| PromotionController.responseAppliedBenefitResult(visitDay, orders); |
There was a problem hiding this comment.
아래의 예시와 같이 동일한 depth에 위치한 컨트롤러 간 호출은 안티패턴이 맞다고 생각합니다.
여기서의 동일한 depth는 비즈니스에 맞는 적절한 Service / Domain Layer를 호출하는 컨트롤러간의 호출로 보시면 될 것 같습니다.
View <- OrderController -> XXService
|
V
View <- PromotionController -> XXService
하지만 Controller를 View에게 응답결과를 적절히 전달, Service / Domain을 적절히 호출 하는 역할로 분리하여 아래와 같은 그림이 된다면 이야기는 조금 달라진다고 생각합니다.
View <- FrontController -> XXXController -> XXService
-> XXXController -> XX
여기서 FrontController는 마치 스프링의 서블릿 디스패처와 같은 결이라 보면 되지 않을까 싶어요. (매우 매우 다르지만)
그리고 이러한 구조의 장점은 XXXController는 적절한 Service를 호출하여 응답 결과를 FrontController에게 내려주는 역할을 수행하기 때문에 Service Layer 전반에 걸친 테스트(Layer Test? 있는 말인진 모르겠네요)를 하기 굉장히 수월하다 느껴졌습니다.
이 부분에 대해서 @h-beeen 님의 생각을 여쭙고 싶습니다 :)
| public static Orders requestOrders() { | ||
| OrderOutputWriter.printMessageResponse(REQUEST_MENU_ORDERS); | ||
| return ExceptionHandler.retryOnBusinessException(OrderController::createMenuOrdersFromInput); | ||
| } |
There was a problem hiding this comment.
저는 현재 미션에선 크게 상관은 없다고 생각하지만, 있으면 가독성은 더 좋아질 것이라는 생각은 듭니다.
해당 미션을 진행하며, 인스턴스 필드의 존재 여부는 이 객체가 어떤 객체들과 협력 관계에 놓여 있구나를 굉장히 직관적으로 보여줄 수 있는 장점이 있다고 느껴졌습니다.
해빈님의 OrderController 코드를 예시로 들면,
public class OrderController {
private final OrderOutputWriter;
// something
}인스턴스 필드만으로, OrderController가 도메인 결과값을 OrderOuputWriter에게 렌더링하겠구나를 쉽게 파악할 수 있다고 생각합니다. static으로 클래스 레벨에서 참조하나, 인스턴스 필드로 참조하나 모두 강한 결합상태이기에, 협력 관계에 놓여 있는 객체들이라면 인스턴스 필드로 두는게 낫지 않을까? 라는 생각입니다. 이 부분에 대해서 해빈님 생각을 여쭙고 싶습니다 😀
| ErrorCode(String message) { | ||
| this.message = message; | ||
| } | ||
|
|
There was a problem hiding this comment.
BooleanSupplier 라는 것도 있군요! 배워갑니다 👍
해빈님의 domain 쪽 validate 로직을 보면, ErrorCode의 validate 메서드를 람다로 호출하는 것으로 보입니다. 확실히 코드 재사용성 측면에서 장점이 있는 것 같습니다 :)
하지만 domain쪽 검증 로직을 exception 패키지에서 throw하는게 좋은 구조인가? 에 대한 생각도 드는 것 같아요! 약간 어색한 느낌도 들긴 합니다. 하지만 throw를 한 곳에서 관리하는 구조도 설명은 못하겠지만 괜찮은 구조인 것 같습니다.
좋다/나쁘다 단정 짓는� 느낌보단 순수한 궁금증이라 명확히 딱 말씀을 못드리겠네요 ㅎㅎ.. 그냥 코드 재사용성 이외에 어떤 장점을 염두해 두시고 구현하신건지 여쭙고 싶습니다 :)
There was a problem hiding this comment.
이 부분도 사실, 4주차 과제에서 꼭 리뷰를 받아보고 싶어 시험 삼아 설계해본 로직입니다.
ErrorCode의 로직의 책임과 관심사를 어디까지 보느냐에 따라 다를 것 같은데,
사용해본 결과, 로직을 직관적으로 줄여주고, throw 문을 담당하는 예외처리 메소드를 극한으로 줄일 수 있는 장점이 있지만,
ErrorCode의 책임과 권한이 전역에 굉장히 크게 퍼져있다는 생각도 들었어요!
물론 말씀주신 것 처럼 코드 재사용성만을 염두해두고 작성했던 구조인 것은 사실입니다!
저도 좋은 구조라고 단정을 짓지는 못하곘지만, 인상적인 구조라고 생각합니다.
다만! 최종 코딩테스트 환경이라면, 사용을 지양할 것 같아요!
| this.name = name; | ||
| this.price = price; | ||
| } | ||
|
|
There was a problem hiding this comment.
HashMap 캐싱과 관련한 코멘트가 있어서 코드만 추가로 첨부드리면
public enum Menu {
// 열거형 상수들 ..
private final Map<String, Menu> cachedMenu = new HashMap<>();
static {
for (Menu menu : values()) {
cachedMenu.put(menu.getName(), menu);
}
}Key: String에 대한 NPE 우려가 있으니,
public static Menu findMenuByName(String name) {
return Optional.ofNullable(cachedMenu.get(name)).stream()
.filter(menu -> menu.isSameName(name))
.findFirst()
.orElseThrow(() -> BusinessException.from(INVALID_ORDER));
}해빈님 코드에선 이런식으로 바꿔볼 수 있을 것 같습니다!
이건 오늘 들었던 고민인데 위의 코드와 관련된 고민이라 해빈님 생각도 여쭙고 싶네요!
위의 코드 처럼 캐싱을 도입하면 Optional은 따라올 수밖에 없는 구조라고 생각합니다.
이 때, findMenuByName 메서드에서 String name에 대한 메뉴가 없을 경우
- Optional.empty() 반환
- 예외 throw
어떤 구조가 더 좋다고 생각되시나요? 명확한 기준이 안서, 의견을 여쭙고 싶습니다.
There was a problem hiding this comment.
저도 @june-777 님의 질문에 대한 다른분들의 의견이 궁금합니다!
저는 find 라는 역할에 맞게 해당하지 않는 메뉴가 없다면 '없다' 라는 의미의 Optional.empty()를 반환하는 방향으로 설계했는데 다른 분들의 의견이 궁금합니다!
There was a problem hiding this comment.
사실상 지금 Menu의 기능을 확장해서 판단하자면 MenuRepository 의 기능을 간접적으로 수행하고 있다고 생각합니다.
JPA에서 예외처리를 작성하는 것 처럼, 예외를 던지는 것이 훨씬 더 안정적인 설계라고 생각합니다.
return sampleRepository.findById(id)
.orElseThrow(IllegalArgumentException::new);그렇지 않다면 하위 비즈니스 로직에서 Null과 empty에 대한 제약을 항상 강제적으로 갖춰야 할 것 같아요.
휴먼 에러에 따른 NPE 이슈도 간과할 수 없구요!
There was a problem hiding this comment.
@june-777 님께서 남겨주신 의견에 대한 생각을 짧게나마 남겨보자면..!!
이번 미션에서 주어진 요구사항에서는 메뉴가 존재하지 않는다면 예외를 발생하는 사항이 있었기 때문에,
h-beeen님께서 말씀하신 것처럼 .orElseThrow()와 같이 값이 존재하지 않는다면 예외를 발생시키는 것이 맞는 것 같습니다.
.orElseThrow() 자체가 Optional을 검사해서 값이 없는 경우에만 예외를 발생시키기 때문에,
Optional을 직접 반환받지 않고도 안정적으로 구현할 수 있다고 생각합니다!
(Optional을 반환받은 쪽에서도 if문을 통해 한번 더 확인하고, 예외를 던져야하는 번거로움이 생길수도 있구요)
하지만 만약 요구사항에 메뉴가 존재하지 않는 경우, 메뉴판을 준다와 같이 추가적인 처리가 필요한 경우에는
Optional.empty()를 반환하고, 추가적인 처리를 하는 것이 더 안정적일 것 같아요
결론적으로 요구사항에 맞춰서 추가적인 처리가 필요할 때에는 Optional을 그렇지 않을 때에는 기본적으로 .orElseThrow를 사용하는 편인 것 같습니다!
|
|
||
| import static christmas.domain.consumer.constants.PlannerConstraint.PROMOTION_MONTH; | ||
| import static christmas.domain.consumer.constants.PlannerConstraint.PROMOTION_YEAR; | ||
|
|
There was a problem hiding this comment.
저는 VisitDay의 인스턴스 필드를 int로 두다보니 사실상 DTO처럼 밖에 사용이 안되었는데,
LocalDate를 활용 하신점, 그리고 래핑해서 사용하신 점이 정말 좋은 것 같습니다. 배워갑니다! 👍
| import static christmas.domain.consumer.constants.PlannerConstraint.MINIMUM_APPLICABLE_PURCHASE_TOTAL_PRICE; | ||
| import static christmas.domain.promotion.constants.PromotionPeriod.MONTHLY_DECEMBER; | ||
| import static christmas.domain.promotion.constants.PromotionPeriod.UNTIL_CHRISTMAS; | ||
|
|
There was a problem hiding this comment.
프로모션 조건 자체를 캡슐화할 생각은 전혀 못했는데 굉장히 신박한 것 같습니다.. 👍
-
조건 자체를 Enum으로 캡슐화해서 관리하면 어떤 이점이 있을 것으로 판단하셨는지 여쭙고 싶어요!
-
그리고
PromotionCondition은할인 프로모션에 대한 조건,배지 프로모션에 대한 조건을 모두 담고 있는데, 두 조건의 변경 사이클은 충분히 달라질 수도 있을 것 같아요. 그런 측면에서 둘을 분리하여 각각discount,gift패키지에 위치해 두는 것은 어떨까요? -
그리고
Orders를 참조함으로 인해consumer <-> promotion패키지 간 순환 참조가 발생하는 것으로 보입니다. 이 부분에 대한 해결책을 드리고 싶지만 저도 마땅히 떠오르진 않는데,, 어떤 방법이 있을까요? 🤔
Enum에서 Domain 객체를 의존해도 되는가?에 대해 디코 토론방에 글을 올렸는데, 한 번 슥 보고 오셔도 좋을 것 같습니다. 이야기 나누고 싶네요 :)
| import christmas.domain.consumer.VisitDay; | ||
|
|
||
| import java.time.LocalDate; | ||
|
|
There was a problem hiding this comment.
날짜를 하드코딩했던 저로썬 LocalDate을 활용하신게 너무나도 훌륭한 것 같습니다 :)
| return Arrays.stream(BadgePromotion.values()) | ||
| .filter(promotion -> promotion.isPromotionPeriod(visitDay)) | ||
| .filter(promotion -> promotion.isApplicable(totalBenefit)) | ||
| .findFirst() |
There was a problem hiding this comment.
저도 해빈님 의견에 동의합니다.
열거형 상수들의 순서에 의존적인 비즈니스 로직은 위험한 요소라 생각됩니다!
열거형 상수들은 해당 Enum 타입의 static final 인스턴스 라 볼 수 있는데, 마치 인스턴스 필드 배치 순서에 따라 public API의 결과물이 달라지는 것과 동일하다고 생각됩니다 :)
이펙티브 자바 서적에 Enum ordinal 관련 내용이 있어서 참고해보시면 좋을 것 같습니다 :)
There was a problem hiding this comment.
배지에 대한 조건을 Predicate 을 활용하신게 정말 좋은 전략 같습니다 👍
배지 없음을 나타내기 위한 수단으로 Optional도 고려해보시면 좋을 것 같습니다. DEFAULT로 없음을 나타내는 전략도 훌륭하다 생각합니다:)
| import static christmas.domain.promotion.badge.BadgePromotion.DEFAULT; | ||
| import static christmas.view.constants.ResponseFormat.PRICE_RESULT; | ||
| import static christmas.view.constants.ResponseMessage.*; | ||
|
|
There was a problem hiding this comment.
PromotionController 전반적인 리뷰입니다!
아무래도 Promotion에 대한 모든 내용을 처리하다보니 필연적으로 메서드가 길어진 것 같습니다. 하지만 Promotion도 할인 이벤트 프로모션, 증정 이벤트 프로모션, 배지 프로모션으로 나뉘게 되니, 각각에 대한 Service Layer를 구상하신다면 PromotionController가 한층 얇아지는데 도움이 될 것 같습니다 🤔
There was a problem hiding this comment.
점점 MVC 3계층이 MVC 5계층으로 변하게 된 이유를 실감하는 것 같아요!
비즈니스 로직이 복잡해지고, 이를 도메인에 모두 담으려고 하니, 서비스 로직의 책임까지 지면서
관리해야할 인자도 늘어나고, 전체적으로 강한 결합을 보이는 것 같아요!
Service Layer로 분리하면서 설계하는 방향으로 리팩토링 해봐야겠어요!!
좋은 리뷰 감사드립니다 :)
There was a problem hiding this comment.
안녕하세요 혜빈님!
리뷰를 남기려고 코드를 내려받고 리뷰할 내용을 찾아서 정리했지만 이미 다른 리뷰어분들께서 이미 말씀해주셨네요 :)
어쩔 수 없이 남은 픽스처 부분만 리뷰 남기게 된 점 양해 부탁드립니다 ㅎㅎ
먼저 테스트 대상 시스템 (System Under Test, 이하 SUT) 를 실행하기 위해 해줘야 하는 모든 것을 테스트 픽스처라고 부릅니다.
테스트 픽스처가 등장하게 된 계기는 매 테스트마다 SUT을 생성할 때 중복되는 생성 코드가 발생하는데
이는 너무 많은 사전 준비가 필요하기 때문에 중복을 줄이기 위해서로 알고 있습니다.
보통 테스트 클래스내부에서 지역변수로 만들어서 setup메서드를 통해 초기화하는 방법을 사용합니다.
이방법은 모든 테스트가 setup에 강하게 의존하고 있기 때문에 setup의 코드를 수정하면 다른 테스트가 실패할 수 있기 때문에 권장하지 않습니다.
두 번째 방법은 테스트 클래스 내부에서 private 메서드 형태로 테스트 픽스처를 생성할 수 있는 형태로 제공하는 방법입니다.
이전에 설명드렸던 사용자 생성 예시를 떠올리시면 될 것 같습니다 :)
혜빈님께서 사용하신 방법은 두번째 방법과 유사하지만 조금 다른점이 있습니다.
팩토리 형태로 제공하는 것과 달리 테스트 하는 대상 클래스를 미리 생성한 방법입니다.
이는 테스트 메서드에서 어떤 값을 테스트하고 있는지 확인하기 위해서 직접 테스트 픽스처 클래스 코드를 직접 봐야하기 때문입니다.
아마 예시로 보시면 조금 더 쉽게 이해할 수 있으리라 생각합니다!
도움이 될 만한 참고 자료를 몇개 가져왔습니다 :)
아래는 우테코 지원 페이지에서 사용하는 테스트 픽스처
https://github.com/woowacourse/service-apply/blob/master/src/test/kotlin/apply/UserFixtures.kt
개인적으로 테스트 관련해서 가장 인상 깊게 봤던 책 (91p 참고)
https://www.yes24.com/Product/Goods/104084175
테스트 픽스처와는 관련 없지만 도움이 될 것 같아서 가져왔습니다
https://enterprisecraftsmanship.com/posts/dry-damp-unit-tests
There was a problem hiding this comment.
Fixture 설계를 꼭 적용해, 우테코에서 추구하는 간결한 테스트코드를 작성하고자 했던 소망이 있기에,
꼭 깊이있게 학습하고 연구해서 실제 프로덕트 테스트 코드에 녹여내고 싶었습니다 :)
이 부분에 대해서 좋은 레퍼런스를 많이 참고해주셔서 진심으로 감사드립니다!!
코틀린 코드여서, 다소 읽기에는 어려움이 있지만, 직관적으로 어떤 설계다! 라는 부분은 캐치가 가능했습니다!!
두 번째 방법은 테스트 클래스 내부에서 private 메서드 형태로 테스트 픽스처를 생성할 수 있는 형태로 제공하는 방법입니다. 이전에 설명드렸던 사용자 생성 예시를 떠올리시면 될 것 같습니다 :)
재홍님께서 남겨주신 부분을 참고해서 공부해보고,
혹시 궁금한 점이 생기면 다시 리뷰 남기겠습니다!!
깊이있는 리뷰 감사드립니다 :)
4주간 정말 감사했어요!!
There was a problem hiding this comment.
주문이라는 도메인 객체 내에서 다양한 로직들을 처리함에도 일관성이 있는 모습이 좋은 것 같습니다!
| VisitDay visitDay = VisitDayController.requestVisitDay(); | ||
| Orders orders = OrderController.requestOrders(); | ||
|
|
||
| VisitDayController.responseVisitDay(visitDay); | ||
| OrderController.responseOrdersResult(orders); | ||
| OrderController.responseTotalOriginPriceResult(orders); | ||
| PromotionController.responseAppliedBenefitResult(visitDay, orders); |
There was a problem hiding this comment.
저도 이번 미션을 하면서 Controller를 분리했었는데, 코드를 다시 보니 Controller의 분리 자체를 먼저 고민했어야 했더라구요.
물론 현재 미션의 구조에서는 네이밍 정도의 차이이겠지만, 컨트롤러가 컨트롤러를 가지는 패턴에 대해 고민하기 전 컨트롤러 자체의 역할에 대해 고민해보시면 좋을 것 같습니다.
제 경우는 컨트롤러가 아닌 서비스에 속했어야 됐다는 판단을 하게되었었거든요.
물론 컨트롤러가 이렇게 분리되어야 한다는 결론이 나온다면 현재 방법도 나쁘지 않다고 생각합니다!
| AppliedDiscountPromotions discountPromotions = AppliedDiscountPromotions.create(visitDay, orders, defaultBadge); | ||
| AppliedGiftPromotions giftPromotions = AppliedGiftPromotions.create(visitDay, orders, defaultBadge); | ||
| BadgePromotion badge = BadgeContext.applyPromotions(visitDay, discountPromotions, giftPromotions); | ||
|
|
||
| final int totalDiscountAmount = discountPromotions.getExpectedPayment(orders); | ||
| final int totalBenefit = giftPromotions.getTotalBenefit(discountPromotions); |
There was a problem hiding this comment.
이벤트 결과의 저장에 대해서 저도 고민해봤었고, 실제로 기능을 추가하기도 했었습니다. 다만 제 결론은 미션은 이벤트 플래너로 예상 방문일과, 예상 주문에 따른 혜택을 미리 보여주는 것이라고 이해했기 때문에 실제 배지를 제공하는 부분은 일어나서는 안된다고 생각해 기능을 제거했습니다. 실제 방문일에 주문하는 메뉴가 달라지는 등의 상황이 있을 수 있기 때문입니다.
물론 이 부분은 다양한 해석으로 바라볼 수 있을 것 같습니다.
이 로직이 컨트롤러가 아닌 서비스에 존재해야 하는 것에 대해서는 위에서도 남겼듯 이 부분은 컨트롤러에 알맞은 로직인가에 대한 고민이 한 번 더 이루어져야 할 것 같다고 생각합니다.
| public class Orders { | ||
| private static final int ORDERS_MAXIMUM_RANGE = 20; | ||
|
|
||
| private final EnumMap<Menu, Integer> menus; |
There was a problem hiding this comment.
저도 Orders에서 Map을 사용하도록 구현했는데 코드리뷰를 받고 다시 생각해보니 Order 클래스를 따로 만들고 Order 클래스들을 담는 일급 컬렉션의 개념으로 Orders를 사용하는 것이 더 낫다는 판단을 하게 되었습니다.
물론 Map을 사용해 Orders를 구현하는 방식도 장점이 있지만 Order 클래스를 따로 만들었을 때의 장점도 생각해보시면 좋을 것 같아요!
| public static Orders requestOrders() { | ||
| OrderOutputWriter.printMessageResponse(REQUEST_MENU_ORDERS); | ||
| return ExceptionHandler.retryOnBusinessException(OrderController::createMenuOrdersFromInput); | ||
| } |
There was a problem hiding this comment.
저도 의견을 남겨도 괜찮을까요?!
저도 과제 중간에 static을 사용해서 controller와 view를 관리했었는데요!
다음 글을 읽고 이번에는 조금 static을 지양하려고 했었습니다.
물론 static의 사용 목적이 이번 과제로만 국한 했을 때 얘기가 달라질 수도 있지만, 참고할 만한 내용이라 생각해서 공유합니다!
https://unabated.tistory.com/1041
@h-beeen님 의견은 어떠신가요?
|
|
||
| import static christmas.exception.ErrorCode.INVALID_DATE; | ||
|
|
||
| public class ExceptionHandler { |
There was a problem hiding this comment.
덕분에 record라는 개념에 대해 새롭게 알게 되었습니다.
dto에 깔끔하게 적용하셔서 정말 좋은 것 같습니다.
| this.menus = menus; | ||
| } | ||
|
|
||
| //== Static Factory Method ==// |
There was a problem hiding this comment.
정적 팩토리 메서드 네이밍 컨벤션에 관하여 질문드립니다!
정적 팩토리 메서드 이름을 create로 지으셨는데, 저는 아래 첨부드린 사이트에서 네이밍 컨벤션� 상 여러개의 매개 변수를 받아서 객체를 생성할 땐 of를 사용해야 한다고 판단했습니다.
혹시 제가 잘 못 이해한 부분이 있을 지 여쭙고 싶습니다!
참고
- https://tecoble.techcourse.co.kr/post/2020-05-26-static-factory-method/
- https://inpa.tistory.com/entry/GOF-%F0%9F%92%A0-%EC%A0%95%EC%A0%81-%ED%8C%A9%ED%86%A0%EB%A6%AC-%EB%A9%94%EC%84%9C%EB%93%9C-%EC%83%9D%EC%84%B1%EC%9E%90-%EB%8C%80%EC%8B%A0-%EC%82%AC%EC%9A%A9%ED%95%98%EC%9E%90#2._api_%EB%AC%B8%EC%84%9C%EC%97%90%EC%84%9C%EC%9D%98_%EB%B6%88%ED%8E%B8%ED%95%A8
There was a problem hiding this comment.
저도 정적 팩토리 메서드 네이밍에 대해 고민한 적이 있는데요..!
@NaMooJoon 님 말씀대로, 여러 개의 매개 변수를 받아서 객체를 생성할 때에 of를 사용하는 것을 권장하고 있지만,
create같은 경우, 항상 새로운 인스턴스를 생성할 때 사용할 수 있으므로 create의 사용도 괜찮을 것 같습니다!
|
많은 분들과 함께한 리뷰를 통해서 정말 많이 배웠습니다. 방법에 대해 여러 의견을 주고받으면서 더 나은 코드를 고민하는 모습이 인상적입니다. 덕분에 좋은 코드 뿐만 아니라 리뷰의 긍정적 효과 또한 배웠습니다! 4주간 고생 많으셨습니다! |
| OrderOutputWriter.printNewLine(); | ||
| OrderOutputWriter.printMessageResponse(RESPONSE_MENU_ORDERS_RESULT); | ||
| OrderOutputWriter.printMenuOrdersResponse(orderResponse); |
There was a problem hiding this comment.
정말 어려운 트레이드 오프인 것 같네요 레페런스 참고해서 학습해보겠습니다!!
| entry -> entry.getKey().getName(), | ||
| Entry::getValue) | ||
| ); | ||
|
|
There was a problem hiding this comment.
중복처리를 이렇게하면 되게 간단하게 처리할 수 있군요... entrySet이라니... 잘 배워 갑니다!
There was a problem hiding this comment.
헉..! entrySet()이라는게 있었군요...! 매주 코드 리뷰를 하면서 몰랐던 api들을 많이 배워갑니다..!! 👍
| "없음", | ||
| ALWAYS, | ||
| always -> true | ||
| ); |
| private OrderController() { | ||
| } |
There was a problem hiding this comment.
이런 식의 방법도 있군요 😮 방법에 정답은 없기에 고려해볼만한 방법인 것 같습니다.
| private static void responseTotalBenefitResult(final int totalBenefit) { | ||
| PromotionOutputWriter.printNewLine(); | ||
| PromotionOutputWriter.printMessageResponse(RESPONSE_TOTAL_BENEFIT_RESULT); | ||
| PromotionOutputWriter.printTotalBenefitResponse(totalBenefit); | ||
| } |
There was a problem hiding this comment.
저는 개인적으로 하나로 묶는 방법이 더 낫다고 생각합니다. 해당 메소드를 비롯한 다른 형식의 출력 메소드는 요구사항대로 출력하기 위한 기능들을 묶어서 호출하는 것으로 보입니다.
이 방법은 반복되는 메소드 호출 작업을 줄인다는 장점도 있지만 요구사항을 만족하기 위한 출력을 통일시키고 강제하는 효과도 있다고 생각합니다. 마치 로또 과제의 Lotto 객체의 생성자 안에서 검증 메소드를 넣어서 생성시점에 유효성 체크를 강제하는 것 처럼 말이죠.
더 나아가서 협업을 할때에도 요구사항을 전혀 몰라도 이 메서드 호출을 통해 요구사항을 만족하는 올바른 출력기능을 사용할 수 있기 때문에 더 나은 방법이라고 생각합니다.
좋은 유지보수인가 라는 관점에서도 기존의 출력 방식과 다른 출력을 원할때에도 새로운 메소드를 만들어서 호출하는 부분만 바꾸면 되니까 적절한 방법이지 않을까요? 만약 출력하는 방식이 여러개라서 이런식으로 만들어야하는 메소드가 많아 진다면 그건 이미 묶는것의 의미가 없으니 논외라고 생각합니다.
| VisitDay visitDay = VisitDayController.requestVisitDay(); | ||
| Orders orders = OrderController.requestOrders(); | ||
|
|
||
| VisitDayController.responseVisitDay(visitDay); | ||
| OrderController.responseOrdersResult(orders); | ||
| OrderController.responseTotalOriginPriceResult(orders); | ||
| PromotionController.responseAppliedBenefitResult(visitDay, orders); |
There was a problem hiding this comment.
@h-beeen 은 각각 역할을 다르게 하는 컨트롤러 이기 때문에 분리하신것으로 이해됩니다! 제 생각엔 '각 컨트롤러가 역할이 명확해지고 조금 더 간결해진다'가 분리 함으로써 얻을 수 있는 이점이라고 생각하는데, 이 외에 더 얻을 수 있는 이점이 있을까요?
| private static void responseTotalBenefitResult(final int totalBenefit) { | ||
| PromotionOutputWriter.printNewLine(); | ||
| PromotionOutputWriter.printMessageResponse(RESPONSE_TOTAL_BENEFIT_RESULT); | ||
| PromotionOutputWriter.printTotalBenefitResponse(totalBenefit); | ||
| } |
There was a problem hiding this comment.
정답이 없는 문제라지만, 유지보수성, 책임관계, 타인과의 협업 등 신경쓸게 많은 문제네요! 저는 타인과의 협업, 코드의 가독성을 높이기 위해 하나의 메소드로 묶는것을 선호하는데, 다른분들은 어느 곳에 중점을 두시는지 궁금해지는 문제네요! ㅎㅎ
| this.name = name; | ||
| this.price = price; | ||
| } | ||
|
|
There was a problem hiding this comment.
저도 @june-777 님의 질문에 대한 다른분들의 의견이 궁금합니다!
저는 find 라는 역할에 맞게 해당하지 않는 메뉴가 없다면 '없다' 라는 의미의 Optional.empty()를 반환하는 방향으로 설계했는데 다른 분들의 의견이 궁금합니다!
| AppliedDiscountPromotions discountPromotions = AppliedDiscountPromotions.create(visitDay, orders, defaultBadge); | ||
| AppliedGiftPromotions giftPromotions = AppliedGiftPromotions.create(visitDay, orders, defaultBadge); | ||
| BadgePromotion badge = BadgeContext.applyPromotions(visitDay, discountPromotions, giftPromotions); | ||
|
|
||
| final int totalDiscountAmount = discountPromotions.getExpectedPayment(orders); | ||
| final int totalBenefit = giftPromotions.getTotalBenefit(discountPromotions); |
There was a problem hiding this comment.
저도 결과 저장에 대해서는 '미리 보여준다'에 의거하여 미구현으로 남겨놓았어요!
배지 제공을 하기 위해선 '누가 언제 어떠한 주문을 통해 해당 배지를 받았다'라는 기록을 작성해야 할텐데 이는 실제 결제 시스템을 통해야 한다고 생각했거든요.
서비스에 해당 로직이 존재해야 한다는 의견에는 동의합니다!
| private static void responseGiftResult(GiftResponse giftResponse) { | ||
| PromotionOutputWriter.printNewLine(); | ||
| PromotionOutputWriter.printMessageResponse(RESPONSE_GIFT_RESULT); | ||
| PromotionOutputWriter.printGiftQuantityResponse(giftResponse); | ||
| } |
There was a problem hiding this comment.
저도 출력 포맷과 방법은 View에 책임이 있다고 생각했고, 컨트롤러에서는 데이터만 넘기는 방향으로 설계했습니다!
말씀하신대로 출력 방법이 단순 콘솔에서 터치스크린 등으로 변경될 경우 출력 메시지 형태는 바뀔 수 밖에 없다고 판단했어요.
따라서 인터페이스를 통해 특정 데이터 출력을 지정하고, 콘솔 뷰에서 보여줄 메시지 및 데이터 형식에 맞는 출력문구를 작성했습니다!
| DESSERT, | ||
| BEVERAGE; | ||
|
|
||
| MenuCategory() { |
There was a problem hiding this comment.
엇! Enum의 생성자는 기본적으로 private로 생성되기 때문에, 멤버가 없을 경우에는 생성자가 없어도 될 것 같은데
빈 생성자를 명시하신 이유가 궁금합니다!
There was a problem hiding this comment.
원래 생성자 내부 로직이 있었는데, 리팩토링 과정에서 제거하지 않은 로직이에요!
섬세한 리뷰 감사합니다 :)
| private boolean hasOnlyBeverages(EnumMap<Menu, Integer> menus) { | ||
| return menus.keySet() | ||
| .stream() | ||
| .allMatch(this::isBeverage); |
There was a problem hiding this comment.
와 저도 생각 못하고 필터 사용했었는데.. 정말 깔끔하게 구현이 가능한 부분이었군요!!
| @DisplayName("[OrderController] - Controller Layer") | ||
| class OrderControllerTest { | ||
|
|
||
| @Nested |
There was a problem hiding this comment.
안녕하세요, 해빈님! 세 번째 리뷰입니다.
이번에는 SonarLint 플러그인을 이용해 코드를 분석했습니다.
플러그인 설치 방법은 디스코드에 올렸어요.
한 번 설치해보시는 걸 추천드려요!
링크 : https://discord.com/channels/1149138870433230900/1179345425791205437/1179345425791205437
| private static final int THURSDAY_VALUE = 4; | ||
| private static final int SUNDAY_VALUE = 7; | ||
|
|
||
| private final LocalDate visitDay; |
There was a problem hiding this comment.
🤔고민🤔
SonarLint 에 따르면, 필드명은 클래스명과 중복되지 않는게 좋다고 합니다.
클래스명과 같다면 혼란을 줄 수 있다고 합니다.
조금 더 설명할 수 있는 이름으로 변경하는 걸 권장합니다.
Bad Practice
public class Foo {
private String foo;
public String getFoo() { }
}
Foo foo = new Foo();
foo.getFoo() // what does this return?Best Practice
public class Foo {
private String name;
public String getName() { }
}
//...
Foo foo = new Foo();
foo.getName()| private final LocalDate visitDay; | |
| private final LocalDate visitDate; |
There was a problem hiding this comment.
이 부분에 대해서도 인지하고 있었지만, 고민이 많았던 로직이에요.
특히나, 일급 컬렉션 객체의 변수명을 짓는 과정은 꽤나 많은 고민을 하게 만들었어요.
여전히 변수명을 짓는데 오랜 시간을 할애하고, 고민하지만, 직관적인 변수명을 작명하는데는
어려움이 많은 것 같아요 :(
소나린트도 사실 깔려는 있지만, 이번 과제에서 많이 활용하지는 않았던 것 같아요...!
린트를 준수하면서 조금 더 좋은 코드 작성을 하도록 노력해볼게요 :)
| public EnumMap<DiscountPromotion, Integer> applyPromotions( | ||
| VisitDay visitDay, | ||
| Orders orders | ||
| ) { |
There was a problem hiding this comment.
🤔고민🤔
SonarLint 에 따르면, 구체적인 구현체인 EnumMap를 반환하는 것 보단 인터페이스인 Map 을 사용하는 걸 권장한다고 합니다. EnumMap 을 반환하고 외부에서 계속 쓰는 것 보다 Map 을 사용하는 건 어떨까요? 구현체를 외부로 노출해선 안된다고 주장합니다.
Java Collections API는 수집 구현 세부 정보를 숨기도록 설계된 인터페이스의 잘 구성된 계층 구조를 제공합니다.
목록, 집합, 지도 등 다양한 수집 데이터 구조의 경우 특정 인터페이스(java.util.List, java.util.Set, java.util.Map)가 필수 기능을 포함합니다.
컬렉션을 메서드 매개 변수, 반환 값으로 전달하거나 필드를 노출할 때는 구현 클래스 대신 이러한 인터페이스를 사용하는 것이 일반적으로 권장됩니다.
java.util.LinkedList, java.util.ArrayList 및 java.util.HasMap과 같은 구현 클래스는 컬렉션 인스턴스화에만 사용해야 합니다.
이러한 구조의 성능 특성을 보다 세밀하게 제어할 수 있으며 개발자는 사용 사례에 따라 선택합니다.
예를 들어 빠른 랜덤 요소 접근이 필수적이라면 java.util.ArrayList를 인스턴스화해야 합니다.
만약 임의의 위치에 요소를 삽입하는 것이 중요하다면 java.util.LinkedList를 선호해야 합니다.
하지만 이것은 API가 노출해서는 안 되는 구현 정보입니다.
| public EnumMap<DiscountPromotion, Integer> applyPromotions( | |
| VisitDay visitDay, | |
| Orders orders | |
| ) { | |
| public Map<DiscountPromotion, Integer> applyPromotions( | |
| VisitDay visitDay, | |
| Orders orders | |
| ) { |
| // when | ||
| int totalDiscountAmount = discountPromotions.getTotalDiscountAmount(); | ||
| // then | ||
| assertEquals(totalDiscountAmount, 6_446); |
There was a problem hiding this comment.
🤔고민🤔
assertEquals 의 시그니처는 다음과 같습니다.
assertEquals(int expected, int actual)
현재 코드에서 expected 의 역할과 actual 의 역할이 서로 뒤바뀐 것 같습니다.
두 개의 파라미터를 서로 뒤 바꿔보는 건 어떨까요?
테스트 결과에는 문제가 없지만, 테스트 실패 오류 메시지가 잘못 표기될 수도 있습니다.
The standard assertions library methods such as org.junit.Assert.assertEquals, and org.junit.Assert.assertSame expect the first argument to be the expected value and the second argument to be the actual value. For AssertJ instead, the argument of org.assertj.core.api.Assertions.assertThat is the actual value, and the subsequent calls contain the expected values.
What is the potential impact?
Having the expected value and the actual value in the wrong order will not alter the outcome of tests, (succeed/fail when it should) but the error messages will contain misleading information.
This rule raises an issue when the actual argument to an assertions library method is a hard-coded value and the expected argument is not.
README.md는 PR 환경에서 최적화되어 작동합니다. PR 코드리뷰 페이지에서 깔끔한 UI로 확인이 가능합니다 :)🎄 126명의 리뷰어에게 전하는 프리코스 마지막 이야기
▪️ 리뷰가 많아 로딩이 오래 걸립니다!
🦄 오류 발생시 새로고침 하거나 네트워크 상태를 확인해 주세요!!
▪️ 프리코스를 열심히 완주한 당신! 이번 크리스마스 🎄
우테코 식당으로 모여주세요.맛있는
1,554원의 아이스크림과별 배지가 기다리고 있답니다.▪️ 지난 3주 동안, 성장곡선을 지수함수로 그릴 수 있도록 만들어 주신 4주간 101명의 크루 여러분
크리스마스에 우테코 식당에서 같이 아이스크림 먹어요!
(제가 사겠습니다)▪️ 추신)
지수함수의 정의역, 치역이 0인 지수함수라고 가정합니다.(완전 가파르다는 뜻) 참고로 저는
문과입니다. ^____^!📦 패키지 구조
➜ dto
➜ consumer
➜ constants
➜ promotion
➜ badge
➜ promotion
➜ discount
➜ promotion
➜ gift
➜ promotion
➜ constants
➜ utility
➜ constants
➜ input
➜ output
✨ 구현 목록
1️⃣ Non-Functional Requirement
✅ 크리스마스 프로모션 프로그램은 java17 환경에서 MVC 패턴에 따라 설계한다.
✅ 1월 이벤트 간, 본 프로그램을 추가 개발하여 활용할 예정이므로 YAGNI 원칙에 입각해 개발한다.
✅ 필요 구현 요소를 구현함에 있어, 추후 요구 조건을 염두하고, 확장성있게 구현하는 것을 목표로 한다.
✅ DTO/Response/Test 계층에서 사용하는
@Getter를 제외하고,@Getter,@Setter사용을 금지한다.2️⃣ Functional Requirement (Controller Code Flow)
✅ [Application.main] VisitDayController.requestVisitDay()
✅ [Application.main] OrderController.requestOrders(visitDay)
✅ [Application.main] VisitDayController.responseVisitDay(visitDay);
✅ [Application.main] OrderController.responseOrdersResult(orders);
✅ [Application.main] OrderController.responseTotalOriginPriceResult(orders);
✅ [Application.main] PromotionController.responseAppliedBenefitResult(visitDay, orders);
✅ [Application.main] Console.close();
🆘 고민일기
1️⃣ 단위 테스트의 비용과 효능
2️⃣ Enum + Functional Interface VS Strategy Pattern
✅ 전략 패턴으로 최초 구현(8e5400)
✅ Enum + 함수형 인터페이스 사용 리팩토링(ea786c)
👬 101명과 함께한 906개의 소중한 코드리뷰 적용기
[숫자 야구] 31명과 함께한 224개의 이야기
(🦄 오류 발생시 새로고침 해주세요!)
[자동차 경주] 39명과 함께한 296개의 이야기
(🦄 오류 발생시 새로고침 해주세요!)
[로또] 22명과 함께한 213개의 이야기
(🦄 오류 발생시 새로고침 해주세요!)
🌱 0x00
자식 클래스가부모 클래스인가? LSP 원칙에 대한 고찰graph TD; OutputWriter --> ErrorOutputWriter OutputWriter --> OrderOutputWriter OutputWriter --> PromotionOutputWriter OutputWriter --> VisitDayOutputWriter🌱 0x01
Fixture 방식 테스트 코드설계 간 가독성의 문제점🌱 0x02 예외 발생 간 재귀를 활용한 재요청시, Stack Overflow Issue
🌱 0x03
검증 메소드의 네이밍 컨벤션에 대해서🌱 0x04
인터페이스에 일반 구현메소드 작성 방법🌱 0x05
중복되는 테스트 코드를 줄이는 방법🌱 0x06 DTO VS Mapper Class 설계
🌱 0x07 방어적 복사에 대해서
출처 : [3주차] 로또 코드리뷰 중 발췌 후 정리
참고 : [Java] Collection 복사 - 복사 방법(방어적 복사, 얕은 복사, 깊은 복사) 및 상황별 최적의 복사 방법
Thanks To @SSung023
🌱 0x08
컨트롤러의호출과책임📝 개발 시 작성한 기능 구현 목록
✅ 사용자에게 방문 날자 메세지 출력 후 요청 ->
Return VisitDay✅ 사용자에게 예상 방문 날짜 입력 (1~31)
✅ 숫자가 아닌 문자열 예외처리
✅ System Constraint 기준으로 해당 달의 달력에 없는 날짜 예외처리
✅ 사용자에게 메뉴 입력 메세지 출력 후 요청 ->
Return Orders✅ 자체 제약조건 : 음식은 무조건 한글이다. (제로콜라, 타파스가 영어임을 생각했을때)
✅ 콤마로 끝나거나, 스페이스바가 들어가거나, 정규표현식 패턴이 맞지 않거나 whiteSpace를 포함하지 않을 때 예외처리
✅ input을 콤마 단위로 파싱하기 (파싱 후
한글-숫자형태 검증) -> 정규표현식✅ 정규표현식) 단일 메뉴 주문 갯수가 1~20 범위 밖이면 예외처리
✅ 정규표현식) 파싱된 각 인자가
음식-숫자형식이 아닐경우 예외처리✅ Orders 생성자 호출 -> EnumMap 일급 컬렉션 객체 생성
✅ Orders) 메뉴판에 없는 메뉴 예외처리
✅ Orders) 메뉴 전체의 총 합이 20개를 초과 할 경우 예외처리
✅ Orders) 음료만 주문하면 예외처리
✅ 주문 메뉴 출력
✅ 할인 전 총 주문 금액 출력
✅ DailyDiscount (12/1 ~ 12/25)
✅ WeekdayDiscount 평일 할인 (일,월,화,수,목) 디저트 메뉴당 2,023원 할인
✅ WeekendDiscount 주말 할인 (금, 토) 메인 메뉴 1개당 2,023원 할인
✅ SpecialDiscount 매주 일요일 + 크리스마스(별이 있으면) 총 주문금액 1,000원 할인
✅ (매주 일요일 + 크리스마스 당일) : 총 주문금액에서 1,000원 할인 -> 날짜 enum 하드코딩
✅ 최종 할인가를 종합해 증정 이벤트 판별
✅ 마지막 최종 주문금액 산출 후 // 혜택 금액에 증정가 추가
✅ 새해 선물 : 총 혜택 금액에 따른 배지 부여
✅ 증정 이벤트 : 총 주문 금액 12만 이상시 샴페인 증정(혜택 25,000원!!)
✅ 증정 메뉴 출력 : 총 혜택 금액을 바탕으로
- TotalDiscountPrice
✅ 혜택 내역 출력 : 총 할인 금액과, 증정 메뉴 가격을 바탕으로
- Entry<DiscountPromotion, DiscountedAmount> + TotalGiftPrice
✅ 총 혜택 금액 출력 : 총 할인 금액과 증정 메뉴 가격을 바탕으로
- TotalDiscountAmount + TotalGiftPrice
✅ 할인 후 예상 결제 금액 : 총 주문 금액와 총 할인 금액을 바탕으로
- TotalOriginPrice - TotalDiscountAmount
✅ 이벤트 배지 : 총 혜택 금액을 바탕으로
- 5000원 - 별
- 10000원 - 트리
- 20000원 - 산타
Special Thanks to Human Papago 🦜@saeyeonn during 4 Weeks