PRにマイルストーンを明記する
プルリクエストを作成する際に、その実装の背景の説明だけではなく、全体でどのような機能を作成しようとしていて、今はどの段階なのかマイルストーンを明示すると、レビュワーの人が安心できます。
レビューの1回の指摘で、他のファイルにも応用できないかを考える
何かレビューで指摘をいただいた際に、別のファイルでもその内容が適用できないか考えましょう。例えば「変数に接頭辞をつけなくて良い」といったレビューが一箇所あった際に、別のファイルやテストでも同様のミスをしている可能性があります。1回の指摘で、他の箇所にも直す箇所がないか考えましょう。

具体のコードやコマンドに関するもの

rails consoleの際の--sandboxオプション
rails consoleを使用してデータの確認などする機会があった際に、--sandboxのオプションをつけるとデータの変更を行っても、セッションが終了すると元に戻る処理が実行されるので便利です。そもそもconsoleを使わないと絶対に確認できないか？など考える必要もありますが、実行する際は意識しておくと良いでしょう。

コードを動かしたときにデータが変更されないようにするには、以下のようにbin/rails console --sandboxを実行します。

本記事の執筆にあたり調査したところ、やはり安易に使わない方が良いことを解説してくださっている記事もありました。大変参考になりました。

テストが落ちた際、テストとロジックどちらに問題があるのか落ち着いて切り分ける
テストが落ちたときに、何かロジックにミスがあるのだと自分は思いがちでしたが、実はテストのやり方が悪かったなんてことがありました。時間を無駄にしてしまいました...。テストコードにも誤りがないかdebuggerを使うなどして調査しましょう。
安易に継承をしない
RailsでModelを作成した際に、デフォルトで表記される継承の記述をそのままにしていました。

class Sample < ApplicationRecord
end

こんな感じです。そのままPRを作成した際に、以下のようなレビューをいただきました。

今回はActiveRecordの機能を使っていないのでApplicationRecordを継承する必要はなさそうです。
ちなみにDBに永続化はしないけど、ActiveRecordのオブジェクトっぽく振る舞うモデルを作りたいときはActiveModel::Modelをincludeするという方法があります。

確かに〜〜〜と思いました。これまでは作成したモデルをそのまま使う、デフォルトで継承しているものをあまり意識できていなかったのですが、注意しようと思いました。

呼び出す側で値の判定をしない
そのクラスやメソッドの責任範囲（責務）を意識しましょう。
例えば、メソッドの呼び出し側は必要があれば値を渡して、メソッドに正しい処理を実行してもらい、返却された値を活用したいです。

返却された値をif文などで条件分岐する際に、その条件分岐の責務はメソッド側にないかを考えましょう。

例えば、極端ですがUserのクラスに成人かどうかを判定するメソッドを追加する際は、以下のような感じのコードで実装できるでしょう。

悪い例

class User
  attr_reader :age

  def initialize(age)
    @age = age
  end

  def age
    @age
  end
end

user = User.new(20)

# 受け取った値で成人かどうかを判定する
puts "あなたは成人です。" if user.age >= 18

この例では、年齢の判定の責務はUserが持つべきですし、メソッドの使い回しができない点で悪いと言えます。

良い例

class User
  attr_reader :age

  def initialize(age)
    @age = age
  end

  def adult?
    @age >= 18
  end
end

user = User.new(20)

# adult?を呼び出せばUserクラス側で成人かどうかを判定をしてくれる
puts "あなたは成人です。" if user.adult?

どちらの例も同じifを使用していますが、良い例の方は@ageの判定をUserクラス側で行っています。呼び出す側はadult?のメソッドを信頼して呼び出すだけ良いです。再利用性や読みやすさ（何をしているか）の面でも良いと言えます。

レビューにコメントしたら、Re-request reviewをする

レビューをしてもらって修正などを行い、再度確認してほしい時はここをポチッと押しましょう。
メモ化を活用する
メモ化とは何らかの処理の結果を変数などにキャッシュしておき、同じ変数が呼び出された際に再度処理をせずにキャッシュした結果を使うことだと認識しています。
これにより、再処理をすることがなくなってパフォーマンスが良くなります。

example

 @users ||= User.find(params[:id])

使用できる箇所があったら、積極的に使用するようになりました。

