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

目次
概要
Rails
- no_contentなのにレスポンスボディを返さない
- 変数名がシリアライザー名と同じ場合、シリアライザーを指定しなくて良い
React
仕様を明確に決めてから実装する
Stateをいろんなところで更新できるようにしない
onClickにonをつけることで、callbackの発火させるタイミングとしての意味合いが強くなる
レビュワーが考え込むような複雑なコードを書かない
利用頻度の高い関数で毎回同じ値を渡す場合、面倒なのでラッパー関数を作る
終わり
参考記事

概要

最近受けたコードレビューで知らなかったことをざっくりまとめます！

Rails

no_contentなのにレスポンスボディを返さない

ステータスがno_contentの場合、レスポンスボディは空である必要があるので、レスポンスボディにjsonを含めたりしないように気をつけましょう。Railsの場合、以下のように書くことでステータスがno_contentのレスポンスを返すことができます。

head :no_content

変数名がシリアライザー名と同じ場合、シリアライザーを指定しなくて良い

変数をrender jsonする場合、変数名(もしくはモデル名?)に基づいて、Railsは自動的にシリアライザーを探して利用してくれます。そのため、コントローラでわざわざシリアライザーを指定しなくても大丈夫です。シリアライザー内でbelogs_toなどを利用するときも同様です。

# before
# シリアライザー内のコード
belongs_to :shipping_address, serializer: ShippingAddressSerializer

# after
# シリアライザー内のコード
belongs_to :shipping_address

シリアライザーの良いところは、内部の実装を変更せずにどんなデータを返すのかをシリアライザー側で調整できるところだと最近思いました。シリアライザーのおかげで、ある特定のユースケースで内部の実装を一気に変更するのを防ぐことができました。

React

真偽値の時の変数名とコンポーネントのprop名に気をつける

真偽値の変数名は多くの場合、is ~と命名します。なぜisをつけるのかと調べたところ、「if 主語に続けてis ~ を記述した際に自然な英文になるから、isが付けられるようになった」って説が自分の中でしっくりきました。

// この命名に従うとif文が非常に読みやすいです
if current_user.is_active
  pass

次にコンポーネントの真偽値のprop名です。コンポーネントのpropsで真偽値を渡す理由の多くが、表示を制御したいためです。そのような場合、prop名をis ~ にするとコンポーネントを使う人は「is ~って何？」となって、コンポーネント内部でis ~ がどのように使われているのかを確認する必要があります。Reactでは、自分自身の状態を管理するカプセル化されたコンポーネントを組み合わせて、複雑なUIを構築します。カプセル化されたコンポーネントとは、内部でどんな処理が行なわれているのか、状態を持っているのかを秘匿して、渡した値に応じて必要なUIだけを提供するようなコンポーネントです。このようなカプセル化をすることで、このコンポーネントを使う人が内部実装を理解しなくても良くなり、このコンポーネントを使う人の負担が減ります。そして、変更の影響もこのコンポーネントだけに局所化できます。is ~というprop名を使うと、コンポーネント内部の実装を見に行く必要があるので、カプセル化が正しくできていないということになります。このコンポーネントを利用する人にも負担をかけてしまいます。利用する人の負担を下げるためにも、prop名をshow ~ にすると良いでしょう。show ~ にすることで、このpropに真偽値を指定すると、 ~ が表示されるんだなということがコンポーネント内の実装を見なくても直感的に理解することができます。

<Order showPrice={true}>

同じfeaturesからインポートする際は、相対パスで書く

同じfeaturesであることを明示したいためでもありますが、絶対パスだと、相互依存の問題でインポートに失敗する可能性があるためです。ここら辺はあまりわかっていないので、次もう一度同じ問題に遭遇した時に調べます。

&で交差型を作れる

交差型とは、複数のオブジェクトの型をまとめた一つの型のことです。交差型を作ることで、オブジェクトの型を拡張することができます。交差型を作るためには、型同士を&で列挙します。型の一部を共有したい場合、交差型を使うことで以下のように書くことができます。

type BaseData = {
  id: string;
  logId: string;
};

type UpdateData = BaseData & PatchProductRequest;

type DestroyData = BaseData;

0時のDateオブジェクトを作成する

Dateオブジェクトを作成する際、複数の引数を指定することで、特定の日付のDateオブジェクトを作成できます。引数を指定しない場合、使用可能な最小の値が指定されます。そのため、あえて時間の部分で引数を指定しないことで、0時のDateオブジェクトを作成できます。

const now = new Date();
const today = new Date(now.getFullYear(), now.getMonth(), now.getDate())
console.log(today); // => 2023-01-28T00:00:00.000Z

日付のフォーマット用の関数を使って、日付の比較をしない

