【2022/12/21 ~ 2023/1/7】コードレビューで知ったことをざっくりまとめてみた

目次

概要

最近コードレビューを受けて知ったことをまとめます。

Open API

どんなリソースを操作しているかが分かるパスを書く

APIのパスは短ければ良いのかと思ってました。しかし、そういうわけではなく、どんなリソースを操作しているのかがひと目見たら分かるようなパスが良いそうです。

使い手の意図しない挙動が発生するようなAPIを作らない

APIの使い手にとって、挙動が想像しやすいAPIほど使いやすいです。APIの使い手が意図しない挙動を実装するのは使いずらくなるので、やめましょう。

Rails

has_manyで指定する関連モデル名は複数形にする

has_manyで指定する関連モデル名は複数形にする必要があるので、忘れないようにしましょう。

Active Recordオブジェクトの保存時に何か処理を実行したい場合、モデルにコールバックを書く

RailsにおけるコールバックはRailsドキュメントに詳細が書いてあったので引用させていただきます。

コールバックとは、オブジェクトのライフサイクル期間における特定の瞬間に呼び出されるメソッドのことです。コールバックを利用することで、Active Recordオブジェクトが作成/保存/更新/削除/検証/データベースからの読み込み、などのイベント発生時に常に実行されるコードを書くことができます。

つまり、モデルにコールバックを書いておくことで、オブジェクトライフサイクルにおける特定のイベント発生時に、指定したメソッドを呼び出すことができます。 以下の例では、ブロックをafter_createコールバックとして呼ぶということをProductモデルに定義しています。

class Product < ApplicationRecord
  # 省略
  after_create do
    product.update!(surveyed_at: Time.current)
  end
end

hashid-railsのhashidをfindに渡しても検索できる

hashid-railsのhashidをfindに渡しても検索できます。そのため、find_by_hashidを使わなくても大丈夫です。

create_paramsとupdate_paramsが共通とは限らないので、どちらも定義する

将来的に保持する値が異なる可能性があるので、現状が同じだとしても、別々に定義しましょう。

Rspec

メッセージの検証は不要

場合によりますが、ステータスコードだけ分かれば良い場合、メッセージの検証は不要です。

単体取得のAPIをテストする場合、create_listで複数データを作る必要はない

Specは積み重なると、全部実行するのにそこそこ時間がかかります。そのため、節約できるところは節約しましょう。今回の場合は、単体取得のAPIをテストしたいだけなので、create_listを使わずに普通にcreateで単体データを作成すれば大丈夫です。

テストコード上で無駄に具体的な値を指定しない

テストコード上では「何か具体的な値を指定をする場合、意味があるものだ」という認識で見ます。そのため、もし意味がないなら指定しないようにしましょう。

let, let!, beforeをいつ使うのかを明文化する

beforeで作成するデータと、letで直接作成するデータの違いが、よく読み解かないと分からないため、明文化します

• なるべく let を用いて定義する
• let! を使わないとどうしようとないものに関してのみ let! を使う
• 込み入った前提データの作成があれば before で行う

React

propsでFactory関数を受け取らない

この状況の場合、Factory関数は子コンポーネントのonClickイベントにアローを使わずに関数を指定したいから作っています。つまり、子コンポーネントの都合なので、親コンポーネントでFactory関数を定義するのではなく、子コンポーネントに定義しましょう。

TSでクラスのインスタンスを作るときに、括弧を省略しない

TSでクラスのインスタンスを作るときに括弧を省略できます。しかし、分かりづらいのでしないようにしましょう。

■before

new Product

■after

new Product()

モデルのインスタンスの値をハードコーディングしている場合、疑似enumを使う

ハードコーディングを管理していくのは辛いので、擬似enumを利用して、定数を一元管理しましょう。この擬似enumはモデルが持つべき責任なので、モデルのファイルに書いておきましょう。

// 擬似enum
const StorageTemperatures = {
  Normal: "normal",
  Frozen: "frozen",
} as const;
type StorageTemperatures = typeof StorageTemperatures[keyof typeof StorageTemperatures];

フォーマットを変換する系の関数の命名は、convertよりformatの方が良い

format~の方が意味がわかりやすいので、良いです。

モデルのインスタンスを生成しないとできない処理で、インスタンスを生成する必要がないと判断した場合、ヘルパー関数で定義する

