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

目次

作った機能

• 終売した商品も画像や商品名を表示する

Rails

スコープ名は形容詞または形容詞句を使う

スコープ名は、形容詞または形容詞句を使う。スコープ名がモデル名を後置修飾する感じ。「どんな〇〇」の「どんな」の部分をスコープ名にして 〇〇にはモデル名が入れる。

Railsモデルのメソッドの命名について #Ruby - Qiita

before

product = Product.where.not(shopify_product_id: nil).find_or_shopify_product_id(params[:id])

after

「エンドユーザーに対して露出していいかどうか」を意味したscopeにする enableだと、「何に対して有効なのか」というところが曖昧である。そのため、 リストできる(listable)と参照できる(showable)のスコープを作る。参照できるは、参照可能なproductだけを参照する。「リストできる」というのは、主に商品の一覧を取得する時のことを表す。

  scope :listable, -> { active.where.not(shopify_product_id: nil) }
  scope :showable, -> { where.not(shopify_product_id: nil) }

動詞で始めるのは、あくまで「処理を実行するメソッド」

動詞で始めるのは、あくまで「処理を実行するメソッド」。毎回同じ値を返すだけのプロパティ系のメソッドとかは動詞はいらない。Rubyの場合、真偽値を返すようなメソッドやデータ型を変換するメソッドも動詞から始めるの対象外である

モデルやメソッドに名前を付けるときは英語の品詞に気をつけよう #Ruby - Qiita

Rubyでは慣習としてselfを省略する

selfが省略できるのは知ってたけど、これが省略できるのは普通に知らなかった

  def inactive?
    !self.active?
  end

  def inactive?
    !active?
  end

スコープ名は文脈が大切

同じようなスコープがあった場合、文脈にあったスコープを選択する

クーポンコードなど、ユーザーの瑣末な一情報をユーザーレスポンスに含めない

無理にfakerを使わない

fakerを使う目的は、「どうでもいい内容を考える脳リソースの節約」のためなので、「テストケースにとって重要なデータ」を定めるときは逆に使わない方が良い。

before

let(:pre_registered_email) { Faker::Internet.email.gsub(/.+(?=@)/, &:upcase) }

let(:pre_registered_email) { "Sample@gmail.com" }

テストを書くなら、まずは通常ケースを検証した後に、異常ケースや特殊ケースを検証する

おそらくこんな感じでテスト書いてた。あまりにも書きすぎると、CIの時間が遅くなるから、必要じゃないテストケースを判断して書かないのも大事。入念にやる必要がないところはあまり書きすぎない

Rspec

フロントエンド

商品のstatusが終了の際にhrefで遷移できないようにする場合、statusでhrefのアドレスを切り替えるというよりかはdisableのようなpropを持った方が良い

純粋にその方がわかりやすいから。

before

<Link key={product.id} href={product.isActive() ? routes.PRODUCTS_SHOW(product.id) : "#"} noDecoration>

after

<Link key={item.id} href={routes.PRODUCTS_SHOW(item.product.id)} disabled={!item.getProduct().isActive()} noDecoration>

// またはこう書いてもいける
<Link key={item.id} href={routes.PRODUCTS_SHOW(item.product.id)} disabled={item.isInactive()} noDecoration>

export const Link: CFC<Props> = ({
  href,
  decorated,
  disabled,
  className,
  children,
}) => {
  const classes = classNames(
    className,
    {
      "border-b border-dashed border-primary": decorated,
    },
  );

  if (disabled) {
    return <div>{children}</div>;
  }

  return (
    <NextLink href={href} className={classes}>
      {children}
    </NextLink>
  );
};

モデルがisActiveなのは当たり前なので、isAnactiveではなくてイレギュラーな方を検知するメソッドを持っていた方が自然だし、否定の必要性もない。

理由がなければ独立していた方が良い

