Yuki's Tech Blog

仕事で得た知見や勉強した技術を書きます。

トップ > コードレビュー > 【2023/1/29 ~ 2023/6/30】コードレビューで知ったことをざっくりまとめてみた part3

【2023/1/29 ~ 2023/6/30】コードレビューで知ったことをざっくりまとめてみた part3

コードレビュー

目次

作った機能

  • 税込価格を表示
  • 欠品ステータスを商品ごとに表示
  • キャンセルした時にクーポンを復活させる

Rails

「価格をフォーマットするだけ」のヘルパーが「税込」「税抜」を意識するのは違う

フォーマットするという一つの目的だけを果たすために作られているので、このヘルパー内で税込、税抜処理を書くのは違う。

before

  def format_price(price, is_tax_included = false)
    if is_tax_included
      "税込 #{price.to_fs(:delimited)}円"
    else
      "#{price.to_fs(:delimited)}円"
    end
  end

<strong><%= format_price(checkout_item.price) %>(<%= format_price(checkout_item.calc_tax_included_price(), true) %>)&nbsp;</strong>

after

  def format_price(price)
    "#{price.to_fs(:delimited)}円"
  end

<strong><%= format_price(checkout_item.price) %>(税込 <%= format_price(checkout_item.calc_tax_included_price()) %>)&nbsp;</strong>

消費税の計算は TasService.tax_amountを使う

before

  def calc_tax_included_price
    (price * (1 + (tax_percentage / 100.to_f))).to_i
  end

after

  # 単純に税込価格を返すだけのメソッドになったので、calcのprefixはいらん
  def tax_included_price
    price + TaxService.tax_amount(price, tax_percentage)
  end

ruby では引数のない関数呼び出しは () を省略する慣習がある

既にトランザクション内にいるのに、再度トランザクションを貼る必要はない

ビジネス特有のロジックはモデルに集約させる。あちこち散らばってたら、変更が大変なので

決済作成された場合、与信が確定したわけではない

以下の流れで与信が確定する。stripe一から実装してえな。結局stripeよく分からんかった。

  1. checkoutの作成
  2. paymentの作成（checkoutと同時に作成される）
  3. ここでAPIレスポンスされる
  4. frontendでカードの与信審査が投げられる
  5. 成功した場合はcheckoutが確定に回る
  6. 失敗した場合checkoutがfailedステータスに移行する

nil.updateと呼び出す例外が発生するので、nilをレシーバとして例外を発生させたくないなら、ぼっち演算子を使えば良い

ビジネスのルールに関する条件分岐の場合、モデルにメソッドとして定義した方が良い

これも当たり前か。いろんなところにビジネスのルールに関する条件分岐が書かれていたら変更が辛いし、管理が辛い。あと、以下のコードの場合、completed or partially_completed みたいな条件でfullfill情報が返されている処理を見ても、それがどういう理由でそんな処理になっているのかコンテキストの全くない人が見ても全く理解できない。そのため、抽象化してどんな目的なのかを命名から表して意味付けしているとも言える。あと、public?だけだと、一体何がpublicのかが分からないので、意味の分かるメソッド名にしている。

以下はシリアライザーのコード

before

has_many :fullfillment_shippings, if: -> { object.shipping_completed? || object.shipping_completed_with_partially? }

after

has_many :fullfillment_shippings, if: -> { object.fullfillment_shippings_public? }

  def fullfillment_shippings_public?
    shipping_completed? || shipping_completed_with_partially?
  end

Rspec

フロントエンド

デフォルトで消費税10%を持たない

モデルの消費税のメンバ変数にデフォルトで具体的な値を持つと、8%の商品に対して10%で計算されてしまって表示されてしまうリスクが高いので、デフォルトはundefinedとしておき、税込価格表示をする場所は、常にtaxPercentageの有無を見てハンドリングしてあげた方が良い。なので、価格表示だけをするコンポーネントを一つ用意した方が良い。

