静的コード解析ツールの MetricFu は何を見ているのか

Rubyの静的コード解析ツールに MetricFu があります。

静的コード解析とは、ソースコードを静的に解析して、特定のパターンに合致している箇所や、違反している箇所などさまざまな視点から定量的に評価する解析手法の一つです。

実装時の早い段階からローカルやJenkins等のCIサーバで解析することにより、コードの可読性や複雑さ、コーディングスタイルの違反など、ある程度改善することが可能になり、ソフトウェアの品質向上に繋がります。

Railsプロダクトであれば、他に RuboCop(Ruby向けのコーディングスタイルチェッカー) や Brakeman(Rails向けのセキュリティスキャナ) と併せて使うのが一般的だと思います。

今回はこの MetricFu が指摘してくれる項目を紹介しようと思います。

Flog

コードの複雑度をチェックします。1メソッド毎に特定の演算子や処理がどのくらい含まれているかをチェックし、合計値がより多いメソッドほど複雑であるという判定になります。複雑なメソッドは可読性や保守性に優れないので可能であればリファクタリングしましょう。

class Test
  def blah
    a = eval "1+1"
    if a == 2 then
      puts "yay"
    end
  end
end

class Test
  def blah         # 11.2 =
    a = eval "1+1" # 1.2 + 6.0 +
    if a == 2 then # 1.2 + 1.2 + 0.4 +
      puts "yay"   # 1.2
    end
  end
end

Test#blah: (11.2)
     6.0: eval
     1.2: branch
     1.2: ==
     1.2: puts
     1.2: assignment
     0.4: lit_fixnum

Rails Best Practices

Railsのベストプラクティスに従っているかどうかをチェックします。
あくまで個人？のベストプラクティスらしいので、本当にベストプラクティスなのかを検討する必要が若干ありますが、大半は有用なものです。

サンプルアプリケーションに対する指摘の例

適当なRailsアプリケーションに対してRails Best Practicesのチェックを行うと、以下のような結果になりました。上位をピックアップして詳細を説明しようと思います。

ベストプラクティス	件数
Replace instance variable with local variable	1330
Simplify render in controllers	105
Simplify render in views	101
remove unused methods	70
Move Model Logic into the Model	49
remove trailing whitespace	41
Restrict auto-generated routes.	22
Use multipart/alternative as content_type of email	13
Overuse route customizations	9
Remove empty helpers	8
Use Observer	5
Use query attribute	4
Always add DB index	4
Move code into model	2
Don't rescue Exception, rescue StandardError	2
Replace Complex Creation with Factory Method	2
Use model association	2

指摘数1位 : Replace instance variable with local variable

Partial View でも、インスタンス変数（@variable）を使えますが、partial を含んでいる View との依存関係が強まり、他のところで使いまわしにくくなります。
インスタンス変数ではなくローカル変数として、render で呼び出す際に明示的に渡した方がコードの見渡しが良くなります。

• ダメな例

class PostsController < ApplicationController
  def show
    @post = Post.find(params[:id])
  end
end

<%= render :partial => "sidebar" %>

• ベストプラクティス

<%= render :partial => "sidebar", :locals => { :post => @post } %>

もしくは

<%= render "sidebar", :post => @post %>

指摘数2位 : Simplify render in controllers

Controller内でのrender指定時にactionのみ指定する場合は、明示的にハッシュのキーを指定しなくても
よいので記述を簡潔にできるよっていう指摘。他にも省略可能。

• Before

render :action => :edit
render :action => 'edit'

• After

render :edit
render 'edit'

別コントローラから

• Before

render :template => 'books/edit'

• After

render 'books/edit'

ファイル指定の場合

• Before

render :file => '/path/to/rails/app/views/books/edit'

• After

render '/path/to/rails/app/views/books/edit'

指摘数3位 : Simplify render in views

partialのrenderを指定する際に、:partialの部分や、optionの:localsの部分は省略可能っていう指摘。

• Before

<%= render :partial => 'sidebar' %>
<%= render :partial => 'shared/sidebar' %>

• After

<%= render 'sidebar' %>
<%= render 'shared/sidebar' %>

その他のプラクティス一覧

他にたくさんのベストプラクティスがあります。
時間があれば一通り目を通してみることをオススメします。

