Railsアプリのコードレビューでよく指摘する点

フィヨルドブートキャンプでは最後のカリキュラムで**自分で考えたWebアプリをリリースする**というのがあります。 最後にコードレビューをしているのですが、似た指摘点が多いのでこちらで共有します。 自分でアプリを作ったときにも同じような点が無いか確認すると良いコードになるかもしれません。 ## READMEの情報が足りない railsのwebサービスのリポジトリであれば、下記をちゃんと書こう。 - どんなアプリなのか。 - どんなアプリなのかわかりやすいスクリーンショット。（全ページスクショする必要は無い。） - 動作環境。 - 動かし方。 ## bin/setupで動くようにしよう READMEに細々と動かすためのコマンドが貼ってある場合があるが、基本`bin/setup`を叩けば動くようにしよう。 ## 小文字のApi [RailsでAPIを作る時の設定 - komagataのブログ](https://docs.komagata.org/5430) これ。 ## 名前付け - [クラスの命名のアンチパターン - Qiita](https://qiita.com/magicant/items/8134edf969f9629fa66e) - [プログラミング初心者は変数名やメソッド名を略さない方がいいよ、という話 - give IT a try](https://blog.jnito.com/entry/2020/10/20/092724) - [\[RubyTips\] クラスは名詞、メソッドは動詞 \- komagataのブログ](https://docs.komagata.org/5735) ## Fat Controller controllerにはcontrol程度の処理（モデル等のクラスのメソッド呼び出す、別のページにリダイレクトする）のみを書くべきで、込み入った処理はモデルのメソッドにすべきです。 世の中で**Fat Model問題**と言われているのはあくまで**Fat Controller**を解消した結果、ModelがFatになってしまったのでそれをどうすべきかという問題であって、controllerをスリムにするのは大前提です。 controllerでやっていた処理をmodelでやるようになるととてもテストがしやすくなることに気づくはずです。 ## Fat Model なんでもModelに詰め込んで長くなっていたら注意。 フィヨルドブートキャンプでも**Rubyのオブジェクト指向プログラミング**のカリキュラムでクラス設計を学びますが、それを思い出してモデリングしてみよう。 `app/models/`以下に何も継承しないクラスを作って良いんだよ。 元のコードから単純にメソッドを抜き出して新しいクラスにする人が多いがそのやり方だと大抵変なコードになります。きちんと考えて再設計しましょう。 ## belongs_toしてるものは最初から必須 ```ruby belongs_to :user validates :user, presence: true # 要らない ``` デフォルトでbelongs_toになってるものは必須になってるのでわざわざ必須のvalidatesを書く必要はない。 ## CRUD以外のactionを使っている [DHHはどのようにRailsのコントローラを書くのか | POSTD](https://postd.cc/how-dhh-organizes-his-rails-controllers/) デフォルト以外のactionメソッドがある場合は上記に従ってControllerを分割しよう。 ## 不要なものまでrender :jsonしている ```ruby render json: @user ``` JSON APIを作る時に不要なものまで出してしまうとセキュリティホールになるのでjbuilderで項目を絞ると良い。 ## 不要なActionCableがある ActionCableを使ってない場合は、関連コード丸っと消せるので消すと良い。 ## booleanを返す場合はxxx? booleanを返すメソッドは名前の最後に?をつけると良い。 逆にbooleanを返さないのに`?`が末尾にあると混乱してしまう。 ## 実装のないファイルは消す メソッドのないHelperやTestなど中身のないファイルは丸ごと消すと良い。 ## 不要なroutes ```ruby resources :reports, only: %i(index new create) ``` 使ってないroutesはonlyなどできっちり設定しよう。 ## resource以外のroutesを多用している `get`や`post`はあまり使わず、基本的には`resource`, `resources` を使ってroutesを設定しましょう。 ## model, helperのテストが不十分 modelやhelperにメソッドを追加した場合は最低でも対応する正常系のテストが一つは欲しいです。異常系のテストもあればなお良いです。 ```ruby class User < ApplicationRecord def self.foo # ... end def bar # ... end end ``` ```ruby class UserTest < ActiveSupport::TestCase test '.foo' do # ... end test '#bar' do # ... end end ``` ↑少なくとも1メソッドに対して1つは欲しい。 ## validationをテストしている validationに関するエラーはテスト不要です。何故ならvalidationはActiveRecordの機能であって自分のアプリの機能ではないからです。validationのテストはActiveRecord（ライブラリ）側にあるべきテストです。 （自作のvalidationメソッドやValidatorクラスを作った場合は必要です） ```ruby class User < ApplicationRecord validates :name, presence: true end ``` ```ruby class UserTest < ActiveSupport::TestCase test 'nameが無いとエラーになる' do # ... end end ``` ↑要らない。