さかなソフトブログ

プログラミングやソフトウェア開発に関する情報

プログラミング

Rubocopを使ってRailsコードをチェックしてみる

更新日：

正直、コード規約、静的解析は余り好きでは無いです。正確には、あるツールのルールによって「意味も無く縛られる」のが嫌いです。

「こうした方が良いよ」っていう助言はとても貴重で有り難いのですが、最終的に「そうした方が良いので対応しよう」と判断するのは実際に実装を行うエンジニアがやるべきです。プロダクトコードに統一感が出て可読性も上がるのであれば当然そちらの方が良いのですが、「ルールは絶対」なんてことにしてしまうとその対応によって生産性が下がってしまうなんていうことになると本末転倒な話です。新しいRuby/Railsバージョンで推奨される書き方を学んでいくためのきっかけとして使うのであって、決して足枷になってはいけません。

・・・前置きが長くなりましたが、Rubyで静的解析を行ってくれるRubocopを試してみましたので使い方と実際にどんな警告が出てどう対応したかを見ていきます。

スポンサーリンク

正方形336

目次

使い方

rubocop gemをGemfileで読み込んでbundle installすればあとは$ rubocopとコマンドを打てばプロジェクトコードがチェックされます。

group :development do
  gem 'rubocop'
end

また、rubocop -hを見ると、-RオプションでRailsコードに関する解析も行ってくれるようです：

$ rubocop -R

規約のカスタマイズ

Rubocopではルートフォルダに.rubocop.ymlがあると自動的に設定を読み込んで実行されます。デフォルトの設定だと長さがどうだとか結構やかましい事言われるみたいなので巷で良さげそうな設定を拝借して、さらにRailsコードのチェック追加とアプリで利用するターゲットコードに絞ってチェックする様に設定しておきます(最後のまとめでチェックした後の最終版があります)：

AllCops:
  TargetRubyVersion: 2.5
  Exclude:
    - bin/*
    - db/schema.rb
    - db/migrate/*
    - vendor/**/*
    - config/**/*
    - lib/tasks/*
    - Gemfile
    - features/**/*
    - spec/**/*

Rails:
  Enabled: true

Style/Documentation:
  Enabled: false

Style/FrozenStringLiteralComment:
  Enabled: false

LineLength:
  Enabled: false

Style/AsciiComments:
  Enabled: false

Style/ClassAndModuleChildren:
  Enabled: false

Metrics/MethodLength:
  Enabled: false

Metrics/ClassLength:
  Enabled: false

Metrics/ModuleLength:
  Enabled: false

Metrics/AbcSize:
  Max: 20

FormatString:
  EnforcedStyle: percent

NumericLiterals:
  Enabled: false

RedundantReturn:
  AllowMultipleReturnValues: true

DotPosition:
  EnforcedStyle: trailing

それではいつものweb-k/tiramisu: WebSocket(Pusher)を使ったチャット＆囲碁Railsアプリに実際に組み込んでチェックしてみます。

解析結果とその対応

実際実行してみると、

$ rubocop
Inspecting 30 files
CWCCCC....C..CCCCCC.....W...W.

Offenses:

config.ru:3:52: C: Layout/ExtraSpacing: Unnecessary spacing detected.
require ::File.expand_path('../config/environment',  __FILE__)
                                                   ^
Rakefile:1:1: W: Lint/ScriptPermission: Script file Rakefile doesn't have execute permission.
#!/usr/bin/env rake
^^^^^^^^^^^^^^^^^^^
Rakefile:5:14: C: Style/ExpandPathArguments: Use expand_path('config/application', __dir__) instead of expand_path('../config/application', __FILE__).
require File.expand_path('../config/application', __FILE__)

...

まあ出るわ出るわ・・・60個くらいの違反があったようです。メッセージ内のCはConvention(慣習)、WはWarning(警告)、EはError(エラー)、FはFatal(致命的)ということで指摘の重大さはC < W < E < Fになります。

自分のアプリではWとCが出ました。それでは個別にW→Cと見ていきたいと思います。