でも確か、型の問題でうまくいかなくて、結局taxPercentageの初期値に-1を採用した気がする。 before

taxPercentage = 10;

after

 taxPercentage?: number  = undefined;

taxIncludedPrice だけだとこいつが具体的に何をする関数なのかが分かりづらいため、 calcTaxIncludedPriceのような動詞をつけた命名にする

あと、価格に関する処理は product モデルのヘルパーに限定した処理ではないので、共通の部分で定義する

before

export function taxIncludedPrice(price: number, taxPercentage: number) {
  return Math.ceil((price * (1 + (taxPercentage / 100))));
}

after

export function calcTaxIncludedPrice(price: number, taxPercentage: number) {
  return Math.ceil((price * (1 + (taxPercentage / 100))));
}

  get taxIncludedPrice() {
    return calcTaxIncludedPrice(this.price, this.taxPercentage);
  }

taxPercentageがないときに全部を見せないのはちょっとやりすぎ感がある。この時は「税抜価格だけ表示する」が正しい。

before

export const ProductPrice: BFC<Props> = ({
  price,
  taxPercentage,
  className
}) => {
  if (!taxPercentage) {
    return null;
  }

  if (!price) {
    return null;
  }

  return (
    <div className={classNames(className)}>
      {formatPrice(price)}(税込 {formatPrice(calcTaxIncludedPrice(price, taxPercentage))})
    </div>
  );
};

after

export const ProductPrice: BFC<Props> = ({
  price,
  taxPercentage,
  className
}) => {
  if (!price) {
    return null;
  }

  if (!taxPercentage) {
    return `${formatPrice(price)}(税抜)`;
  }

  return (
    <div className={classNames(className)}>
      {formatPrice(price)}(税込 {formatPrice(calcTaxIncludedPrice(price, taxPercentage))})
    </div>
  );
};

消費税の計算は「切り捨て」で行うことになっているので、切り捨て処理をする

before

export function calcTaxIncludedPrice(price: number, taxPercentage: number) {
  return Math.ceil((price * (1 + (taxPercentage / 100))));
}

after

export function calcTaxIncludedPrice(price: number, taxPercentage: number) {
  return Math.floor((price * (1 + (taxPercentage / 100))));
}

同じようなメソッドを定義しない

いじらないほうがいいなで、似たようなメソッドを量産すると、どれを使えば良いかわからなくなるし、ちょっと変更したらバグが起こるようなコードになってしまう。このような場合、既存のメソッドを修正しつつもうまく動くように既存の仕組みをぶっ壊して根本を変える必要がある。そうしないと無限にコードが増えてしまう。

個別にステートを定義する意味が薄いなら、まとめてステートを管理できるモデルを使ったほうが良い

before

  const [totalPrice, setTotalPrice] = useState(0);
  const [subtotalPrice, setSubtotalPrice] = useState(0);
  const [discountPrice, setDiscountPrice] = useState(0);
  const [totalShippingRate, setTotalShippingRate] = useState(0);
  const [serviceCommission, setServiceCommission] = useState(0);
  const [smallCheckoutCommission, setSmallCheckoutCommission] = useState(0);
  const [totalTax, setTotalTax] = useState(0);

after

const [checkout, setCheckout] = useState<Checkout>(new Checkout);

タイトルの表示など、ユースケースにかなり依存しそうな処理は再利用化するメリットがあまりないので、オプションで出しわけするのではなくて、そのコンポーネントに含めないで切り分けたほうが良い。用はタイトルは使う側でベタ書きするってこと

もちろんタイトルの有無をpropsでやっても良いケースもある。あくまでも今回のケースがそうって話なだけ。

          <CheckoutPaymentDetail
            checkout={checkout}
            showTitle={false}

          <CheckoutPaymentDetail
            checkout={checkout}

注意喚起の文を表示するかしないかをpropで制御する場合、suffixにnoticeをつける