日付のフォーマット用の関数は、そもそも日付の表示を整えて表示することが目的です。そのフォーマット関数でフォーマットを整えてから比較するとなると、フォーマット関数の本来の目的からズレるし、そもそも一回フォーマットを挟むのが面倒です。このような場合、date-fnsというライブラリのisSameDayを利用すると、Dateオブジェクトを渡すだけで簡単に日付の比較ができます。

isSameDay(product.surveyedAt, new Date())

子コンポーネントで定義した関数を親コンポーネントで使う

あまり、利用するケースは少ないですが、Propsで以下のように書くことによって、子コンポーネントで定義した関数を親コンポーネントで使うことができます。結構強引なやり方なので、他の方法がどうしてもない場合に利用しましょう。

// 子コンポーネント
type Props = {
  onSuccess: (setError: UseForm) => void;
};

const Todo: FC<Props> = ({ onSuccess }) = {
  const setError = // 省略
  const onClick = useCallback(() => {
    onSuccess(setError);
  }, [])

  return (
    // 省略
  );
};

仕様を明確に決めてから実装する

これはReactに限らず、機能がどんな仕様であるかを最初に具体的に全部決めてから実装にかかる方が後戻りがないので良いです。あと、実装方法が固まらないことも防ぐことができます。

Stateをいろんなところで更新できるようにしない

Stateをいろんなところで更新できるようした場合、どこがでStateが更新されているという視点を常に持ちながらコードを書く必要があります。これは他の開発者にとって、とても負担です。そして、どこからどんな変更が入るのかが分からなくなり、バグの温床になりやすいです。カスタムフック内でStateを定義した場合、Stateの更新処理は、カスタムフック内にできるだけ秘匿した方が良いでしょう。コンポーネント内にStateを定義した場合も同様で、そのコンポーネント内でStateの更新処理をするようにしましょう。こうすることで、Stateの更新箇所をカスタムフックやコンポーネントだけに局所化できます。

onClickにonをつけることで、callbackの発火させるタイミングとしての意味合いが強くなる

onClickやonSelectedなどに、なんでonがついているのかと調べたところ、onをつけることで、callbackの発火させるタイミングとしての意味合いが強くなるそうです。このような慣習があることによって、子コンポーネントのPropsの型情報を知っているだけで、子コンポーネントの呼び出し側は子コンポーネントの内部の実装を読まなくても、このpropに関数を渡せばこのタイミングで実行されるなということが分かります。

レビュワーが考え込むような複雑なコードを書かない

どんな値でStateが更新されているのかが分かりづらいようなコードを書かない方が良いでしょう。レビュワーがサッとわかるような分かりやすいコードを書く方がレビュワーの負担がなくなるので良いです。

利用頻度の高い関数で毎回同じ値を渡す場合、面倒なのでラッパー関数を作る

以下の場合、flash関数を呼び出すことでフラッシュメッセージを表示するのですが、第一引数に定数を渡しています。

// 関数呼び出し
flash(FlashType.SUCCESS, "商品情報を更新しました"),

利用頻度の高い関数で毎回このような定数を渡すのは面倒です。そのため、以下のようなラッパー関数を作ります。このようなラッパー関数を作ることで、関数の使い手の負担が減ります。

// 関数定義
const flashSuccess = useCallback((message: string) => flash(FlashType.SUCCESS, message), [flash]);

// 関数呼び出し
flashSuccess("商品ログ情報を更新しました"),

外部に公開する値は命名に気をつける

値を外部に公開する場合、可能な限り内部の実装に依存しない一般的な名称で露出した方がいいです。内部の実装に依存していない一般的な名称にした方が、使い手が理解しやすいです。

処理を実行するメソッドや関数は「動詞 + 名詞」 or 「動詞」にする

処理を実行する系のメソッドや関数は、「動詞 + 名詞」or 「名詞」にすると分かりやすいです。あくまでも、処理を実行する系のメソッドや関数の場合なので、その点を気をつけましょう。

何のものなのかが分からない命名をしない

コンポーネント名で、BrandLogoと命名すると、カードのブランドロゴなのか、自社のブランドに関するロゴなのか、もしくは他社なのか、分かりません。もしカードのブランドロゴの文脈で命名している場合、コンポーネント名はCardBrandLogoにした方が良いでしょう。

APIレスポンスのあるプロパティのデータ型を定義する際に、インスタンスしか受け付けないような型を定義しない

これは、フロントエンドでモデルを利用する際に気をつけるべきです。あるプロパティのデータ型をモデル内で定義する際に、クラスの型を指定すると、インスタンスしか受け付けないようになってしまいます。その結果、モデルのプロパティの型がクラスの型に引きづられてしまいます。APIのレスポンスデータを受け取るのにクラスの型を指定するのは、不適切です。この場合、OpenAPI Generatorなどで生成したAPIレスポンスの型を利用しましょう。