Lint/ScriptPermission

Rakefile:1:1: W: Lint/ScriptPermission: Script file Rakefile doesn't have execute permission.
#!/usr/bin/env rake
^^^^^^^^^^^^^^^^^^^

shell scriptなのに実行権限無いといわれてます。追加します。

Lint/UselessAssignment

app/models/table_template/video_chat.rb:5:9: W: Lint/UselessAssignment: Useless assignment to variable - new_table.
        new_table = Table.create name: 'VideoChat'
        ^^^^^^^^^

使っていないローカル変数の定義があるそう。削除します。

Layout/EndAlignment

app/models/table_template.rb:7:14: W: Layout/EndAlignment: end at 7, 13 is not aligned with case at 3, 4.
    else nil end
             ^^^

caseとendのインデントは揃えたいようです。揃えます。

Rails/ActiveRecordAliases

app/controllers/items_controller.rb:12:11: W: Rails/ActiveRecordAliases: Use update! instead of update_attributes!.
    @item.update_attributes! move_params
          ^^^^^^^^^^^^^^^^^^

Rails4でupdate_attributesはupdateと記述出来る様になったようです。リネームしときます。

Style/ExpandPathArguments

Rakefile:5:14: C: Style/ExpandPathArguments: Use expand_path('config/application', __dir__) instead of expand_path('../config/application', __FILE__).
require File.expand_path('../config/application', __FILE__)
             ^^^^^^^^^^^

Ruby 2.0より実行中の絶対ディレクトリパスを示す__dir__が追加されているのでこちらの方が綺麗に書けるようです。修正します。

Layout/ExtraSpacing

script/rails:4:53: C: Layout/ExtraSpacing: Unnecessary spacing detected.
APP_PATH = File.expand_path('../config/application',  __dir__)

スペースが余分に入ってるらしいです。面倒くさいので無効化します：

Layout/ExtraSpacing:
  Enabled: false

Style/HashSyntax

app/controllers/pusher_controller.rb:2:24: C: Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
  protect_from_forgery :except => :auth
                       ^^^^^^^^^^

Ruby 1.9からexcept: :authと表記出来るからそっち使った方が良いといっています。自分もそっち好きなので対応します。

Metrics/AbcSize

app/controllers/pusher_controller.rb:4:3: C: Metrics/AbcSize: Assignment Branch Condition size for auth is too high. [20.12/20]
  def auth ...
  ^^^^^^^^

ABC Metricsが20を超えたといっているようです。で、コード見ると、

def auth
  if session[:user_name].present?
    session[:user_id] = request.session_options[:id]
    response = Pusher[params[:channel_name]].authenticate(params[:socket_id], {
      user_id: request.session_options[:id],
      user_info: {
        name: session[:user_name]
      }
    })
    render json: response
  else
    render text: "Forbidden", status: '403'
  end
end

うーん。複雑じゃないですね。ABC Metricsは代入、メソッド呼び出し、条件分岐をそれぞれ2乗で足し合わせて平方根を取るらしいですが、

  • 代入は意味を持たせるために意図的に増やす
  • Relationのjoin/where/orderとかStrong Parametersのメソッドチェインであっさり増える
  • 条件分岐もエラーチェックすれば大量に増える。ネストは増えてはいけないとは思う

あたりで、絶対的な量を指標とするのはまずいのではと思います。他の警告もチェックしましたが、複雑なコードは無かったので、警告が出ない数値の40まで上げておきます：

Metrics/AbcSize:
  Max: 40

Style/BracesAroundHashParameters

