【Rails】「rubocop」の導入、ソースレビューのポイントなど

Gemfileに以下を追加

次に以下のコマンドを実行する。

そうすると「rubocop.yml」というファイルができる。以下の設定を追加する。

実行

自動修正

.rubocop_todo.yml

rubocopは初期状態だと指摘がかなり厳しいです。一旦今出てるエラーを無視するような設定ファイルを生成してくれます。

中身には指摘を無視する設定がいろいろ書かれています。
最初はこれを全て適用しておいてこの指摘の無視は必要だと思ったら徐々に、「.rubocop.yml」の方に移したり、rubocop_todo.ymlの中から削除したりと対応をしていく運用が想定されています。

Ruby自体のコードレビューのポイント

参考（ruby-style-guide）

https://github.com/rubocop-hq/ruby-style-guide

コーディング規則、推奨されるコーディング方法

• インデントはスペース2つ
• for文はどうしても使わなければならないという明確な理由がなければ使わない。
• 引数のないメソッドを呼び出す際は()を省略して記載する。（例：people.methods()ではなく、people.methodsとする。)
• 真偽値を返すメソッドを作成した場合はメソッド名の最後に「?」がついているか。
• 破壊的メソッドの場合はメソッド名の最後に「!」がついているか。
• ブロックを中カッコ「{}」で表現する際はかっこと値の間を開けること（例：{ num })
• 配列は積極的に「%記法」を使うこと。（%w、%W、%i）
• 分岐のないif文の場合は後置ifを使うことを検討すること。
• 分岐があるが単純なif文の場合は三項演算子を使うことを検討すること。
• 論理演算子はand、orというような英単語ではなく記号(&&、||)で書くこと。

ソースコード解析ツール

ruby-style-guideのルールに沿ってソースコードをチェックする「Rubocop(ルボコップ)」というツールをこのruby-style-guideの作者の方が作成しております。実際の現場でも

Railsコードレビューのポイント

全体

• メソッド名、変数名の名前が適切か
• 配列が複数なのに単数になっていないか。
• 異なるクラスのインスタンスが入っていると誤解されるような変数名になっていないか。
• 複数モデルやコントローラの共通機能がconcernsディレクトリにモジュールとして切り出されているか。
• blank?が多用されていないか。
• ログが出力されているか。
• コードの意図がすぐにわかるか。わからないならコメントがあるか。
• 日時フォーマット(strftime）は共通化しているか。

記述場所

• コードの記述場所が適切か？（コントローラ、モデル等）
• メソッドの記述場所としてクラス/インスタンスの使い分けは適切か。

ビュー

ディレクトリ

app/views/ビュー名（例：users)

コントローラ

• フィルタ(before_action等)の利用が適切か。
• URL設計が適切か。
• route.rbがコントローラ別にちゃんと分かれているか。
• render前にflashではなくflash.nowが使われているか。
• Strong Parametersが適切に使われているか。
• パラメータ加工で変化させたくないデータをセッションではなくパラメータ経由で次ページに送ってないか。
• 直打ちできるようになっていないか。

コントローラ名

モデルの複数形(例：users)が一般的だが、フォームコントローラとかinfoコントローラとかDBと直接連携しないコントローラの場合は複数形にしない場合もある。