変数名を、受け取り側が使用したい名前にしない。責務を意識する
例えば、ユーザーのプロフィール画像を返すメソッドと、それを受け取ってSNS用のシェア画像として使うケースがあったとします。そのようなメソッドの実装の際に、以下のようなメソッド名にしていました。

  def user_share_image
    # プロフィール画像を取得する処理
  end

レビューでいただいた指摘としては、その画像がどのように使われるかはあくまでも使用側が決めることであり、そのメソッドの責務はユーザーのプロフィール画像を返すことという内容でした。

確かにその通りでした。

  def profile_image
    # プロフィール画像を取得する処理
  end

Userモデルに書いてあればuserという接頭辞を省略しても意味が伝わりますし、プロフィール画像を返す、という責務に応じたメソッド名にしました。

🗺 設計に関するもの

見通しを立てる
当然のことではありますが、見通しを立てて設計を立てることが大切だと考えています。

私の考える見通しとは、次のようなものを指します。

必要なリクエストとレスポンス（適切なステータスコードも）
エラーケースとエラーハンドリング
入力される値のデータ型
今後のビジネスサイドの方針（DB設計などにも関わります）
かかる負荷と対策
コスト...etc

見通しを立てるとは、課題を抽出するとも言えます。
設計はもちろん大切なのですが、起こりうる課題を抽出して見通しを立てて設計しないと、後からの対応が大変になってしまいます。「こんなことが起こるかもしれない」という気持ちで課題を洗い出し、その上で設計をすることが大切だと思っています。

自分一人ではそもそも知らない事情等あると思うので、他のエンジニアやPMにも積極的に設計した図や内容をレビューしてもらうと良いと思います。

🧑‍💻 エンジニアとしての仕事の仕方に関するもの

そのタイミングで人がいる想定で動かない
例えば、見積もりをした結果、多くの機能を開発をする必要があると気づいたとします。その場合、レビューしてもらう回数も増えそうですね。
ここで、レビューをするエンジニアも他の業務で忙しかったり、休暇を取ったりする可能性もあります。他の人が稼働できる前提で見積もると、期限内に実装が完了しないことがあるのでお互い注意しましょう。
プルリクの粒度を考える
ある機能を作成する際、エンジニアになったころの自分は1つのプルリクに全部をまとめていました。これだとレビューがしづらいですし、ユーザーがいきなり機能を使えるようになるので、リスクが大きいです。
DB、バックエンド、フロントエンドなどといった単位で実装を分けて、プルリクの粒度を小さくすると良いと思います。
ドキュメントに影響が及んでないか考える
新機能の実装や、これまでの機能の改善の過程でREADMEやユーザー、社内のドキュメントに影響がないか考えましょう。例えば、環境構築の方法を簡略したが、READMEの変更をしておらず、新しく入社した人が困ってしまうなどのケースが考えられます。
影響範囲はコード状だけとは限らないので、何か他にも直すものがないかと考えてみると良いと思いました。
お問い合わせが来るかもしれないので、関係各所に共有する
1つ前の内容とも重複しますが、機能改善や新機能の追加によって、ユーザーさんがお問合せをする可能性があります。会社によって違いますが、その時に一次対応をしてくれるのは、実装したエンジニア以外の可能性もあります。
そこで、実装が完了したら変更点を関係各所に伝え、お問合せの可能性があることを伝えるのも大切だなと考えています。
実装前に、根本を改善できないか考える
今後実装する機能の話が上がったタイミングで、以下のような内容も一緒に考えたいと思っています。

そもそもその実装は必要そうか
根本の改善すべき課題はなにか

よく話を聞いてみると、新しい実装が必要なかったり、そもそも実装しようと打診されていることと、解決すべき課題が違うこともあります。言われたものをどう実装するか考える前に、実装が必要なのか、相手の解決したいことはどのようなことなのかを考えるようにしましょう。

🔗 教えていただいたリンク

最後に、業務で共有していただいたリンクで、特に都度見ているものです。
社内ドキュメントなどが多いので、以下2つだけの共有です🙏

コードレビューの基準 | google-eng-practices-ja

t_wadaさんのポスト

記事の概要

背景

対象読者

チームでの開発に関するもの

具体のコードやコマンドに関するもの

🗺 設計に関するもの

🧑‍💻 エンジニアとしての仕事の仕方に関するもの

🔗 教えていただいたリンク

Discussion