레벨 4 미션인 점진적인 리팩토링 미션이다. 취준과 함께 약 한 달 동안 진행했던 미션이다.
3단계로 이루어진 미션이었는데 단계별로 아래와 같은 요구사항이 있었다.
1단계는 리팩토링을 하기 위해 우선 테스트 코드를 작성해서 비즈니스 로직을 보호하는 미션이었다.
2단계는 본격적으로 프로덕션 코드를 리팩토링하고 단위 테스트를 추가하고 수정하는 미션이었다.
3단계는 도메인 연관 관계를 전부 단방향으로 바꾸는 미션이었다.
미션을 진행하면서 받았던 의미 있는 코드리뷰를 정리해보자!
따봉 하나 받았을 뿐이지만 나에겐 의미 있는 코드 리뷰다. 왜냐하면 고민을 많이 했던 부분이기 때문이다.
테스트 코드를 작성할 때 필요한 entity들을 생성하고 repository에 save해주는 로직이 너무 많은 부분을 차지하고 있어서 좀 더 가독성 좋게 테스트 작성하는 방법이 없을까 고민했다.
처음엔 테스트 클래스 내부에서 entity 생성을 메서드 분리해서 처리하다가 여러 테스트 클래스에서 분리한 entity 생성 메서드가 중복되는 것을 보고 아예 fixture 패키지와 entity별 fixture 클래스를 만들어주는 방식으로 처리했다.
아래와 같은 방식에서
@Test
void create() {
BigDecimal price = new BigDecimal(19000);
MenuGroup menuGroup = new MenuGroup(null, "양념반 후라이드반");
MenuGroup savedMenuGroup = menuGroupRepository.save(menuGroup);
Product product = new Product(null, "치킨", new Price(price));
Product savedProduct = productRepository.save(product);
... // 테스트 로직이런식으로 바뀌었다.
import static kitchenpos.fixture.MenuGroupFixture.createMenuGroupWitId;
import static kitchenpos.fixture.ProductFixture.createProductWithPrice;
...
@Test
void create() {
BigDecimal price = new BigDecimal(19000);
MenuGroup savedMenuGroup = menuGroupRepository.save(createMenuGroupWithoutId());
Product savedProduct = productRepository.save(createProductWithPrice(price));
... // 테스트 로직가독성도 좋아지고 entity 인스턴스 변수가 변경됐을 때 Fixture 클래스 메서드에서만 수정을 해주면 되니까 테스트 코드를 유지 보수하기 더 좋아졌다고 생각한다. 별거 아닌 부분이지만 고민했던 부분이 1 따봉을 받으니 뿌듯했다.
id가 null인 객체를 생성하기 위해 all-arguments 생성자에 null을 인자로 넘기는 방식으로 짰다. 코드에 null이 들어간다는 것 자체에 거부감이 있긴 했지만 id가 null인 객체를 생성해준다는 의도를 표현해주지 않을까하는 생각으로 작성했었다.
하지만 리뷰에서 지적하신 것처럼 불필요한 인자를 넘기는 코드를 작성하는 것은 옳지 않은 방법이었고 오히려 코드를 보는 사람으로서는 가독성도 떨어졌다. 보는 사람은 직접 클래스 내부 생성자를 보아야만 id를 null로 넘긴 거구나 하고 알 수 있기 때문이다.
해결 방법으로 static factory 생성자를 만들어주는 방법과 인자로 id를 받지 않는 생성자를 만들어주는 방법 두 가지를 고민했는데 생성자를 만들어주는 방법으로 해결했다.
static factory 생성자로 해결하지 않은 이유는 기존 생성자를 private로 바꾸지도 못하는 상황에 객체 생성을 캡슐화하는 경우도 아니었고 리뷰를 받은 로직 외에는 사용할 곳이 없었기 때문이다.
결론적으로 static factory 생성자를 만들었을 경우 장점이 이름을 가진다는 장점밖에 없었다. 그래서 id를 인자로 받지 않는 일반 생성자를 추가하는 방향으로 해결했고 로직의 흐름상 일반 생성자로도 의도 표현이 충분하다고 생각했다.
그런데 지금 다시 생각해보니 이름을 가진다는 장점 하나만으로도 static factory 생성자를 사용할 이유는 충분하지 않았나.. 라는 생각도 든다.
확실한 건 다음부터 무심코 null을 인자로 넘기는 코드를 작성하다가도 아차! 하고 상황에 맞는 더 좋은 방법을 고민할 것이다.
위 리뷰를 받았을 때 Price라는 VO 객체는 이미 만들고 사용하고 있는 상태였다. 내가 고민했던 것은 BigDecimal을 사용해서 가격을 나타내고 있는 다른 코드나 로직에도 Price로 전부 감싸주느냐 아니면 Price가 필요한 부분에만 감싸주느냐였다. 글로 표현하기가 조금 어려운데 리뷰를 받았던 아래 코드를 보자.
private void validateTotalAmount(Menu menu, List<MenuProduct> menuProducts) {
BigDecimal sum = BigDecimal.ZERO;
for (MenuProduct menuProduct : menuProducts) {
sum = sum.add(menuProduct.calculateAmount());
}
if (menu.isSmallerPrice(sum)) {
throw new IllegalArgumentException("MenuProduct 전부를 합한 금액이 Menu 금액보다 작을 수 없습니다.");
}
}BigDecimal 객체의 add 메서드를 사용해서 금액을 더해주고 검증하는 코드였다. 처음엔 굳이 Price 객체로 감싸줄 필요성을 못 느꼈다. Price 객체 자체로 검증을 할 수 있는 로직도 아니었고 BigDecimal 객체에 메시지를 보내는 형식으로 되어있어서 객체 지향적으로도 문제가 없다고 생각했다.
리뷰를 받고 나서는 Price VO로 금액을 감싼 로직과 안 감싼 로직이 혼용되어있으면 더 도메인을 명확하게 파악하게 만들기 힘들겠다는 생각이 들었다. 리뷰어님의 말대로 VO를 아예 일괄적으로 사용해서 변경에 대응을 쉽게 하고 도메인을 명확하게 표현해서 가독성을 향상시키는 것이 훨씬 옳은 방향이라는 것을 깨달았다.
아래와 같이 수정하고 검증 로직도 Service 레이어에 있던 것을 MenuProducts라는 일급컬렉션을 만들고 옮겨주었다.
private static void validateTotalAmount(Menu menu, List<MenuProduct> menuProducts) {
Price result = Price.ZERO;
for (MenuProduct menuProduct : menuProducts) {
result = result.sum(menuProduct.calculateAmount());
}
if (menu.isSmallerPrice(result)) {
throw new IllegalArgumentException("MenuProduct 전부를 합한 금액이 Menu 금액보다 작을 수 없습니다.");
}
}
주문을 시작하기 위해선 주문 상태가 OrderStatus.COOKING이라는 ENUM 객체로 되어있어야했는데 아무 생각 없이 처음 Request DTO에서 Order entity로 바꿔줄 때 때려박아넣었었다.
주문을 시작하기 위해 주문 상태가 COOKING으로 되어있어야 하는 요구 사항이고 검증해야 하고 명확히 표현해줘야 하는 부분인데 너무 생각 없이 짰던 것 같다.
요구 사항이나 표현해줘야 하는 부분은 더 세심히 신경 쓰고 명확히 표현해줘야 한다는 걸 잊지 말자.
점진적인 리팩터링 미션은 아쉽게 취직 준비와 시기가 겹쳐서 100퍼센트 집중하지는 못한 미션이라는 게 아쉽다. 물론 핑계일 수 있다..
그래도 오랜만에 정말 재밌게 했던 미션이다. 신경 쓰지 못한 부분이 많아서 코드 리뷰해주시면서 화나셨을 텐데 꼼꼼하고 친절하게 리뷰해주신 리뷰어님께 정말 감사함을 느꼈다.
많이 고민했고 재밌어서 기억에 남는 미션이 될 것 같다.