descriptionよりnoticeの方が分かりやすい。 あと、この時、既存のデザインを大きく崩さないように表示してって言われたな

before

          <CheckoutPaymentDetail
            checkout={checkout}
            showTitle={false}
            showTotalPrice={false}
            showShippingRateDescription
            className="mt-1 pt-3 pb-2 border-t"
          />

after

            <CheckoutPaymentDetail
              checkout={checkout}
              showTotalPrice={false}
              showShippingRateNotice
            />

カート詳細ページで、上に価格に関する情報がたくさん積まれて、頭の重たいレイアウトになってしまったので、商品群が上、価格表示が下のレイアウトにする

動線的には、カートに入っている商品を全て見てから、価格を見に行くため。

TSではnew Checkout() の ()は省略しない

これはプロジェクトによるかも。

カートアイテムがない場合は支払い詳細全体が非表示になるが望ましい

subtotalが0で条件分岐をしちゃうと、ワンチャン無料商品が混ざった時に意図通りの表示にならなそう。 コードって文脈がすごく大切なので、意味の通る記述を心がける。なんでこう書いているんだろうと他の人が思うハイコンテキストなコードはなるべく書かない。文脈にあったコードを書く。 （文脈を無視する部分、つまり読んでもよく分からない部分は常にコメントが必要

before(なんでsubtotalPriceが条件文に使われているの？が書いた人に聞かないと分からないハイコンテキストなコード)

{checkout.subtotalPrice > 0 ? formatPrice(checkout.totalPrice) : formatPrice(checkout.subtotalPrice)}

after

          {cart.quantity > 0 ? (
            <>

カートが空の時はカート内商品のapiを無駄に呼び出さない

APIを呼び出すのが無駄なので呼び出さない対応をする。

enabled: parcel.items.length > 0,

価格や税率は、 productベースで表示すると購入時から変わっている可能性があるので、checkout_itemベースの数字で出す

要は、チェックアウト時の情報を出さないと、混乱を招くってこと。

直接newでインスタンスを作らず、メソッドを経由してインスタンスを取得する

こうすることで、コンポーネントとクラスが直接依存するのを防いでくれる。クラスに変更が加わって必須の引数が2個になったとしても、メソッド内の処理を1つ直せば良くなる。また、メソッド内に独自の処理を追加したりできるので、自由度が高い。

before

<ShopifyProductProvider product={new Product(item.product)}>

after

<ShopifyProductProvider product={item.getProduct()}>

中のgapをpropで外から変えようとしている場合、そもそもの構造がおかしい可能性がある

中のgapをpropで外から変えるということは必要ない。コンポーネント利用者は、コンポーネントの内部構造を知りすぎている。つまり、うまく抽象化されていない使いづらいコンポーネントになっている可能性がある。余白が必要なら、そもそもの余白を増やした方が良い。余白は取るべきところはちゃんと取らないと見づらい画面になってしまう。

コードを書く場合、既存のものからうまくやろうとするんじゃなくて、「こうあった方が良い」という自分の考えのもと、既存のものをぶっ壊しながらあるべき形に近づけた方が良い

じゃないと、壊れかけのジェンガみたいなプロダクトになる

関数やメソッドの戻り値の型は統一していた方が使いやすい

当たり前だけど、利用する側で条件文を書くのが辛いので、基本的には戻り値の型は統一されていた方が、使いやすい関数やメソッドになる

before(戻り値が共用体型の値になっている)

  getFixedItems(checkout: Checkout) {
    if (this.isShippingMailCompleted()) {
      if (checkout.fullfillmentShippings) {
        return checkout.fullfillmentShippings.reduce((acc, { items }) => {
          const fixedItems = items.map((item) => new FullfillmentShippingItem(item));
          return [...acc, ...fixedItems];
        }, [] as FullfillmentShippingItem[]);
      } else {
        return [] as FullfillmentShippingItem[];
      }
    } else {
      return this.getItems();
    }
  }

after

  getFixedItems(checkout: Checkout) {
    if (this.isShippingMailCompleted()) {
      if (checkout.fullfillmentShippings) {
        return checkout.fullfillmentShippings.reduce((acc, { items }) => {
          const fixedItems = items.map((item) => new CheckoutItem(item));
          return [...acc, ...fixedItems];
        }, [] as CheckoutItem[]);
      } else {
        return [] as CheckoutItem[];
      }
    } else {
      return this.getItems();
    }
  }
}

asを使わなくて良いなら、なるべく使わない

以下の場合、配列なので、戻り値の型を明示すれば解決できる

before

  getFixedItems(checkout: Checkout) {
    if (this.isShippingMailCompleted()) {
      if (checkout.fullfillmentShippings) {
        return checkout.fullfillmentShippings.reduce((acc, { items }) => {
          const productIds = acc.reduce((a, { product }) => [...a, product.id], [] as string[]);
          const fixedItems: CheckoutItem[] = [];
          items.forEach((item) => {
            if (productIds.includes(item.product.id)) {
              const duplicatedItem = acc.find(({ product: { id } }) => id === item.product.id) as CheckoutItem;
              duplicatedItem.quantity += item.quantity;
              if (typeof duplicatedItem.cancelQuantity === "number") duplicatedItem.cancelQuantity += item.cancelQuantity;
              fixedItems.push(duplicatedItem);
              const index = acc.findIndex((a) => a.id === duplicatedItem.id);
              acc.splice(index, 1);
            } else {
              fixedItems.push(new CheckoutItem(item));
            }
          });
          return [...acc, ...fixedItems];
        }, [] as CheckoutItem[]);
      } else {
        return [] as CheckoutItem[];
      }
    } else {
      return this.getItems();
    }
  }
}

