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

目次

作った機能

• お届け先の一覧ページ
• 支払い方法の一覧ページ
• 会員登録後のWelcomeメールを送信する
• Embla Carouselでカルーセルを作成した。
• お届け先の編集機能
• お届け先の削除機能
• お支払い方法の削除機能

Rails

メールのtext版は自動生成されるので、必要ない。

メール送信はトランザクション外でやる。メール送信はロールバックで取り消されないため。

そもそもトランザクションって、データベースのデータの不整合を防ぐためのものだから、メール送信処理をを囲む意味がないか。

ActiveRecord::Base.transactionの返り値は、最後に実行した処理

インスタンス変数を使うと、いろんなところで変更される可能性があって変更箇所を把握しながらコードを書くのが辛いので、なるべく使わない

before

class User::RegisterService
  # @param [String] email
  # @param [String] password
  # @param [String] password_confirmation
  # @param [String] shopify_cart_id
  # @param [Hash] params
  # @return [User]
  def execute!(email:, password:, password_confirmation:, shopify_cart_id: nil, **params)
    ActiveRecord::Base.transaction do
      @user = User.register!(email:, password:, password_confirmation:, shopify_cart_id:, **params)
      generate_pre_registration_coupon_code(@user)
    end
    RegistrationMailer.on_receipted(@user).deliver_later
    @user
  end

after

class User::RegisterService
  # @param [String] email
  # @param [String] password
  # @param [String] password_confirmation
  # @param [String] shopify_cart_id
  # @param [Hash] params
  # @return [User]
  def execute!(email:, password:, password_confirmation:, shopify_cart_id: nil, **params)
    user = ActiveRecord::Base.transaction do
            user = User.register!(email:, password:, password_confirmation:, shopify_cart_id:, **params)
            generate_pre_registration_coupon_code(user)
            user
          end
    RegistrationMailer.on_receipted(user).deliver_later
    user
  end

管理画面で「公開する」「非公開にする」はどちらか一つだけ表示されていた方が良い

どっちも表示されていたらUI的に分かりづらいので。後マウスを動かさないと非公開にできないのがつらい。

ビジネスに関係したロジックはサーバー側のモデルに集約させる

ビジネスに関係したロジックがいろんなところにあったら、どこを変更すれば良いか分かりづらいので。あと、シリアライザーにビジネスに関係したロジックを書かない

Rspec

フロントエンド

初期の開発ではトランジションの要素を無駄に入れない

以下の理由があります。 - domが複雑になる - デザインが未確定の段階で凝った演出を入れても無駄になる可能性が高い

modalOpenとoepnModalの違いがわかりづらい

openModalはモーダルをオープンにする関数なんだなとわかる。modalOpenはmodalOpendやmodalShownのように命名した方が開いているかどうか分かりやすい。

before

const [modalOpen, openModal, closeModal] = useBooleanState();

after

const [modalOpened, openModal, closeModal] = useBooleanState();

変数名がnotSignInMenuItemGroupsより、notSignedInMenuItemGroupsの方が英語的に自然な命名になる

ある値をpropsで渡さないと実は動かないような、ハイコンテキストなコンポーネントをなるべく作らない。

どんな表示をするのかの責務は、コンポーネントに集中させる。流用性も低くなる。使う側の都合はpropsでどうにかすれば良い。ハイコンテキストになるから、実装を見ないと怖くて使えないコンポーネントになるし。

回している配列が違うだけで、展開されるDOMの構造は一緒なら、DRYにした方が良い。

リストの部分とかはDRYにしない方が良い。例えばリストを5つ出すとかを無理やりDRYにして1行で書くメリットはない。DRYはそもそも変更箇所を一箇所にすることで、変更に強くなろうという原則。なので、リストの部分をDRYにしてもそもそもそこでしか使ってないからメリットがないし、リストごとのスタイルの変更ができなくて柔軟性に欠ける。今回言いたいのは、配列ごとにDOM構造を作るメリットがないので(配列の値に応じて、スタイルを変えるとかがない)、そこはDRYにして変更を最小限にしようって話。