• Move finder to named_scope
• Use scope access
• Add model virtual attribute
• Use model callback
• model.collection_model_ids (many-to-many)
• Needless deep nesting
• Nested Model Forms
• Not use default route if you use RESTful design
• Keep Finders on Their Own Model
• Love named_scope
• DRY Controller (and debate)
• DRY Metaprogramming
• Extract into Module
• Extract to composed class
• Isolating Seed Data
• Move code into controller
• Move code into helper
• the Law of Demeter
• Use before_filter
• Fix N+1 Queries
• Use caching !
• Double-check your migrations
• Write ur own spec macros
• Writing specs for 3rd party declaratives
• Use 'Background' to consolidate common steps in a feature
• Use nested step to improve readability of ur scenario
• DRY your database.yml
• Use pickle & ur choice factory for data setup in ur cucumber features (revised)
• Use render :collection Rails 3 when possible
• abuse content_for
• Use asset_host for production server
• Use I18n.localize for date/time formating
• Use css sprite automatically
• to_s/to_s(:short)
• Use STI and polymorphic model for multiple uploads
• Use say and say_with_time in migrations to make a useful migration log
• Fetch current user in models
• use OpenStruct when advance search
• Put scripts at the bottom
• DRY bundler in capistrano
• Use batched finder for large data query
• Check if external gem-dependent classes are defined
• Prevent SQL Injections by using the ? in queries
• Generate polymorphic url
• Substituting before_filter :load_object
• Select specific fields for performance
• Use memoization
• comment your magic codes
• defer expensive job
• Annotate your models
• split route namespaces into different files
• Keep code struture in models consistent
• Namespaced models
• Create base conroller
• Active Record Query Interface Optimization
• Remove tab
• Optimize db migration
• Use cells to abstract view widgets
• Name your model methods after their behavior, not implementation.
• Not use time_ago_in_words
• Protect mass assignment
• use after_commit
• rolling out with feature flags
• split your cap tasks into different files
• Tell, don't ask
• Check the return value of "save", otherwise use "save!"
• speed up assets precompile with turbo-sprockets-rails3
• Pay more attentions on security
• monitor your backend services
• Clever enums in rails
• default_scope is evil
• Don't modify the params hash
• Use Time.zone.now instead of Time.now

Reek

コードの"不吉な匂い"をチェックしてくれます。これも可能であれば指摘された項目はリファクタリングしましょう。

サンプルアプリケーションに対する指摘の例

不吉な匂い	件数
Duplicate Method Call	884
Too Many Statements	348
Irresponsible Module	344
Uncommunicative Variable Name	197
Utility Function	154
Feature Envy	118
Nested Iterators	69
Control Parameter	68
Unused Parameters	49
Nil Check	35
Prima-Donna-Method	32
Uncommunicative Parameter Name	23
Repeated Conditional	22
Long Parameter List	19
Boolean Parameter	18
Uncommunicative Method Name	17
Data Clump	14
Too Many Instance Variables	5
Too Many Methods	4
Class Variable	2
Module Initialize	2
Uncommunicative Module Name	1

その他の不吉な匂い一覧

• Attribute (disabled by default)

Churn

バージョン管理による変更回数の多いソースは問題が多いのでは？という観点からチェックされます。必ずしも "変更回数が多い = 問題がある" わけではないですが、設計が曖昧であったり、当該ファイルの役目がきちんと定まっていない場合、変更回数は多くなりがちです。

Roodi

メソッド行数や空処理のブロック、命名規則などが一定のルールに沿っているかをチェックします。

Saikuro

分岐、行数等からコードの複雑度をチェックをします。

Cane

1行あたりの文字数や、スペーシング、クラスコメントの有無などをチェックします。

Stats

コードのクラス数、行数、メソッド数の統計およびテストコードの行数とプロダクトコード：テストコード比などをチェックします。

Flay

コードの重複部(いわゆるコピペコード)をチェックします。

Hotspots

各分析結果から、悪しきプログラムを順に表示します。ここで上位にきているファイルは見直したほうが良いでしょう。

まとめ

MetricFu は様々なツールを用いてソフトウェアのメトリクス解析を行ってくれます。ツールで被っているチェック項目などありますが、大半がソフトウェアの品質向上につながる指摘です。場合によっては基準をクリアしない場合、本番環境にデプロイさせないような運用を行うのもよいでしょう。まだ使ってないという人や、ソフトウェアの品質チェックはどういった点を確認するのか興味ある人は MetricFu をチェックしてみください。