モデルのインスタンスを生成しないとできないような処理は、モデルのインスタンスを事前に作る必要があるので面倒です。format系の関数はヘルパー関数として定義した方が使い勝手が良いです。

各modelクラスのメンバーの型を定義する場合、モデル名 + メンバーの型名にする

各modelクラスのメンバーの型を制限する型は、 モデル名 + メンバーのようにモデル名のprefixがついている方が良いです。 これはメンバーの名称の幅が広すぎる場合、名前が衝突する可能性があるためです。

バランスを考えてDRYにする

ベタ書きしすぎても辛いですし、過度にDRYにしすぎても定義元を見たりロジックを読み解かないといけなかったりするので、バランスを考えてDRYにした方が良いです。メリットが多い場合にDRYにした方が良いです。

テーブルヘッダーのカラムにはthタグを使う

テーブルヘッダーのカラムにはthタグを使うので、忘れないようにしましょう。

index.tsxとindex.tsを同階層に配置しない

パスを書くときにindex.tsxが反応しなかったりするので、やめましょう。

TSは関数もオブジェクトなので、メンバーを持つ関数を定義することができる

TSの関数はオブジェクトなので、メンバーを持つ関数を定義できます。ちなみにメンバーとは、クラスやオブジェクト（インスタンス）の持つ変数や関数などのことです。オブジェクトの持つ変数をメンバ変数、オブジェクトの持つ関数をメンバ関数と呼んだりします。言語によってはメンバ変数をプロパティ、メンバ関数をメソッドと呼んだりします。

以下のようにして、関数にメンバーを持たせることができます。

type SomeFuncWithMembers = (() => void) & {
  a: string;
  b: number;
};

const some: SomeFuncWithMembers = () => void(0);
some.a = "aaa";
some.b = 1;

console.log(some.a); // => "aaa" 
console.log(some.b); // => 1

Reactコンポーネントは関数なので、コンポーネントをメンバーに持つコンポーネントを定義することができます。こうすることで、名前空間の衝突を防ぐことができます。テーブルコンポーネントなどの割と名前が衝突しやすいコンポーネントで行うと良いです。

zodで文字列を必須にする場合は、min(1)をつければ良い

zodで文字列を必須にする場合は、min(1)をつけましょう。

更新系の関数もuseProductで持つ

セットにすることでupdate後のstateの更新がやりやすいです、また、更新だけを単独で走らせたいケースはあんまりなく、大体get処理とセットになるためです。そのため、更新系の関数を返すuseUpdateProductを定義する必要はないです。更新系の関数もuseProductで返すようにしましょう。

Props は 慣習上コンポーネントの引数に対して用いる名称

Props は 慣習上コンポーネントの引数に対して用いる名称です。関数の引数の型などに、むやみにPropsと命名しないようにしましょう。

同じpathからのインポートの場合、まとめる

同じpathからのインポートの場合、まとめるようにしましょう。

無駄に条件を適用させる範囲を広げない

フォームコンポーネントの表示をある条件で制御したいのに、フォームコンポーネントの外側のJSXも含めて非表示にする必要はないです。指定した条件で、くくる範囲を広くしすぎないようにしましょう。

Next.jsの環境変数で真偽値を持ちたい場合、0か1を使う

Next.jsの環境変数は文字列しか使えません。そのため、基本的には0か1を使うようにしましょう。

型にはめ込んだ憶え方をしない

三項演算子を使うのか、ifを使うのか、switchを使うのか、論理和演算子を使うのかみたいな事は、「ケースバイケース」です。 そのため、「こういう時はこう書く」ではなく、都度「どう書くのが自然なのか」「どう書くのが一番問題が起きづらいのか」ということを考えながらコーディングできると良いです。

LayoutはgetLayoutパターンで適用させる

LayoutはgetLayoutパターンを使うことで、Layoutの状態を保持しつつページコンポーネントに動的にLayoutを適用させることができます。

終わり

やっぱフロントの指摘が多いなと感じました。バックエンドのタスクはスピーディーにできるようになってきたが、フロントはまだ指摘が多いので、フロントもスピーディーにできるようになりたいなと感じました。

参考記事

Active Record の関連付け - Railsガイド

Active Record コールバック - Railsガイド

GitHub - jcypret/hashid-rails: Use Hashids (http://hashids.org/ruby/) in your Rails app ActiveRecord models.

メンバとは - 意味をわかりやすく - IT用語辞典 e-Words

Next.js12.xのLayouts機能

Next.jsのレイアウトパターン