All Articles

실무에서 간단 리팩토링해보기

회사 다른 업무에 투입되게 되었는데, 아직 마크업 단계이다 보니 문제가 될만한 코드들이 많이 보인다.

가장 문제가 될만한게 배송비 계산하는 부분이다. 내친김에 조그만 헬퍼 함수를 리팩토링해보기로 했다.

원본코드부터 보자

const calculateDeliveryFee: CalculateDeliveryFeeType = (product) => {
  const {
    deliveryPricingMethod,
    deliveryBaseFee = 0,
    unitChargeStandard = 0,
    conditionalChargeStandard = 0,
  } = product.deliveryGroupInfo

  let price = 0
  let count = 0
  product.skus.forEach((sku) => {
    price = price + sku.sellPrice * sku.quantity
    count = count + sku.quantity
  })

  switch (deliveryPricingMethod) {
    case 'conditional_charge':
      const conditionalCharge =
        price >= conditionalChargeStandard ? 0 : deliveryBaseFee
      return conditionalCharge
    case 'free':
      return 0
    case 'regular_charge':
      return deliveryBaseFee
    case 'unit_charge':
      const unitCharge = Math.ceil(count / unitChargeStandard) * deliveryBaseFee
      return unitCharge
  }
}
export default calculateDeliveryFee

대충 어떤 내용인지는 알 수 있다. product로 Object가 들어올 경우에 price, count에 맞추어, 배송비 타입에 맞추어 배송비를 계산해서 던져주는 함수이다.

문제가 될만한 소지들을 체크해보자.

1. 가변 데이터인 price, count

가변 데이터는 소프트웨어에 문제를 일으키는 가장 큰 골칫거리이다. 가변데이터는 서로 다른 두 코드를 이상한 방식으로 결합하기도 하고, 연쇄 효과를 일으켜서 생각지도 못한 곳에 문제를 일으키곤 한다. 유효범위를 좁히는게 좋다.

물론 여기서는 유효범위가 calcuateDeliveryFee이지만, 할 수만 있다면 파생 변수를 질의 함수로 바꾸는 것(9.3장)이 좋아보인다.

2. 반복문 안에서 두 가지 동작

 product.skus.forEach((sku) => {
    price = price + sku.sellPrice * sku.quantity
    count = count + sku.quantity
  })

두 가지 동작을 forEach에서 하고 있는데, 각각 쪼개게 되면 다른 동작을 신경쓸 필요가 없어보인다.

3. 변경될 부분이 많아 보이는 조건부 로직

배송비는 바뀔 가능성이 높은 부분이라는 생각이 든다. 계산하는 로직들이 회사 사정에 따라 많이 변경될 수 있고 이렇게 되면 더 복잡한 계산식들이 각 switch문에 포함되게 될것이다. 그러면 매우 뚱뚱한 switch문이 될 가능성이 크다.

따라서 나는 다형성으로 바꾸는 방법이 좋아보인다고 생각했다. (10.4장)

배송비 계산하기 리팩토링

호출하는 부분들을 전부 바꾸어야 한다. 우선 클래스를 선언하고 한 곳에 묶는게 좋다고 생각하였다.

클래스를 선언하고 호출하는 부분을 변경해준다.

다행히도 호출하는 부분은 한 군데 밖에 없어서 바꿀때 side effect를 많이 고려할 필요는 없었다.

calculate를 아예 Delivery에 넣어버리고 일단 호출은 아래와 같이 해준다.

// 새로 만든 Delivery 클래스 
export class Delivery {
  private _product: CartProduct
  constructor(product: CartProduct) {
    this._product = product
  }
  get fee() {
    return this.calculateDeliveryFee(this._product)
  }

  private calculateDeliveryFee: CalculateDeliveryFeeType = (product) => {
    const {
      deliveryPricingMethod,
      baseDeliveryFee = 0,
      unitChargeStandard = 0,
      conditionalChargeStandard = 0,
    } = product.deliveryInfo
    let price = 0
    let count = 0
    product.productSellableSkus.forEach((sku) => {
      price = price + sku.sellPrice * sku.quantity
      count = count + sku.quantity
    })
    switch (deliveryPricingMethod) {
      case 'conditional_charge':
        const conditionalCharge =
          price >= conditionalChargeStandard ? 0 : baseDeliveryFee
        return conditionalCharge
      case 'free':
        return 0
      case 'regular_charge':
        return baseDeliveryFee
      case 'unit_charge':
        const unitCharge =
          Math.ceil(count / unitChargeStandard) * baseDeliveryFee
        return unitCharge
    }
  }
}
// 호출하는 부분
<div className="total-delivery">
  총 배송비 {new Delivery(product).fee}원
</div>

반복문 쪼개기

배송비 계산은 기존에 보면 위에서부터 순차적으로 나타나게 되는데, 하나만 잘못 변경해도 모든 로직이 잘못될 가능성이 있다.

let price = 0
let count = 0
product.productSellableSkus.forEach((sku) => {
  price = price + sku.sellPrice * sku.quantity
  count = count + sku.quantity
})

여기부터 다 캡슐화할 필요가 있다.

product, price를 계산하고 들어온 Product를 deliveryPricingMethod에 따라 돌려주는 방식이다.

price와 count를 각각 나누어준다. 그리고 안에서 쓰이는 것들을 하나씩 바꾸어준다.

private get productPrice() {
  let price = 0
  this._product.productSellableSkus.forEach((sku) => {
    price += sku.quantity * sku.sellPrice
  })
  return price
}