app/controllers/pusher_controller.rb:7:81: C: Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.
      response = Pusher[params[:channel_name]].authenticate(params[:socket_id], { ...

メソッドパラメータの際のハッシュに{ ... }がついてて余計と言ってます。見やすい場合はつけないし、見にくい場合はつけるので無効化します。

Layout/IndentHash

app/controllers/pusher_controller.rb:8:9: C: Layout/IndentHash: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
        user_id: request.session_options[:id],
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

ハッシュでインデントしているといっています。余計なお世話なので無効化します。

Style/StringLiterals

app/controllers/pusher_controller.rb:15:20: C: Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
      render text: "Forbidden", status: '403'

意味も無くダブルクォーテーション使っていないかということで意味は無いですが縛る必要も無いので無効化します。

Rails/Presence

app/controllers/authentication_controller.rb:10:17: C: Rails/Presence: Use callback_url.presence || root_path instead of callback_url.present? ? callback_url : root_path.
    redirect_to(callback_url.present? ? callback_url : root_path)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

present?じゃないならnilとできるのがpresenceで三項演算子でも意味は分かりやすいから問題無いのではと思いますが覚えておいて損は無いので対応してみます。

Layout/TrailingWhitespace

app/controllers/channels_controller.rb:31:38: C: Layout/TrailingWhitespace: Trailing whitespace detected.
  rescue ActiveRecord::RecordNotFound
                                     ^

行末の余計なスペース。これは見つけにくいので在りがたいです。削っておきます。

Layout/SpaceInsideHashLiteralBraces

app/controllers/channels_controller.rb:32:15: C: Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
    options = {status: 404, error_label: 'not_found'}

ハッシュ{の後にスペースが入ってないといってますが、場合によって入れたり改行入れたりしたいので無効化します。

Layout/EmptyLinesAroundClassBody

app/controllers/channels_controller.rb:42:1: C: Layout/EmptyLinesAroundClassBody: Extra empty line detected at class body end.

class endの前に無駄な改行が入ってるっぽいです。入れたいしどうでも良いので無効化します。

Style/AndOr

app/controllers/messages_controller.rb:10:57: C: Style/AndOr: Use && instead of and.
    render(status: :forbidden, text: 'Invalid message') and return if (params[:message].blank? or params[:message][:content].blank?)
                                                        ^^^

and(or)の代わりに&&(||)を使えとありますが演算子としての優先度も違うしRubyの文化的に従いたくないので無効化します。

Style/ParenthesesAroundCondition

app/controllers/messages_controller.rb:10:57: C: Style/AndOr: Use && instead of and.
    render(status: :forbidden, text: 'Invalid message') and return if (params[:message].blank? or params[:message][:content].blank?)
                                                        ^^^

ifの条件文には括弧つけないでということに見えます。外します。

Layout/IndentationConsistency

app/controllers/messages_controller.rb:15:7: C: Layout/IndentationConsistency: Inconsistent indentation detected.
      if @message.save ...
      ^^^^^^^^^^^^^^^^

インデントが揃ってないとのこと。揃えます。

Style/GuardClause

app/controllers/application_controller.rb:10:5: C: Style/GuardClause: Use a guard clause instead of wrapping the code inside a conditional expression.
    unless login?
    ^^^^^^

return if login?とすればネストしなくて済むからそうして欲しいと言っています。このアプリではアクションが最後まで通るイコール正常系としたいので無効化します。

Style/RedundantReturn

app/controllers/application_controller.rb:13:7: C: Style/RedundantReturn: Redundant return detected.
      return false
      ^^^^^^

最後の評価だからreturn外してfalseにしても同じ挙動するよってことなんですが、コードを見ると、

def authenticate
  unless login?
    callback_url = (request.fullpath.present? && request.fullpath != '/') ? request.fullpath : nil
    redirect_to controller: 'authentication', action: 'index', callback: callback_url
    return false
  end
end

とあって、正常系で後処理が何かあれば後ろに記述していいよという意味が込められています。ここでもしreturnをなくして後でコードを追加すると正常に動作しなくなるのでこのままにすることが好ましいです。なのでこの規約は無効化します。

Style/SymbolLiteral

app/controllers/application_controller.rb:23:76: C: Style/SymbolLiteral: Do not use strings for word-like symbol literals.
    @error_title = ERROR_TITLE[:"#{options[:error_label]}"] || ERROR_TITLE[:'not_found']
                                                                           ^^^^^^^^^^^^

シンボルリテラル定義時に使わなくていい文字列表記:'not_found'を使っているということ。:not_foundでOKなのでシングルクォーテーションは外します。

Layout/SpaceBeforeComma

app/controllers/application_controller.rb:28:38: C: Layout/SpaceBeforeComma: Space found before comma.
        format.html { render '/error' , status: status }
                                     ^

カンマの前にスペースがある。削っておきます。

Rails/ApplicationRecord

app/models/item.rb:1:14: C: Rails/ApplicationRecord: Models should subclass ApplicationRecord.
class Item < ActiveRecord::Base
             ^^^^^^^^^^^^^^^^^^

Rails5よりモデルではActiveRecord::Baseを直接では無くApplicationRecordを継承することを推奨しています。Rails4からアップデートした場合はこれがmodelsフォルダに作られていないので以下でapp/models/application_record.rbを作成して継承しておきます：

class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true
end

Layout/TrailingBlankLines

app/models/table_template/video_chat.rb:10:1: C: Layout/TrailingBlankLines: 1 trailing blank lines detected.

ファイルの最後に余計な改行があるらしいです。削っておきます。

Rails/HasManyOrHasOneDependent

app/models/table.rb:3:3: C: Rails/HasManyOrHasOneDependent: Specify a :dependent option.
  has_many :items
  ^^^^^^^^

Tableを削除したときの関連itemsオブジェクトをどう扱うのかというのを定義してくださいということらしい。削除はしないんだが、もし削除したら要らなくなるので:destroyを指定しておく：

has_many :items, dependent: :destroy

Style/MethodDefParentheses

app/models/table_template.rb:2:17: C: Style/MethodDefParentheses: Use def with parentheses when there are parameters.
  def self.find table_name
                ^^^^^^^^^^

メソッド引数を定義するときは括弧をつけてくださいということなのだが、好みなので無効化する。

最終的な.rubocop.ymlとまとめ

これで漸くオールグリーンになりました。疲れた。対応したコミットはRubocopによるリファクタリング · web-k/tiramisu@8eeca14で、最終的な.rubocop.ymlは以下の様になっています：

AllCops:
  TargetRubyVersion: 2.5
  Exclude:
    - bin/*
    - db/schema.rb
    - db/migrate/*
    - vendor/**/*
    - config/**/*
    - lib/tasks/*
    - Gemfile
    - features/**/*
    - spec/**/*

Rails:
  Enabled: true

Style/Documentation:
  Enabled: false

Style/FrozenStringLiteralComment:
  Enabled: false

LineLength:
  Enabled: false

Style/AsciiComments:
  Enabled: false

Style/ClassAndModuleChildren:
  Enabled: false

Style/StringLiterals:
  Enabled: false

Layout/EmptyLinesAroundClassBody:
  Enabled: false

Layout/ExtraSpacing:
  Enabled: false

Layout/IndentHash:
  Enabled: false

Layout/SpaceInsideHashLiteralBraces:
  Enabled: false

Metrics/MethodLength:
  Enabled: false

Metrics/ClassLength:
  Enabled: false

Metrics/ModuleLength:
  Enabled: false

Metrics/AbcSize:
  Max: 40

FormatString:
  EnforcedStyle: percent

NumericLiterals:
  Enabled: false

Style/BracesAroundHashParameters:
  Enabled: false

Style/AndOr:
  Enabled: false

Style/GuardClause:
  Enabled: false

Style/MethodDefParentheses:
  Enabled: false

Style/RedundantReturn:
  Enabled: false

Style/TernaryParentheses:
  Enabled: false

DotPosition:
  EnforcedStyle: trailing

Warningでもそんなに大した問題は無く、Conventionはたまに勉強になる程度で殆どが余計なお世話な状態でした。一度デフォルト$ rubocop --force-default-configで全ての警告を眺めてみて、好みによって書き方を統一する必要も無く、個別に無効化した方が幸せな様に感じました。

正方形336

正方形336

-プログラミング
-, , ,

Copyright© さかなソフトブログ , 2024 All Rights Reserved.