Webアプリケーション(Rails)のコードレビューをする時に気にしている観点

コードをレビューする時にいつも似たようなところを指摘している気がしたので自分が気にしている観点をまとめてみました。
使用言語に依存しない観点もありますが、私は仕事では主にRailsを使っているので、RubyやRailsに特化した観点もあります。

とりあえず現時点で思いつくものをざっと書きましたが、今後も気にしている観点を思い出したら追記していきます。
開発もQiita記事も最初から完璧を目指さずに継続してデリバリー(CD)していくのがよいですね。

インデントやコーディングルール

インデントやコーディングルールに従っているかなど、コードの静的な部分のチェックは静的解析ツールを使いましょう。
人が目視で確認するのは時間がもったいないし、漏れやブレが発生します。
また細かいところにばかりに気が取られてしまい本来指摘すべきところを見逃してしまいます。

静的解析ツールはRubyではRubocopが有名です。
このような開発をサポートする機能の導入は初期開発では後回しにされがちですが、静的解析ツールは途中で入れるのがとても大変です。
後から追加しようとすると、ルールを緩和したり免除するコードがでてきてしまうので初期開発の段階で入れるようにしましょう。
個人的にはきつめに設定しておいて、必要に応じて緩和していくのが良いと思います。

静的コードチェックに縛られすぎないか

1つ前に静的コードチェックを使えと書いたばかりで早速矛盾するテーマなのですが、静的コードチェックの指摘を直すためにわかりづらい修正をするくらいなら静的コードチェックの方を緩和したり例外を設定した方が良いです。
よくあるのがメソッドの行数超過です。
メソッドが一定行を超えると分割しろと言われるのですが、処理が1つのまとまりであるとことが妥当な場合はわかりづらくなってしまうだけなので無理に分ける必要はないと思います。

HTTPのステータスコードは適切か

Railsだとステータスコードとメソッドやレスポンスの整合性があっていなくても問題なく動くことが多いので、ステータスコードの使い方が適切かを確認するようにしています。
細かい使い分けは人によって解釈が分かれるところだと思いますが、主要なステータスコードについて私の解釈を書きます。

2XX

リクエストを正常に処理した時のステータスコードです。

201 Created

POSTやPUTリクエストでリソースを生成した時に使います。
201を使う場合はレスポンスボディに生成したリソースを返却することがよくあります。
もし返却するものがないのであれば、204 No Contentを使った方が良いと思います。

204 No Content

DELETEリクエストでリソースを削除した時に使うことが多いですが、POSTやPUTなどでもレスポンスボディがない場合に使います。
これを使う場合はレスポンスボディに値を入れてはいけません。

200 OK

上記以外で正常なレスポンスを返す場合は基本的に200 OKを使えば良いと思います。

3XX系

別のページへリダイレクトさせたい時のステータスコードです。

301 Moved Permanently

恒久的に別のページにリダイレクトさせる場合は301を使います。
例えばWebページを移行した場合などに古いページのURLから新しいページへリダイレクトさせる時に使います。
また、SEO対策としても役立ちます。301を指定すると検索エンジンが新しいURLをインデックスしてくれます。

303 See Other

私も最近まで意識していなかったのですが、リソースを登録・更新させた後にそのリソースの詳細ページなどにリダイレクトさせる場合は303を使います。
下記のようなパターンです。
POST /resources でリソースを新規登録して、リソース情報を表示するページ(GET /resources/:id)へリダイレクト

302 Found

301とは異なり、一時的に別のページにリダイレクトさせる場合は302を使います。
Railsでredirect_toを使うとデフォルトで302になりますが、リダイレクトさせる目的を考えると302の用途は少なく、301 or 303に振り分ける方が適切なことが多いと思います。

4XX系

クライアント起因のエラーが発生した場合のステータスコードです。

404 Not Found

ページが見つからない場合に使われるステータスコードです。
画面に大きく「404」と表示されることも多いので一般の方でも知っている方が多いと思います。

人によっては考え方が分かれるところかもしれませんが、私はリソースが見つからないことがクライアントにとって異常なのか正常なのかを考えて404と200を使い分けるようにしています。
イメージしやすいようにいくつか具体例を挙げます。

• GET /resources/:idでリソースが見つからない場合
  クライアントが存在しないidを指定しているため404を返却

• TODOリストを取得するGET /todosでTODOが0件の場合
  TODOが0件ということは正常動作なので200を返却

• お店検索で絞り込み条件を指定したら0件になった
  クライアントが指定した条件に合致するお店がなかったの404を返却
  SEO観点でも0件のお店検索結果ページを検索エンジンにインデックスさせたくないので404を指定することでインデックスさせない。

401 Unauthorized

認証が必要なアクションに対して有効な認証情報が渡されなかった場合に使用します。

403 Forbidden