private get productCount() {
  let count = 0
  this._product.productSellableSkus.forEach((sku) => {
    count += sku.quantity
  })
  return count
}

private calculateDeliveryFee: CalculateDeliveryFeeType = (product) => {
    const {
      deliveryPricingMethod,
      baseDeliveryFee = 0,
      unitChargeStandard = 0,
      conditionalChargeStandard = 0,
    } = product.deliveryInfo
    let price = 0
    let count = 0
    product.productSellableSkus.forEach((sku) => {
      price = price + sku.sellPrice * sku.quantity
      count = count + sku.quantity
    })

    switch (deliveryPricingMethod) {
      case 'conditional_charge':
        const conditionalCharge =
          this.productPrice >= conditionalChargeStandard ? 0 : baseDeliveryFee  // 내부 함수들로 인라인한다.
        return conditionalCharge
      case 'free':
        return 0
      case 'regular_charge':
        return baseDeliveryFee
      case 'unit_charge':
        const unitCharge =
          Math.ceil(this.productCount / unitChargeStandard) * baseDeliveryFee // 내부 함수들로 인라인한다.
        return unitCharge
    }
  }

이러면 이 로직은 필요 없으므로 통째로 제거한다.

 let price = 0
 let count = 0

product.productSellableSkus.forEach((sku) => {
  price = price + sku.sellPrice * sku.quantity
  count = count + sku.quantity
}

Switch문 다형성으로 변경

그리고 이제 fee를 보내줄때 배송비결제 타입에 따라 달라진다. 여기는 Swith 문으로 되어 있는데 각각 서브클래스로 나누어서 가져오면 좋을것 같다.

그런데 가만히 보니 중요한 것은 Charge를 받아오는 것이고, price와 count역시 charge에서만 사용되는 것을 발견했다. 이 말은 Delivery클래스는 그대로 두고 (후에 배송비 관련 추가 로직들이 붙을 수 있으니),Charge 클래스로 바꾸고 여기서 가져오면 되겠다라고 생각했다.

Charge는 여기서 보면 4개다.

  • 무료인 경우
  • 기본 요금일 경우
  • 1개만 부과하는 경우
  • 여러개로 부과하는 경우

따라서 기본 Charge에서 price, count를 계산한 후 나머지는 extends를 하고 fee를 오버라이드하여 보내주면 된다. 

class Charge {
  _product: CartProduct
  constructor(product: CartProduct) {
    this._product = product
  }
  get fee() {
    const { baseDeliveryFee } = this.deliveryInfo
    return baseDeliveryFee
  }
  get deliveryInfo() {
    return this._product.deliveryInfo
  }
  get baseDeliveryFee() {
    const { deliveryInfo } = this._product
    return deliveryInfo.baseDeliveryFee
  }
  get unitChargeStandard() {
    return this.deliveryInfo.unitChargeStandard || 0
  }
  get conditionalChargeStandard() {
    return this.deliveryInfo.conditionalChargeStandard || 0
  }
  get productPrice() {
    let price = 0
    this._product.productSellableSkus.forEach((sku) => {
      price += sku.quantity * sku.sellPrice
    })
    return price
  }
  get productCount() {
    let count = 0
    this._product.productSellableSkus.forEach((sku) => {
      count += sku.quantity
    })
    return count
  }
}
class FreeCharge extends Charge {
  get fee() {
    return 0
  }
}
class ConditionalCharge extends Charge {
  get fee() {
    const conditionalCharge =
      super.productPrice >= super.conditionalChargeStandard
        ? 0
        : super.baseDeliveryFee
    return conditionalCharge
  }
}

class UnitCharge extends Charge {
  get fee() {
    const unitCharge =
      Math.ceil(super.productCount / super.unitChargeStandard) *
      super.baseDeliveryFee
    return unitCharge
  }
}

이렇게 작성하고 나니 일단은 깔끔해보인다. Delivery는 아래와 같이 변경되었다. 

export class Delivery {
  private _product: CartProduct
  constructor(product: CartProduct) {
    this._product = product
  }
  calculateFeeByChargeType(method) {
    switch (method) {
      case 'free':
        return new FreeCharge(this._product)
      case 'regular_charge':
        return new Charge(this._product)
      case 'conditional_charge':
        return new ConditionalCharge(this._product)
      case 'unit_charge':
        return new UnitCharge(this._product)
      default:
        return new Charge(this._product)
    }
  }
  get calculateFee() {
    const { deliveryPricingMethod } = this._product.deliveryInfo
    return this.calculateFeeByChargeType(deliveryPricingMethod).fee
  }
}

마무리


확장성이 잘 될까?

추후에 배송관련해서 주문한 사람 이름이라던가, 추적 정보등이 포함될 수 있다. 지금은 Charge만 반환하지만, Delivery 클래스에 추가될 정보들을 get() 메소드로 추가해주면 깔끔해질것 같다. 물론 이렇게 쓰여질지는 의문이긴하다.

수정이 쉬울까?

수정이 쉬웠으면 하는 바람에서 만들었지만, 그래도 각 Charge 클래스에서 복잡한 계산을 하고 있는 건 마찬가지이다. 그래도 기존보단 고려해야할 변수들이 많이 적어졌다.

배송비 요금 사용되는 곳에서의 사용

추후에 배송비 요금은 독립적으로 쓰이는게 아니라 총 상품의 가격을 결정하는데 사용될 것이다. 그때마다 product 레코드를 받아서 계산된 요금만 받아오는 방식으로 사용하면 될 것 같다.

Published 31 Mar 2022

Web Developer 웹개발자 김요한입니다.Yohan Kim