コントローラファイル(app/controller/配下）

スネークケースで記述します。

モデル

• コールバック(before_save等)が適切か。
• saveとsave!の使い分け
• 戻り値がエラーになる場合の確認をしているか。
• ActiveRecordの関連名が適切か。
• ActiveRecordの関連のdependent: :destroyが正しく設定されているか。
• 汎用的な検索条件がscopeとして実装されているか。
• マイグレーションは適切か。
• SQL実行回数の問題はないか。

モデル名

• 単数形の名詞で作成する。
• 基本的にはdog等の「可算名詞」を使う。（infomartionのように非可算名詞は使わない。）

クラス名

先頭大文字で後は小文字にする。（例：User）

モデルのファイル名や「rails g model」コマンド実行時の指定

全て小文字でモデル名を指定する。(例：user)

テーブル名

モデルの複数形にする。(例：users)

ヘルパー

• メソッド名が衝突しないか。
• グローバルで見て意味が通るか。

コーディング規約

RuboCopで自動化するのはあり。（メンテナンスが必要だが）

コードレビューの観点

#全体

##条件文

・未定義なら代入するという表現は「||=」を使うようにする。

・「if  + not」ではなく「unless」を使う。

・三項演算子を使って行数を減らす。

・== trueや== false、== nilを明示的に書かない

##メソッド

・メソッドの戻り値は「return」を書かない。

・特定条件でしか参照しない変数を使っていないか。

　→だったら一番上の変数はnilで宣言しちゃって、 when :fulfill_item の中に入ったところで||=で宣言すれば無駄ない。

・メソッド名は原則動詞にする。

##文字列

・"+”ではなく"#{ }"で文字列を連結する

・複数行にわたる文字列はヒアドキュメントを使う

・万が一変更されると困るので、定数はfreezeさせる

・大きな数値を宣言する場合、"_"を入れて読みやすくする

・blank？、present?の使い分け。

##配列

・配列を作るとき、[ ]の代わりに%w( )、%i( )を使う

・配列のmapで配列を順番に処理するとき、"object.method"の代わりに"&:method"を使う

・配列の便利メソッドを使っているか。（find、findIndex、select、map)

　→JavaScriptと同じ。

　→上で記載した通り「&」で受けること。

##ハッシュ

・ハッシュのキーには文字列ではなくシンボルを使う

https://qiita.com/jnchito/items/dedb3b889ab226933ccf

#Model

・mapとplunk、selectの使い分け。

　→DBから直接抽出する場合はplunkを使用して、インスタンスから取得する場合はmapが速い。

　https://doruby.jp/users/kodama/entries/map%E3%81%A8pluck

　select：ActiveRecordが返る。

　plunk：配列が返る。

　→「取得したデータを配列に整形して入れ直す場合」はplunkの方がシンプルになる。

　##selectのメリット

　　・メソッドチェーンを使ってメソッドを呼び出せる。(where、distinct、find等）

　##plunkのデメリット

　　・取得したデータを使ってテーブルへの更新処理などを行う事が出来ない

　　・メソッドチェーンを使ってメソッドを呼び出す事が出来ない

　　・都度SQLを発行するので場合によってはN+1の様な状態も起きてしまう可能性がある。

　##疑問

　→配列とActiveRecordのフィルタリングはどっちがいいのか？以下記事だとActiveRecordとしている。

　　https://qiita.com/jnchito/items/dedb3b889ab226933ccf　　

・whereとmergeの使い分け

　→グルーピングで使えるらしい。

　https://qiita.com/hirohero/items/8194b8012b9a5f6e592e

　→結合先のテーブルのjoinの条件の記述に使えるらしい。modelのscopeで共通化したい場合に使う感じか。

　https://nakaearth.hatenadiary.org/entry/20141017/1413543723

・joinsとincludesの使い分け。

　→includesは、N+1問題に気を使えるらしい。

　joins：結合するモデルをキャッシュしないのでメモリ消費が少なくて済みますが、それにより N+1 問題が起きる。

   ・includesとselectは併用できないので、その場合はjoinsを使うらしい。

・特定の場合のみバリデーションを使いたい場合は、ifを使う。（validatesで作らなくても良い。）

　https://doruby.jp/users/nakanishi/entries/with_options_if_

・特定の場合のみ複数バリデーションを使いたい場合は、with_optionsを使う。

・ActiveRecordのcountよりRubyのlengthを使う。（不要なクエリ呼び出しを減らせる。）

#コントローラー

・scopeを使って検索ロジックはモデルに移す。

・ロジックはモデルに移す。

#view

・ロジックはhelperへ分離する。

・partial化

#spec

・letとlet!、beforeの使い分け。

　→特定のテストケースのみしか変数を使いたくない場合はletを使った方が良い。

　　https://qiita.com/jnchito/items/cdd9eef2ed193267c651

　→letはキャッシュされるんだけども、その範囲はbefore〜it〜afterの範囲で一回しか初期化されないという意味だそうな。

　　つまり上記のコードではあんまり意味がなかったのだった。

　　https://sugamasao.hatenablog.com/entry/20120623/1340441871

　→let!とbeforeは同じタイミングで呼ばれる。（使いわないテストケースの場合でも実行されてしまう。）