認証はしているが利用権限が不足している場合に使用します。
例えば、あるリソースに対して読み取り専用のユーザーが更新を行おうとした場合などに使用します。

ただし、403を使う時に考慮した方が良い点が1つあります。
あるユーザーにとって非公開のリソースにアクセスした場合です。
この場合、システム的に考えるとアクセス権限がないので403を使いたくなりますが、403を返却するとリソースが存在していることがユーザーに伝わってしまいます。
こういう場合は存在自体を知らせない方が良いので、存在しないという意味を込めて404を返却するのが良いでしょう。

400 Bad Request

必須パラメーターが指定されていないなど不正なリクエストが来た場合に使用します。
401, 403, 404に当てはまらない場合には大体400を使うことが多いです。

410 Gone

期間限定のキャンペーンページで期間が終了してページを削除した後など、恒久的に削除するURLにアクセスされた時に使います。
蛇足ですが、最近はキャンペーン期間が終わっても放置されているページが多くて検索した時のノイズになってしまい邪魔ですね。
期限が過ぎたら削除するというタスクも積んでおき、きちんと実行してほしいものです。

5XX系

サーバー起因のエラーが発生した場合のステータスコードです。

500 Internal Server Error

サーバーで予期せぬエラーが発生した場合に使用します。
プログラムで明示的に500エラーを発生させることはあまりないと思うので、明示的に500エラーを返却している箇所があったら適切なエラーを返却するように修正した方が良いと思います。

503 Service Unavailable

定期メンテナンスなどにより、一時的に正常にアクセスできずメンテナンスページを表示する場合などに利用します。

クライアントへのエラーメッセージが適切か

エラーが発生した時にクライアントにエラーメッセージを返却することがあると思いますが、そのメッセージが適切かを確認します。
観点はそのエラーメッセージを受け取ってアクションを起こせるかです。
よくある悪い例は複数入力項目がある画面で1つ入力条件を満たしていない時に「入力項目が不正です」のような項目を特定しないメッセージを返すパターンです。
これだとどの項目が間違っているのか分からず、何をすれば良いか分からなくなります。
「xx項目の値は数値で入力してください」のように項目とエラー原因を伝えてあげましょう。

ただ、ユーザーに細かく伝えることがNGの場合もあります。
例えばID/パスワードを入力するログイン画面で存在するIDを指定してパスワードが間違っていた場合に「パスワードが間違っています」と返してしまうとIDが存在していることがユーザーに伝わってしまいます。
この実装をしてしまうと悪意のあるユーザーが適当にIDを入力して存在しているIDのリストを作ることができてしまいます。
ログイン画面はIDが存在しない場合もパスワードだけが違う場合も「ログイン情報が不正です」のように何が間違っているか特定できないメッセージを返すのが良いです。

apiのレスポンスは意味のある値を返す

最近のWebアプリケーションはフロントとバックエンドを別で作ることが多いと思います。
そのためバックエンドはAPIとして機能を提供することがよくあります。
APIを作る際、列挙型の項目を返却する時は値を見るだけで意味がわかるように返却すべきです。
例えば下記のようなステータスがあるとします。

enum status {
  todo: 1,
  in_progress: 2,
  done: 3
}

これをフロントに返却する場合は数値(1, 2, 3)ではなく名称('todo'など)を返却すべきです。
status=１のように数値を返却するとレスポンスを見た時に1ってなんだっけ？となっていしまいますが、status='todo'のように名称を返却するとパッと見ただけで意味を理解することができます。

配列の並び順が一意になるか

配列を返却する場合は配列の並び順が一意になるかをチェックします。

下記ではuser_idで絞り込んだreviews配列を返却していますが、orderを指定していないのでどのような並び順で返却されるのか不定です。
呼び出し元が並び順を気にしないことが確定しているのであれば良いですが、並び順が定まるようにorderを指定しましょう。

def my_reviews(user_id)
  Review.where(user_id: user_id)
end

下記ではcreated_atの降順を指定しています。これであれば新規作成したものから順番に取得できそうです。

def my_reviews(user_id)
  Review.where(user_id: user_id).order(created_at: :desc)
end

ただこれでもまだ問題があります。created_atが同じreviewがある場合に順番が定まりません。
並び順を確実に定めるには下記のようにorderに指定しているカラムで一意になるようになっているかを考えると良いです。
今回は一意にするために第２ソートにidの降順を指定しました。

def my_reviews(user_id)
  # 1. created_atの降順に並べる。ただしcreated_atはユニーク制約はないので同日がありえる
  # 2. 同日だった場合、idの降順に並べる。idはユニークなので同じ値はありえない
  # => 並び順が一意に定まる
  Review.where(user_id: user_id).order(created_at: :desc, id: :desc)
end

他人が読んでスムーズに理解できること