どんな理由で同じ括りになっているのかわかりづらいため

        <div className="flex flex-col gap-2">
          {action}
          {inactive && <div className="text-gray-500">この商品は販売終了しました</div>}
        </div>

        <div className="flex flex-col gap-2">
          {action}
        </div>
       <div className="flex flex-col gap-2">
          {inactive && <div className="text-gray-500">この商品は販売終了しました</div>}
        </div>

archivedの場合、404ページじゃなくて、販売終了しました的な一言が表示されている詳細画面が表示される方が親切

showTotalPriceよりhideTotalPriceのようなオプションの方が自然

もちろん場合にもよるけど、表示されているのをデフォルトとするのが自然な場合、hideTotalPriceをpropにした方が良い。trueにする場合も hideTotalPriceを書けば良いだけになるので。

使う側の特定のユースケースを意識すぎた表示に関するロジックをコンポーネントが持つのは避けた方が良い

汎用性のないコンポーネントになってしまう。使う側の都合は渡す側で指定する。

条件文にisDifferentTotalPriceを指定した場合、isDifferentTotalPrice だけだと、一体何とどう違うのかが分からない

isDifferentTotalPrice だけだと、一体何とどう違うのかが分からない。 （メソッド化してなかった時は、比較対象が明示されているのでむしろわかりやすかった）

メソッド化するのであれば、 - この比較がどういう文脈で使われるのかがわかりやすくなる - 比較内容が誤解なくエンジニアに伝わる のいずれかを満たす必要性がある。

前者なら hasCancelItems のような文脈重視の命名、後者なら isDifferentFromBilling のような処理内容重視のメソッド名が望ましい

toCheckout のように「変換メソッド」であることを明示する

getCheckout だとbillingが内部的にcheckoutのデータを保持していて、それを返却するようなメソッドに見えて、誤解を招きそう。

これは、billingをcheckoutと同等に扱えるようにするメソッドなので toCheckout のように「変換メソッド」であることを明示した方が良い。

before

export class CheckoutBilling implements CheckoutBillingType {
  [immerable] = true;
  subtotalPrice = 0;
  serviceCommission = 0;
  smallCheckoutCommission = 0;
  totalShippingRate = 0;
  discountPrice = 0;
  totalTax = 0;
  totalPrice = 0;
  couponCode: CouponCodeType | undefined = undefined;
  billedAt = new Date();
  constructor(data: Partial<CheckoutBillingType> = {}) {
    Object.assign(this, data);
  }

  getCheckout() {
    return new Checkout(this);
  }
}

export class CheckoutBilling implements CheckoutBillingType {
  [immerable] = true;
  subtotalPrice = 0;
  serviceCommission = 0;
  smallCheckoutCommission = 0;
  totalShippingRate = 0;
  discountPrice = 0;
  totalTax = 0;
  totalPrice = 0;
  couponCode: CouponCodeType | undefined = undefined;
  billedAt = new Date();
  constructor(data: Partial<CheckoutBillingType> = {}) {
    Object.assign(this, data);
  }

  toCheckout() {
    return new Checkout(this);
  }
}

flatMapやSomeも意外と便利

「余白」というのは、コンテンツを見やすくするための重要な要素の一つ

「余白」というのは、コンテンツを見やすくするための重要な要素の一つで、多くの場合「関連の遠いものには大きめの余白を、関連の近いものには小さめの余白を」という暗黙のルールがある。

サイドの余白はサイドバー外と中のコンテンツの境界としての余白で、もちろん関連は遠い リンクアイテム同士は、遷移先の関連性は薄くとも「お互いに同じグループに属するリンクアイテム」として関連は近い という状況にも関わらず、暗黙のルールとは真逆のルールが適用されていて、それを「バランスが悪い」と指摘していますー。

こういう風に、余白の大小には明確な意味や意図があるので、このあたりを意識してなかったのであれば意識する。あと、判断がつかない場合は「最低限同じ余白が確保されること」という風にする。

重要度の低いコンテンツのアイコンは、必要以上の主張をしないアイコンにする。

重要度の低いコンテンツなので、必要以上の主張をしたくないため。