[재재구현] 함지수 미션 제출합니다.

[jisu-om]

1. DTO가 도메인 -> 자기자신 으로이 변환 로직을 들고있어도 괜찮을지?
   따로 Mapper 를 두지 않았는데 Mapper를 두는게 한번에 관리할 수 있어 좋을 것 같기도 합니다.
   ex. EventDto, OrderItemDto, OrdersDto, ResultDto 의 from()
   특히 ResultDto 의 from() 의 경우에는 로직이 많이 들어가 있어서 service layer 로 만들어도 괜찮았을 것 같습니다.
   추가로 ResultDto 의 생성로직이… 너무 광범위한 것 같습니다. 이렇게 하지 않는게 좋다고 생각이 드는데 어떻게 생각하실까요?

2. Quantity를 VO로 만들어 사용하였는데, OrderItem 에서만 쓰이고, OrderItem이 Orders 로 포장되면서 Quantity를 제대로 사용하지 못하는 것 같습니다.

3. 할인의 이름(ex. 크리스마스 디데이 할인, 평일 할인 등) 을 model 의 getName() 으로 처리하였는데, 뷰 로직이 모델에 들어간 느낌을 받았습니다.
   Enum 으로 만들어 관리할까도 생각했지만 사용하고 싶을 때 어떻게 꺼내와야 할지 감이 안와서 (Enum 의 첫번째 필드는 이름, ChristmasDiscount.class 를 두번째 필드로 하여 만들어볼까 했는데,, 방법을 못찾겠어서 그리고 돌아가는 느낌이라 그냥 getName()으로 했습니다.

4. Reservation 과 MatchingEvents 의 관계
   Reservation 은 MatchingEvents를 생성하는 역할, 추가적인 가격 계산(matchingEvents 가 하지 못하는) 을 담당하는 객체로 생각하는데,
   혹시 이에 대해 좀더 좋은 설계 추천해주실 수 있으실까요?

아예 Reservation 이 MatchingEvents 를 필드로 갖게 할까 했는데 (아래 2번의 방식) (이렇게 하면 ResultDto 의 생성이 좀더 쉬워지는 것처럼 보임) 이렇게 하면 단일책임원칙을 크게 위반하여 다시 1번 방식으로 수정하였습니다.

//1번 방식
public class Reservation {
    private final VisitingDate date;
    private final Orders orders;

    public Reservation(VisitingDate date, Orders orders) {
        this.date = date;
        this.orders = orders;
    }
    
    public int calculateFinalPrice() {
        return orders.calculateTotalPrice() - findMatchingEvents().calculateTotalDiscountAmount();
    }

    public MatchingEvents findMatchingEvents() {
        List<Event> allEvents = Arrays.asList(new ChristmasDiscount(date), new WeekdayDiscount(date, orders),
                new WeekendDiscount(date, orders), new SpecialDiscount(date), new GiveawayEvent(orders));

        List<Event> events = allEvents.stream()
                .filter(Event::isApplied)
                .toList();

        return new MatchingEvents(events);
    }
}

public class MatchingEvents {
    private final List<Event> events;

    public MatchingEvents(List<Event> events) {
        this.events = events;
    }

    public int calculateTotalBenefitAmount() {
        return events.stream()
                .mapToInt(Event::getBenefitAmount)
                .sum();
    }

    public int calculateTotalDiscountAmount() {
        if (containsGiveaway()) {
            int giveawayBenefitAmount = events.stream()
                    .filter(event -> event instanceof GiveawayEvent)
                    .mapToInt(Event::getBenefitAmount)
                    .findFirst()
                    .orElseThrow(() -> new IllegalStateException(INVALID_GIVEAWAY_BENEFIT_AMOUNT.getMessage()));
            return calculateTotalBenefitAmount() - giveawayBenefitAmount;
        }
        return calculateTotalBenefitAmount();
    }

    public boolean containsGiveaway() {
        return events.stream()
                .anyMatch(event -> event instanceof GiveawayEvent);
    }

    public List<Event> getMatchingEvents() {
        return List.copyOf(events);
    }
}

//2번방식
//MatchingEvents는 그대로 유지하고, Reservation 의 필드로 MatchingEvents 추가 
//-> 단일책임원칙을 위반한 것 같음. 
//1번 방식에서는 Reservation이 MatchingEvents를 생성하는 역할 + 할인 전후 가격 계산 담당. MatchingEvents 는 Event 관련 로직을 담당
//2번 방식은 둘이 혼합되어 Reservation 에 몰빵된 느낌 

public class Reservation {
    private final VisitingDate date;
    private final Orders orders;
    private MatchingEvents matchingEvents;

    public Reservation(VisitingDate date, Orders orders) {
        this.date = date;
        this.orders = orders;
        setMatchingEvents();
    }

    private void setMatchingEvents() {
        List<Event> allEvents = Arrays.asList(new ChristmasDiscount(date), new WeekdayDiscount(date, orders),
                new WeekendDiscount(date, orders), new SpecialDiscount(date), new GiveawayEvent(orders));

        List<Event> events = allEvents.stream()
                .filter(Event::isApplied)
                .toList();

        matchingEvents = new MatchingEvents(events);
    }

    public int calculateFinalPrice() {
        return orders.calculateTotalPrice() - matchingEvents.calculateTotalDiscountAmount();
    }

    public int calculateTotalBenefitAmount() {
        return matchingEvents.calculateTotalBenefitAmount();
    }

    public MatchingEvents getMatchingEvents() {
        return matchingEvents;
    }
}

5. OutputView 의 printGiveaway()
   증정 이벤트 상품을 꺼내오기 위해 방편을 GiveawayEvent 의 getGiveawayItemName()을 static으로 하여 DTO에 넣고 꺼내왔는데,
   이것 말고 상수 클래스로 관리하였어야 했는지 궁금합니다.

6. BadgeFinder 를 사용하지 않고 Reservation 이 Badge 도 생성하도록 하면 어떨까요? BadgeFinder 의 기능이 하나뿐인데 이렇게 만들어도 괜찮은건지 궁금합니다.

[junpakPark]

안녕하세요 지수님! 5기 준팍입니다.

질문도 좋고, 코드도 아주 깔끔하게 리팩터링해주셨네요 ㅎㅎ

1. 도메인을 DTO로 변환하는 로직을 정적 팩터리 메서드로 DTO 내부에 두는 건 좋은 아이디어라고 생각합니다. 5기 프로젝트에서도 많은 팀들이 Mapper를 사용하기보다는 이 방식으로 처리하는 것을 선호하는 편이었어요 ㅎㅎ
   이번 코드에서 유일하게 아쉬웠던 부분이 Reservation과 너무 많은 DTO들, OutputView 였는데
   스스로 느끼셨군요? ㅎㅎ 굉장히 빠르신데요? 👍👍👍
   지금 생각하신 해결방법대로 적용해보시면 될 것 같습니다.
2. Quantity의 목적은 무엇일까요? 이름 그대로 수량을 나타내는 것이겠죠?
   목적대로 잘 사용하셨습니다 ㅎㅎ
3. 어떤 이유에서 뷰로직이 모델에 들어간 느낌을 받으셨는지 궁금합니다 ㅎㅎ
4. 2번 방법이 어떤 이유에서 단일책임원칙을 위반했다고 생각하시는 지 궁금하네요 ㅎㅎ
   두 방법만 놓고 봤을 땐 지금 구조가 더 좋아보입니다만 조금 더 좋은 방법이 있을 것 같지 않나요?
   DTO가 왜 이렇게 복잡해졌는지, OutputView에 boolean값을 넘겨주는 게 정말 맞을지 등등
   조금만 더 고민해주시면 더 좋아질 것 같습니다 ㅎㅎ
5. 상수클래스로 관리하는 것보단 지금이 더 낫습니다. 하지만 더 좋은 방법이 있을 것 같지 않나요?
6. 이 부분은 리뷰에 남겼습니다 ㅎㅎ

추가적으로, 테스트 코드가 하나도 없네요…

시간이 오래 걸려도 좋으니 Model의 모든 public 메서드에 대해서 단위 테스트를 작성해주세요

만약 테스트를 어떻게 짜야할지 모르겠다면, 메서드를 어떻게 수정해야 테스트 하기 편할지 고민해주시구요 ㅎㅎ

짧은 시간 내에 성장한 지수님을 보다보니 저도 욕심이 나서 고칠 부분만 계속 언급하게 되네요.

사실 이번 코드보고 짧은 시간 내에 엄청 성장하셔서 많이 놀랐습니다.
지수님이 얼마나 몰입하셨는지 잘 느낄 수 있었어요.

고민의 방향도 정말 좋구요 ㅎㅎ

다른 사람의 PR을 참고하는 것도 좋지만,
스스로와 다른 사람을 비교하면서 조바심 내다보면 지치기 마련이니까요,
지금 잘 성장하고 있는 스스로에게 자신감을 가지고 즐겁게 하셨으면 좋겠습니다.

수고하셨습니다!!

[junpakPark]

Validator라는 유틸리티 클래스가 꼭 필요한가요?

[junpakPark]

유틸리티 클래스는 인스턴스화를 막아주셔야합니다 ㅎㅎ

[junpakPark]

필드에 String 하나 밖에 없는 데 굳이 DTO로 감싸줘야하나요?

[junpakPark]

해당 DTO가 이렇게 커진 이유는 뭘까요?
printResult()가 하는 일이 너무 많아서 그렇진 않을까요?

View와 Model을 유의미한 단위로 묶어서 메서드 추출하라는 의미는 controller 내에 아래와 같이 만들어 주라는 의미였습니다.

    private void takeOrder(final Orders orders) {
        OrdersDto ordersDto = OrdersDto.from(orders);
        outputView.printOrderDetail(ordersDto);
    }

제가 지수님 PR에선 controller가 outputview의 순서에 대해 알 필요 없다라는
리뷰를 남긴 적은 없는데.. 이러한 피드백을 의식해서 코드가 더 복잡해진 것 같습니다.

이 부분에 대해 조금 더 고민해주시면 좋을 것 같네요