after

  getFixedItems(checkout: Checkout): CheckoutItem[] {
    if (this.isShippingMailCompleted()) {
      if (checkout.fullfillmentShippings) {
        return checkout.fullfillmentShippings.reduce((acc, { items }) => {
          const productIds = acc.reduce((a, { product }) => [...a, product.id], [] as string[]);
          const fixedItems: CheckoutItem[] = [];
          items.forEach((item) => {
            if (productIds.includes(item.product.id)) {
              const duplicatedItem = acc.find(({ product: { id } }) => id === item.product.id) as CheckoutItem;
              duplicatedItem.quantity += item.quantity;
              if (typeof duplicatedItem.cancelQuantity === "number") duplicatedItem.cancelQuantity += item.cancelQuantity;
              fixedItems.push(duplicatedItem);
              const index = acc.findIndex((a) => a.id === duplicatedItem.id);
              acc.splice(index, 1);
            } else {
              fixedItems.push(new CheckoutItem(item));
            }
          });
          return [...acc, ...fixedItems];
        }, [] as CheckoutItem[]);
      } else {
        return [];
      }
    } else {
      return this.getItems();
    }
  }
}

#### immer を使っていると、イミュータブルなオブジェクトになる
immerを使っているので、  acc[index].cancelQuantity += item.cancelQuantity;のような書き方はできない。書き換えたいなら、immerのproduceを使って書きかえる。

before

          const index = acc.findIndex(({ product: { id } }) => id === item.product.id);
          acc[index].quantity += item.quantity;
          acc[index].cancelQuantity += item.cancelQuantity;

after
          const index = acc.findIndex(({ product: { id } }) => id === item.product.id);
          const current = acc[index];
          acc[index] = produce(current, (draft) => {
            draft.quantity = current.quantity + item.quantity;
            draft.cancelQuantity = current.cancelQuantity + item.cancelQuantity;
          });

#### やりたいことが分かれば、シンプルにロジックをかける

####  reduce ではなく mapでもいけるならmapを使う
mapの方がシンプルな記述になるため