コードレビューをしている時に理解するのに時間がかかる実装や勘違いしてしまう実装を見かけることがあります。
このような実装は今後このコードを見る人全員が同じように感じてコードリーディングの工数を上げてしまい、バグを埋め込む確率も上げてしまうので可能な限りシンプルな実装にした方が良いです。
どうしてもシンプルにできない場合はコメントを入れるなどすると良いと思います。
Railsなどフレームワークを使っていると下記のようにフレームワークの枠から離れた使い方をしていると勘違いさせてしまうことがあります。
勘違いさせるコードは高確率で後々バグを生み出すのでなくしていきましょう。

class Review
  belongs_to :user
  enum status {
    open: 0,
    close: 1,
  }
end

class User
  # × デフォルトのアソシエーションなのに条件を指定して絞り込んでいる
  has_many :reviews, -> { open }

  # ○ 条件をつける場合はそれがわかる名前にしましょう
  has_many :open_reviews, -> { open }, class_name: 'Review'
end

class Hoge
  def fuga(user_id)
    user = User.find user_id
    # ここだけ見るとuserのreviewsを全部取得していると勘違いする可能性が高い
    user.reviews
  end
end

YAGNI

ご存知の方も多いと思いますが、YAGNIという言葉があります。
詳細はwikipediaを参照してください。
https://ja.wikipedia.org/wiki/YAGNI

上記に記載されている通り、まだ使わない機能を想像して実装しておいてもそのまま使える可能性はほとんどありません。
未来を想像して書いているコードを見かけたら要注意です。実装されたコードを削除してもらうのは心苦しいですが削除しておく方が良いことが多いです。
残しておくと後々それが邪魔になったり、リファクタリングの時に使っていないのに直さないといけなかったりと生産性が下がる可能性があります。

メソッドや定数のscopeは小さくする

外部から呼び出さないメソッドや定数はprivateにしておきましょう。
publicだとそのメソッドや定数に手を入れる時の影響範囲がソース全体になっていしまいますが、privateだと同Class内だけを気にすれば良いのでリファクタリングなどソース修正の影響範囲調査がかなり楽になります。

責務は適切か

各クラスはそれぞれ責務を持っています。その責務が守れているかをチェックします。

例えばアカウントをアクティベートするアクションについて考えてみます。

• アクティベーションするにはtokenが必要
• Accountのstatusとアクティベート日時を更新してtokenを削除する

アクティベーションする時に実行される処理はAccountの責務を持っているAccountモデルが知っているべきことなので、下記の○のような方針で実装されている方が良いです。

class AccountController
  # × アクティベーションの時にすべきことをcontrollerが把握している
  def activate
    account = Account.find_by(token: params[:token])
    account.status = :activated
    account.activated_at = Time.now
    account.token = nil
    account.save!
  end

  # ○ アクティベーションの時にすべきことをcontrollerは知らず、accountモデルに依頼している
  def activate
    account = Account.find_by(token: params[:token])
    account.activate!
  end
end

class Account
  def activate!
    status = :activated
    activated_at = Time.now
    token = nil
    save!
  end
end

includes, eager_load, preload

これらの違いが曖昧な方は、Railsを使っている方は必ず１回は見ていると思われる下記のQiita記事をご覧ください。
https://qiita.com/k0kubun/items/80c5a5494f53bb88dc58

includesはpreloadとeager_loadを勝手に使い分けてくれる便利な存在です。
Railsを使い始めたことはincludesを使っておけばいいだろうくらいに思っていたのですが今の考えは逆です。
基本preloadかeager_loadを使うようにしています。
というのも実装する時にjoinすべきかどうかはきちんと把握して実装すべきと考えているからです。
実装によってjoinしたりしなかったりするincludesは制御が難しく予期せぬ挙動をしてしまう可能性があるので使わない方が良いです。

無駄なjoinはないか

ActiveRecordはとても便利ですが、RailsエンジニアからSQLを遠い存在にしているように感じます。
自分でSQLを書いていると書かないようなSQLも平気で発行してしまいます。
その中でも特に見かけるのは無駄にjoinしているパターンです。
下記に具体例を示します。
organizatoin_idを指定してその組織に所属するuserを取得するメソッドfind_by_organizationです。

class User
  has_many :user_organization

  def self.find_by_organization(organization_id)
    # × organizationsテーブルは結合する必要がない
    # select * from users inner join user_organizations on users.id = user_organizations.user_id inner join organizations on user_organizations.organization_id = organizations.id where organizations.id = #{organization_id}
    joins(user_organization: :organization).merge(Organization.where(id: organization_id))

    # ○ user_organizationsしか結合していない
    # select * from users inner join user_organizations on users.id = user_organizations.user_id where user_organizations.organization_id = #{organization_id}
    joins(:user_organization).merge(UserOrganization.where(organization_id: organization_id))
  end
end

class Organization
  has_many :user_organization
end

class UserOrganization
  belongs_to :user
  belongs_to :organization
end