rails_best_practicesの指摘を修正してみた
今回はrails_best_practicesの指摘を修正してみる。 survey!に掛けてみたところ23個の警告が出た。
インストール
developmentグループのところにgemを追加してbundle install。
Gemfile
gem 'rails_best_practices'
実行
今回はhtmlに出力させたいので以下で実行。
bundle exec rails_best_practices -f html .
警告一覧
Filename | Line Number | Warning Message |
---|---|---|
app/views/survey/_result_question.html.slim | 14 | move code into model (q use_count > 2) |
app/views/kaminari/_paginator.html.slim | 6 | move code into model (page use_count > 2) |
config/routes.rb | 7 | needless deep nesting (nested_count > 2) |
app/helpers/answer_helper.rb | 1 | remove empty helpers |
app/helpers/question_helper.rb | 1 | remove empty helpers |
app/helpers/survey_helper.rb | 1 | remove empty helpers |
app/helpers/welcome_helper.rb | 1 | remove empty helpers |
app/models/free_choice.rb | 2 | remove unused methods (FreeChoice#value_data) |
app/models/surveys.rb | 5 | remove unused methods (Surveys#my_collab) |
app/models/multiple_choice.rb | 10 | remove unused methods (MultipleChoice#value_data) |
app/models/date_choice.rb | 12 | remove unused methods (DateChoice#value_data) |
app/models/question.rb | 21 | remove unused methods (Question#resize_image_url) |
app/models/single_choice.rb | 12 | remove unused methods (SingleChoice#value_data) |
app/views/question/_single_choice.html.slim | 1 | replace instance variable with local variable |
app/views/question/_single_choice.html.slim | 3 | replace instance variable with local variable |
app/views/question/_multiple_choice.html.slim | 1 | replace instance variable with local variable |
app/views/question/_multiple_choice.html.slim | 4 | replace instance variable with local variable |
app/views/question/_date_choice.html.slim | 1 | replace instance variable with local variable |
app/views/question/_date_choice.html.slim | 8 | replace instance variable with local variable |
app/views/question/_free_choice.html.slim | 2 | replace instance variable with local variable |
app/controllers/survey_controller.rb | 46 | simplify render in controllers |
app/controllers/survey_controller.rb | 62 | simplify render in controllers |
app/views/welcome/index.html.slim | 13 | remove trailing whitespace |
move code into model (q use_count > 2)
モデルに条件を寄せたほうが良いということ。 今回はdecorator側へ移動。 Kaminari Themesのテンプレートにも指摘があったが今回はexclude指定することにした。
before
app/views/survey/_result_question.html.slim
- if q.single_choice? || q.multiple_choice? || q.date_choice?
after
app/decorators/question_decorator.rb
def choice? single_choice? || multiple_choice? || date_choice? end
app/views/survey/_result_question.html.slim
- if q.choice?
needless deep nesting (nested_count > 2)
ルーティングのネストが3つになっているのがまずいらしい。 ここで言うと答え(answer)は質問(question)に属しており、アンケート(survey)に属しているわけではないということらしい。
before
config/routes.rb
resources :survey do resources :question, only: :show do resources :answer, only: [:create, :update] end end
after
config/routes.rb
resources :survey do resources :question, only: :show end resources :question, only: :show do resources :answer, only: [:create, :update] end
確かに書いている時にも3つはやり過ぎかなとは思ったが・・・。 しかし、これを変えると基点テーブルが変わるのでActiveRecordの書き方も変えないといけない。
remove empty helpers
空のヘルパーはロード時間もかかるし勿体無いということだ。 指摘の内容のソースを実行して削除。
Dir.glob("app/helpers/**/*.rb").each do |file| if !File.read(file).index('def') FileUtils.rm file FileUtils.rm_f file.sub("app/", "test/unit/").sub(".rb", "_test.rb") if File.exists?("test") FileUtils.rm_f file.sub("app/", "spec/").sub(".rb", "_spec.rb") if File.exist?("spec") end end
remove unused methods
タイトルの通り使われていないメソッドの削除。
replace instance variable with local variable
partialはインスタンス変数を参照せずに、localで変数を渡したものだけにした方が良い。 明示的に指定することで何が必要かを明確に出来るメリットが有る。
simplify render in controllers
renderを簡潔に書けということ。
before
app/controllers/survey_controller.rb
render action: 'edit'
after
app/controllers/survey_controller.rb
render :edit
remove trailing whitespace
不要な行末の空白を削除しろということ。 テーブルで空のヘッダがあったのを以下のように記述していたのでパイプ部分を削除。
before
th.two.wide.computer.only |
after
th.two.wide.computer.only
結果
まとめ
ベストプラクティスがツールでチェック出来るのは便利で、Railsを学ぶ上では非常に良いと感じた。 そして知っていたとしてもチェックの意味もあるのでお勧めだ。