before

       {showMyMenu ? (
          signInMenuItemGroups.map((group) => (
            <div key={group.label}>
              <div className="bg-black-50 p-2 text-sm text-black-400">
                {group.label}
              </div>
              <div className="flex flex-col divide-y text-black-600">
                {group.items.map((item) => (
                  <Link key={item.href} href={item.href} className="flex p-3 items-center justify-between" onClick={onClose}>
                    {item.label}
                    <HiChevronRight size={20} className="text-black-400" />
                  </Link>
                ))}
              </div>
            </div>
          ))
        ) : (
          notSignInMenuItemGroups.map((group) => (
            <div key={group.label}>
              <div className="bg-black-50 p-2 text-sm text-black-400">
                {group.label}
              </div>
              <div className="flex flex-col divide-y text-black-600">
                {group.items.map((item) => (
                  <Link key={item.href} href={item.href} className="flex p-3 items-center justify-between" onClick={onClose}>
                    {item.label}
                    <HiChevronRight size={20} className="text-black-400" />
                  </Link>
                ))}
              </div>
            </div>
          ))
        )}

after

  const { isSignedIn } = useAuthContext();
  const menuItemGroups = isSignedIn ? signInMenuItemGroups : notSignedInMenuItemGroups;

// 省略
     {menuItemGroups.map((group) => (
          <div key={group.label}>
            <div className="bg-black-50 p-2 text-sm text-black-400">
              {group.label}
            </div>
            <div className="flex flex-col divide-y text-black-600">
              {group.items.map((item) => (
                <Link key={item.href} href={item.href} className="flex p-3 items-center justify-between" onClick={onClose}>
                  {item.label}
                  <HiChevronRight size={20} className="text-black-400" />
                </Link>
              ))}
            </div>
          </div>
        ))}

自明なものに対してわざわざprefixをつけない

AddressIndexPageのコンポーネントのpropsのnewPathやeditPathにaddressをつけなくても、自明であり必要でないからつけなくて良い。

before

