4주차 미션에서 피드백을 받은 설계 문제에 대해 정리해보려고 한다.
[4주차 미션 코드]
https://github.com/ldhapple/java-christmas-6-ldhapple/pull/1
컨트롤러가 컨트롤러를 가지고 있는 문제
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
public class WootecoRestaurantController {
private final RestaurantCalculator restaurantCalculator;
private final EventPlanerController eventPlanerController;
public WootecoRestaurantController(RestaurantCalculator restaurantCalculator,
EventPlanerController eventPlanerController) {
this.restaurantCalculator = restaurantCalculator;
this.eventPlanerController = eventPlanerController;
}
public void takeReservation() {
VisitDate visitDate = getVisitDate();
Orders orders = getOrders();
int orderTotalAmount = getOrderTotalAmount(orders);
showEventPreviewMessage(visitDate);
showOrderDetails(orders, orderTotalAmount);
executeEvent(visitDate, orders, orderTotalAmount);
}
/...
private void executeEvent(VisitDate visitDate, Orders orders, int orderTotalAmount) {
if (canApplyEvent(orderTotalAmount)) {
eventPlanerController.executeEventPlaner(visitDate, orders, orderTotalAmount);
}
}
}
나는 이렇게 WootecoRestaurant 컨트롤러에서 EventPlaner 컨트롤러를 이용해 동작하도록 작성했었다.
레스토랑 컨트롤러에서는 예약에 관한 동작의 흐름을 제어하고 있는 것이었고, 이벤트 컨트롤러에서는 이벤트에 대한 동작의 흐름을 제어하고 있었다.
하지만 기본적으로 MVC 패턴에서 컨트롤러가 컨트롤러를 가지고 있는 것은 자연스럽지 못하다.
그래서 이벤트 컨트롤러가 컨트롤러가 아닌 서비스의 이름을 가져야 했는가? 라는 생각이 들었다.
MVC 아키텍처에서 컨트롤러의 역할
사용자 입력을 처리하고, 서비스 또는 비즈니스(도메인 로직 등) 계층에 위임하며 뷰를 업데이트 한다.
개선 방안
우선 위 코드가 컨트롤러에 있어야 되는가를 생각해보아야 한다.
결론은 ‘아니다’ 이다.
RestaurantController는 사용자의 입력을 처리하고, 예약에 관련된 프로세스를 진행함에 있어 컨트롤러에 어울리는 역할을 가진다.
하지만 EventPlanerController의 코드를 보면 입력된 파라미터를 기반으로 작업을 수행한다. 사용자 입력과 직접 상호작용하지 않고 이벤트 기획 및 혜택 계산과 관련된 로직을 입력 파라미터 기반으로 작업하는 것이다.
따라서 이 부분은 서비스라고 지칭하는 것이 더 나은 방향이다.
(물론 미션에서는 컨트롤러와 서비스의 차이가 네이밍과 패키지 차이이지만 MVC 패턴을 적용함에 있어 컨트롤러와 서비스의 역할에 대해 생각해보아야 한다는 것이다.)
비즈니스 로직이 컨트롤러에 있어도 되는가?
1
2
3
4
5
6
7
8
9
10
11
12
13
14
private void executeEvent(VisitDate visitDate, Orders orders, int orderTotalAmount) {
if (canApplyEvent(orderTotalAmount)) {
eventPlanerController.executeEventPlaner(visitDate, orders, orderTotalAmount);
}
}
private boolean canApplyEvent(int orderTotalAmount) {
if (isOrderTotalAmountValidForEvent(orderTotalAmount)) {
return true;
}
eventPlanerController.notApplyEvent(orderTotalAmount);
return false;
}
이벤트를 적용할지 안할지에 대한 판단을 restaurant 컨트롤러에서 진행하고 있다. 이 부분은 비즈니스 로직이다.
이 부분은 사실 문제가 되지 않을 수도, 될 수도 있다. 기본적으로 설계의 측면에서 어떠한 기조를 가지고 있었는지가 중요하다.
개선 방법
컨트롤러는 사용자 입력 처리, 도메인 모델의 업데이트, 애플리케이션의 흐름제어를 담당한다.
DDD(Domain-Driven Design)나 Clean Architecture과 같은 보다 근래의 아키텍처 구조에서는 문제에 대해 명시적으로 분리하는 성향이 있다.
이러한 원칙을 따른다면 위의 부분은 개선이 필요하다.
- 이벤트 적용과 같은 핵심 비즈니스 로직은 도메인에 배치할 수 있다.
- Event 도메인을 만들어 값을 던져주면 Event 도메인에서 Event의 적용 여부를 판단하는 것이다.
- 혹은 서비스에 비즈니스 로직을 위임할 수 있다.
이러한 분리를 통해 애플리케이션의 핵심 동작과 사용자 입력 처리 등과 비즈니스 로직을 따로 관리할 수 있게 되어 유지 보수에 장점이 생길 수 있다.
Orders 클래스 중복 코드 문제
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
public class Orders {
private final Map<Food, Integer> orders;
/...
public int getMainMenuCount() {
int mainMenuCount = 0;
for (Entry<Food, Integer> entry : orders.entrySet()) {
Food menu = entry.getKey();
int menuCount = entry.getValue();
if (menu instanceof MainFood) {
mainMenuCount += menuCount;
}
}
return mainMenuCount;
}
public int getDessertMenuCount() {
int dessertMenuCount = 0;
for (Entry<Food, Integer> entry : orders.entrySet()) {
Food menu = entry.getKey();
int menuCount = entry.getValue();
if (menu instanceof Dessert) {
dessertMenuCount += menuCount;
}
}
return dessertMenuCount;
}
/...
}
위 부분은 구조가 같다. 주문을 저장하고 있는 Map 자료구조에서 원하는 데이터를 추출하는 작업이다.
이렇게 중복되는 부분은 제거해야 한다.
개선 방안
1
2
3
4
5
6
7
8
9
10
11
12
13
14
public int getMenuCount(Class<? extends Food> menuType) {
int count = 0;
for (Entry<Food, Integer> entry : orders.entrySet()) {
Food menu = entry.getKey();
int menuCount = entry.getValue();
if (menu.equals(menuType)) {
count += menuCount;
}
}
return count;
}
메뉴 타입을 매개변수로 받아 해당 메뉴 타입의 주문 개수를 반환하면 되는 문제였다.
쉽게 생각할 수 있는 부분이었지만 고려하고 있던 부분이 많아 놓친 부분이다.
쉬운 실수를 하지 말자는 의미에서 포스팅에 추가했다.
Orders를 더 개선할 수 없을까?
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
public class Orders {
private final Map<Food, Integer> orders;
private Orders(String orders) {
validate(orders);
this.orders = registerOrders(orders);
}
public static Orders create(String orders) {
return new Orders(orders);
}
/...
private void validate(String orders) {
OrderValidator.validateOrders(orders);
}
private Map<Food, Integer> registerOrders(String orders) {
List<String> menus = Parser.parseMenus(orders);
List<Integer> counts = Parser.parseMenuCounts(orders);
Map<Food, Integer> order = new HashMap<>();
for (int idx = 0; idx < menus.size(); idx++) {
String menuName = menus.get(idx);
Food food = Menu.getFoodByName(menuName);
int count = counts.get(idx);
order.put(food, count);
}
return order;
}
public Map<Food, Integer> getOrders() {
return Collections.unmodifiableMap(orders);
}
}
Orders에서는 String을 Map으로 바꾸어 등록하는 로직, 그 변환된 Map을 가지고 여러 작업을 하는 구조를 가지고 있다.
이러한 부분은 개선사항이 있을 수 있다.
Map이라는 자료구조는 보다시피 전체를 순회하기 위해 여러 줄의 코드가 필요하게 된다. 어려운 코드는 아니지만 조금 번거롭다는 느낌이 든다.
그리고 가장 큰 문제는 Orders에 그렇게 중요한 로직이 없다. Orders에 가지고 있는 자료를 가지고 데이터를 보낼 수 있는데, 그러한 부분이 전체 코드를 보면 Service 같은 부분에 위임되어 있다.
이를 어떻게 개선할 수 있을까?
개선 방안
Orders를 Order 객체를 담는 일종의 일급 컬렉션으로 포장하는 것이다.
1
2
3
4
5
public class Orders {
private final List<Order> orders;
/...
}
1
2
3
4
5
6
public class Order {
private final Food menu;
private final int count;
/...
}
Order 객체에는 메뉴와 해당 메뉴에 대한 주문 수량을 담고 있다. 이 Order 객체를 만들면 아래와 같이 활용할 수 있다.
- Order 객체에서 menu, count를 가지고 해당 주문 메뉴의 총 금액을 구할 수 있다.
- Orders에서는 가지고 Order들에서 구한 총 금액을 가지고 주문 총 금액을 반환해줄 수 있다.
단순하게는 사용하기 번거로웠던 Map 구조를 Order 객체를 만듦으로써 개선할 수 있는 것이다.
물론 주문을 입력받는 String을 Order에 맞게 변환하는 과정은 보다 번거로울 수는 있다. 하지만 객체를 보다 객체답게 사용하고 getter의 사용을 지양할 수 있는 방향으로 설계가 가능하다는 장점이 있다.
결론
MVC 패턴을 사용하고 있었지만, 각각의 책임에 대해 정확히 모르고 사용했던 것 같다. 그리고 피드백을 통해 객체의 책임에 관해 더 경험하고 알아가는 기회가 되었다.
위에 나열한 부분 외에 4주차 미션에서 아쉬웠던 부분은 DiscountPolicy enum 클래스같은 경우 date에 의한 할인 적용 여부, 할인가격 계산식을 모두 관리했었는데, 이러한 조건식이나 할인계산식도 enum이나 객체로 만들어 사용할 수 있었다는 점이다.
어떠한 식들이 해당 부분에 공통적이어도, 각 식에 필요한 input값 같은 것이 다르다면 고려해볼 내용인 것 같다. input값 등이 다르지 않더라도 한 곳에서 식을 관리한다는 의미로도 사용할만한 가치가 있을 것이다.