export const AddressesIndexPage: BFC<Props> = ({
  addressesNewPath,
  addressesEditPath,

after

export const AddressesIndexPage: BFC<Props> = ({
  newPath,
  editPath,

てかページコンポーネントはページ独自の機能が入るので、可能ならば、共有しない方が良い。そうすれば、newPathとかをPropsで渡さずに済む。

ページ単位で独自の機能実装がある場合、ページコンポーネントを分けた方が良い

ページコンポーネントにはページ独自の機能が書かれる。なので、機能的な差異があるなら共有しない方が良い。逆にページコンポーネントにページ独自の機能がない場合(見出しが違うくらいで、ラジオボタンとか独自の機能がない場合等)、ページコンポーネントを共有した方が良い。なぜなら変更箇所が一箇所に集中できるから。

コンテンツは基本的にはフロート（周りにマージンをもった要素）させずに、横幅いっぱいで表示させる

PCの時だけフロートさせる可能性はある

見出しは背景の上に直接おかず、bg-whiteの上に置く

後で表示する可能性が高い情報なので、一旦は表示させちゃって必要があるならクローズする

どんなUIだとユーザーが使いやすいか、開発しやすいかではなくて、どうあった方がサービスとして良いかを優先する。

カルーセルは一つのコンポーネントにした方が抽象化されて分かりやすい。

カルーセルの部分のコードって、そのままベタ書きすると結局何をしているのか分かりづらい。だから関数コンポーネントで抽象化した方が良い。関数を作る目的って、処理の抽象化だったり、処理を再利用できるようにすること。抽象化した方が良い長い処理や、汎用性のある処理に対して、関数を作るので、関数コンポーネントでも大体基本的なことは関数と一緒。

filterを使う際にisを使うと、isに指定した変数の型情報を別のスコープに引き継げた気がする。

TypeScript の"is"と"in"を理解する #TypeScript - Qiita

const isString = (test: unknown): test is string => {
  return typeof test === "string";
};

const example = (foo: unknown) => {
  if (isString(foo)) {
    console.log(foo.length); // fooはstringとして推論される
  }
};

draft.images = p.images.edges.map(({ node: image }) => convertImageObject(image)).filter((image): image is ImageType => !!image);

なんでここにこのコードが書かれているのって他の人が感じるハイコンテキストなコードを書かない

妥協しがちなので、気を付ける。

propにbooleanをtrueで渡したい時はpropだけを書く。そのような慣習がある

before

    <AuthedRoute>
      <AddressEditPage
        returnTo={routes.MYPAGE_ADDRESSES}
        showTitle={true}

after

    <AuthedRoute>
      <AddressEditPage
        returnTo={routes.MYPAGE_ADDRESSES}
        showTitle

両方とも赤いボタンだと、どちらがメインのボタンだかわからないので、削除側は普通の色のボタンにする

「編集」と「削除」が並んでるの、UI的に分かりづらい

「編集」と「削除」が並んでると、誤タップで削除をする可能性があるので、使いづらいUXになる。削除は編集画面においた方が良い。

唐突に削除ボタンを配置するのではなくて、見出しをつける

言われた通りやるんじゃなくて、こうあった方が良いのではで実装して、必要なかったら消すのがプロの仕事

関係のないプロパティで条件分岐を作らない。関係があるように思ってしまうから

showTitleは削除可能かどうかと無関係なプロパティなので、それ用のプロパティを用意した方が良い。

before

        {showTitle && (
          <div className="flex flex-col gap-4 pb-4">
            <div className="bg-white">
              <h1 className="flex items-center gap-2 font-bold px-5 py-6 text-lg">

after

        {showDelete && (
          <div className="flex flex-col gap-4 pb-4">
            <div className="bg-white">
              <h1 className="flex items-center gap-2 font-bold px-5 py-6 text-lg">

削除のためだけに無理に usePaymentMethod を用意しない

usePaymentMethods が destroy(id: string) みたいな関数を返せば良い usePaymentMethodを通すならそもそも destroy に idを渡したくない。

仮想DOMの中でts要素にアクセスするには、{}が必要

つまり、仮想DOM外では、tsのコードを書けるので、無駄なフラグメントを書く必要はない。

before

return (
  <>
    {
        a < 0 ? (
        ) : (
        ) 
    }
  <>
)

after

return a < 0 ? (
) : (
)

こんな感じで処理されている。 tsxのファイルは用はtsのように扱えて、仮想DOMの部分だけちょっと特殊ってだけ。

const A = () => {
  const a = 1;
  const b = a > 0 ? "hoge" : "hage";  // ここで三項演算子を使うのと return のところで使うのには違いが全くない

  return a < 0 ? ( // このラインは普通のtsとして処理される
    <div>{b}</div> // ここはDOMが登場するので仮想DOMとして処理される。ここの中でts要素にアクセスするには「{}」が必要になる */}
  ) : (
    <div>{a}</div> // ここも仮想DOMとして処理される
  );
};

画像だけを表示するコンポーネントの場合、nullを返すと画面に何も表示されなくなって体裁が保てなくなるので、スケルトンを返す

カルーセルでbeforeのようなコードを書いていたので、気を付ける。

before

  if (!product.images.length) {
    return null;
  }

after

  if (!product.images.length) {
    return (
      <div className="border rounded flex justify-center items-center aspect-square">
        <PhotoIcon className="text-black-200 w-8" />
      </div>
    );
  }

あまりにも複雑な条件ならモデルに書く

本当にこの条件じゃないと、条件分岐をできないのかをまずは考えた方が良い。あと、汎用性がないものをメソッドで抽象化するのはなるべく避けたいし、メソッド化することで、どんな条件で分岐されているのかメソッドを見に行かないとわからないって状況もできれば避けたい。多くの場面では、愚直に書いたほうがわかりやすいパターンが多い。

{(checkout.status !== CheckoutStatus.Canceled && checkout.status !== CheckoutStatus.Completed) && (

  isPreparingShipment() {
    return this.status !== CheckoutStatus.Canceled && this.status !== CheckoutStatus.Completed;
  }