ソースコードの減らし方 - 基本的な考え方と10個の方法 - クラウドワークスエンジニアブログ

ステップ数で評価が決まる現場では全く役に立たないテクニックではありますが、ソースコードの減らし方について紹介したいと思います。

開発Div. エンジニアのayasudaです。 2014年の夏にジョインし、会社名と同じサービス、クラウドワークスの開発に携わっています。

ご覧の通り、消したソースコードの方が多いので、ステップ数換算だとマイナスの働きしかしてませんね！

f:id:a_yasuda:20160310155555p:plain

本記事では、特に Ruby on Rails の運用されているプロダクトコードにおける、ソースコードの減らし方について紹介していこうと思います。

基本的な考え方

ソースコードを減らすときの大原則は「ボーイスカウト・ルール - プログラマが知るべき97のこと」です。

普段、ソースコードを触るときに、一つでも良いので簡単な改善を入れる。これを積み重ねるのが大事です。一度に一気に直そうとするのはあまり良くありません。大抵の場合、デグレーションを引き起こします。

一見すると、これではちっともソースコードの量は減らなさそうに見えますが、積み重ねると結構削減できるものですよ！

方法1. 3回目のコピペをやめる

ソースのコピー＆ペースト自体は、まぁ、許すとして、 3 回目に同じ処理を書くときはメソッドなどに切り出しましょう。

コピペは良くないと習っていても、新しい機能を実装する時に既存のコードからコピー＆ペーストをすることは数多くあります。例えば、「xxxなユーザーが記事を参照できるか？」のような細かな判定メソッドや、 Rails のコントローラにおけるインスタンスの初期化メソッドなど、細かくて単純であるほど、ついついコピペで済ませてしまいがちです。

サンプルコードで示してみましょう。すでに FoosController には #bar, #baz という 2 つのアクションが実装されており、それぞれ @foo というインスタンス変数を初期化しています。アクセスしているユーザーによってちょっとだけ表示条件が違ったりしているというような処理は、現実世界のソースコードでも良く見るかと想います。

このコントローラに新しく #qux というアクションが必要で、かつ同じように @foo が必要にな時、急いでいる時は特に、ついついコピペで済ませがちになるかと思います。

class FoosController < ApplicationController
  def bar
    if current_user.admin?
      @foo = Foo.find(params[:id])
    else
      @foo = Foo.where(id: params[:id], published: true).last
    end

    # なんか他の処理
  end

  def baz
    if current_user.admin?
      @foo = Foo.find(params[:id])
    else
      @foo = Foo.where(id: params[:id], published: true).last
    end

    # なんか他の処理
  end

  def qux
    # ここでも @foo の初期化メソッドが必要な時、ついつい上の方からコピペしがち
  end
end

小規模なプロダクトでは特に困ることはありませんが、 コントローラの行数が1000を超えていたりするときに、たまたま条件の追加をしたくなったりするとき にちょっとだけ面倒です。ですので、 3 回目のコピペを行いかけた、今、メソッドに切り出しておきましょう。

class FoosController < ApplicationController
  before_filter :set_foo

  def bar
    # なんか他の処理
  end

  def baz
    # なんか他の処理
  end

  def qux
    # なんか他の処理
  end

  private

  def set_foo
    if current_user.admin?
      @foo = Foo.find(params[:id])
    else
      @foo = Foo.where(id: params[:id], published: true).last
    end
  end
end

方法2. 共通する一部を切り出す

今、同じバリデーションやメソッドを別々のクラスに実装するのであれば、モジュールやクラスに切り出してみましょう。

特に Rails の RAILS_ROOT/app/models ディレクトリは、ファイル数の面でもファイルサイズの面でも肥大化することがとてもよくあります。これは、プロダクトが大きく成長した証拠でもありますので、悲観する必要はありません。ただ、ビジネス的には嬉しい反面、"技術的投資" が "技術的債務" になりかかっているのでエンジニア的には嬉しくありません。

とはいえ、何が共通的なモジュールやクラス、バリデーションクラスになるかはよくわかりません。そこで、これから新しく同じようなメソッドを定義したりするときに、モジュール化やクラスに切り出しにチャレンジするようにしてみましょう。

サンプルとして、様々なものをブックマーク可能なサービスで、新しく記事をブックマーク可能にする修正を考えてみます。

class User < ActiveRecord::Base
  has_many :bookmarks
end

class Bookmark < ActiveRecord::Base
  belongs_to :user
  belongs_to :bookmarkable, polymorphic: true
end

class Article < ActiveRecord::Base
  has_many :bookmarks, as: :bookmarkable

  def bookmarked_by?(user)
    bookmarks.where(user_id: user.id).count > 0
  end

  def bookmarkable?(user)
    !bookmarked_by?(user) && user.active?
  end
end

class Post < ActiveRecord::Base
  has_many :bookmarks, as: :bookmarkable

  def bookmarked_by?(user)
    bookmarks.where(user_id: user.id).count > 0
  end

  def bookmarkable?(user)
    !bookmarked_by?(user) && user.active?
  end
end

class Story < ActiveRecord::Base
  # 新しく Story もブックマーク可能にしたい
end

このケースの場合、関連の定義と２つのインスタンスメソッドが切り出せそうです。まだ３つしか同じコードが存在しないので、あまり困ることはありませんが、 似たような関連とメソッドを持つクラスが 15を超えている状態で、機能追加するときなど には面倒なことになります。ですので、新しく同じようなメソッドを別クラスに実装するときなどにはモジュールへの切り出しなどを検討しましょう。

class User
  has_many :bookmarks
end

class Bookmark < ActiveRecord::Base
  belongs_to :user
  belongs_to :bookmarkable, polymorphic: true
end

module Bookmarkable
  extend ActiveSupport::Concern

  included do
    has_many :bookmarks, as: :bookmarkable
  end

  def bookmarked_by?(user)
    bookmarks.where(user_id: user.id).count > 0
  end

  def bookmarkable?(user)
    !bookmarked_by?(user) && user.active?
  end
end

class Article < ActiveRecord::Base
  include Bookmarkable
end

class Post < ActiveRecord::Base
  include Bookmarkable
end

class Story < ActiveRecord::Base
  include Bookmarkable
end

一般に小さなクラスの方がメンテナンスがしやすいので、 Validator クラスを定義したり、RAILS_ROOT/app/models/concerns におけるモジュールの定義などを、切り出したほうがベターです。新規機能の実装時などに検討してみてはいかがでしょうか？

方法3. 未使用な何かの削除

使われていない気がするローカル変数・定数・インスタンス変数・メソッド・ファイルなどを見かけたら、削除にチャレンジしてみましょう。

2年くらい変更され続けているファイルやクラス、プロダクトなどでは、ごくたまに使われていない気がするファイルやクラス、メソッド、定数や変数を見かけることがあります。ローカル変数などは削除がそこまで難しくありませんが、メソッドやクラスの削除などは注意が必要です。削除する前には grep などを使って参照場所を確認しておくとは思いますが、ごくたまに下記のように動的に参照のされ方をするケースなどもあり、一筋縄では行きません。

  class_name = params[:class].constantize
  object = class_name.send(:find, params[:id])

おすすめなのは、「将来削除する旨を記したコメント」と、参照されたらログを吐いておくという手法です。

例えばメソッドを削除する際には、ActiveSupport.alias_method_chain を使い、廃止予定のメソッドが呼ばれたらログを記録しておくことにします。

def foo_with_deprecated_warning
  log.warn("DEPRECATED 2016/4/1 くらいに廃止予定のメソッドが呼ばれました。")
  foo_without_deprecated_warning
end

alias_method_chain :foo, :deprecated_warning

ロジックの変更や切り出し作業に比べ、メソッドやクラスの廃止は成果に対して労力がかなり大きかったりもします。しかし、不必要なコードは時間がたてばたつほど削除の難易度があがりますので、なるべく早めに削除するようにしたほうがベターです。

方法4. 必要の無くなったコメントの削除

不必要になったコメントは削除しましょう。

主に２種類の不必要なコメントがあります。ひとつは昔に書かれて今は意味のないものとなってしまったもので、１年以上経っている TODO コメントや、実装と矛盾しているコメントなどがそれにあたります。

イメージとしては以下のようなコメントです。

# TODO: '2015-10-02 00:00:00' 以降はここを削除。
if Time.current < '2015-10-02 00:00:00' 
  # なんかのキャンペーンとか
end

def foo(user)
  do_somthing
  do_otherthing
  # user.active? が真の時は 0 を返す
  if user.active? && user.any_other_condition?
    BAR
  else
    BAZ
  end
end

今は意味の無くなってしまったコメントを削除する最初の第一歩は git blame です。コメントを最初に書いた同僚にヒアリングをし、コメントの修正か削除をしてあげましょう。

実装と矛盾しているコメントが現れた場合は注意が必要です。どこかの格言によれば、コメントとコードが矛盾しているときは、たいていの場合両方ともバグっています。初めてコメントが書かれていた時の commit を慎重に読んで設計意図などを汲み取った上で、その後のコードの変化を精査していく必要があります。

もう１つ不必要なコメントはジェネレーターで作成された設定ファイルやGemfileなどにデフォルトで表示されているコメントです。

フレームワークやライブラリの作者が用意した有意義な情報がとてもよく記されてはいます。しかし、数年動かしているプロダクトにおいてはどちらかというと設定ファイルの全体的な把握しやすさのほうが重要です。

例えば、Gemfile に下記のようなコメントが書かれているならば、もはやコメントは不要でしょう。

# Use Capistrano for deployment
# gem 'capistrano-rails', group: :development

# その他の gem 定義が 5-60行続く

group :development do
  gem 'capistrano-rails'
end

Gemfile や設定ファイルは、だいたい100行を超え始めたらコメントの整理などが必要だと思います。

方法5. ネストの見直し

ネストを減らしましょう。

if やループ、ブロックなどでネストが増えてしまうのは別に良いのですが、インデントが 6 レベルとかになってきたらどこかがおかしいです。

例をお見せしましょう。恐らく、初期は単純明快だったはずにもかかわらず、改修を重ねるにつれ複雑になったことが予想されるコードです。

def foo
  if @user.guest?
    @post.guest_flag = true
    unless @post.save(validate: false)
      return render :new_post
    end
  else
    if @post.new_record?
      if @post.save
        if @user.admin?
          flash[:notice] = I18n.t('posts.created.by_admin')
          return redirect_to admin_posts_path(@post)
        else
          flash[:notice] = I18n.t('posts.created.by_user')
        end
      else
        return render :new_post
      end
    else
      if @user.admin?
        @post.user = User.common_admin
      end
      raise AccessForbidden if @post.editable_by?(@user)
      if @post.save
        if @user.admin?
          flash[:notice] = I18n.t('posts.edited.by_admin')
          return redirect_to admin_posts_path(@post)
        else
          if @user == @post.user
            flash[:notice] = I18n.t('posts.edited.by_self')
          else
            flash[:notice] = I18n.t('posts.edited.by_other')
            return redirect_to user_posts_path
          end
        end
      else
         return render :new_post
      end
    end
  end

  redirect_to post_path(@post)
end

このようなコードは、大抵の場合はほっておいても複雑度が下がることはありません。また、テストも厄介で、 C0 カバレッジはまだしも C1, C2 カバレッジを 100% にするのは困難です。

少なくとも、一気に改修するのはかなり困難なので、お勧めするのは「マージ担当者が一目で安全と分かるほど Light な Refactoring」=「Lifactoring (拙作の造語)」です。

レビューワーのために、バカバカしくも作業をひとつづつ commit し、ゆっくりとテストに包んでいくことをお勧めいたします。

例えば、

「非ゲストでの編集時の処理をブロックの外へ移動」

def foo
+ if !@user.guest? && !@post.new_record?
+   @post.user = User.common_admin if @user.admin?
+   raise AccessForbidden if @post.editable_by?(@user)
+ end
  if @user.guest?
    @post.guest_flag = true
...
      end
    else
-     if @user.admin?
-       @post.user = User.common_admin
-     end
-     raise AccessForbidden if @post.editable_by?(@user)
      if @post.save
        if @user.admin?

「ゲストアクセス時のフラグたてをブロックの外へ移動」

def foo  
+ if @user.guest?
+   @post.guest_flag = true
+ elsif @post.new_record?
- if !@user.guest? && @post.new_record?
    @post.user = User.common_admin if @user.admin?
    raise AccessForbidden if @post.editable_by?(@user)
  end
  if @user.guest?
-   @post.guest_flag = true
    unless @post.save(validate: false)
      return render :new_post

「新規投稿と編集時の場合分けを一時変数に保存」

def foo
+ context = @post.new_record? ? :created : :edited
  if @user.guest?
    @post.guest_flag = true
+ elsif context == :edited
- elsif @post.new_record?
    @post.user = User.common_admin if @user.admin?
    raise AccessForbidden if @post.editable_by?(@user)
...
    end
  else
+   if context == :created
-   if @post.new_record?
      if @post.save
        if @user.admin?

「保存時の判定を例外に変更」

    end
  else
+   @post.save!
    if context == :created
-     if @post.save
        if @user.admin?
          flash[:notice] = I18n.t('posts.created.by_admin')
...
          flash[:notice] = I18n.t('posts.created.by_user')
        end
-     else
-       return render :new_post
-     end
    else
-     if @post.save
        if @user.admin?
          flash[:notice] = I18n.t('posts.edited.by_admin')
...          end
        end
-     else
-        return render :new_post
-     end
    end
  end

  redirect_to post_path(@post)
+rescue AccessForbidden => e
+ raise e
+rescue
+ return render :new_post
end

「ゲストでの保存時の成否判定も例外処理に変更」

...
  end
  
  if @user.guest?
+   @post.save!(validate: false)
-   unless @post.save(validate: false)
-     return render :new_post
-   end
  else
    @post.save!
+ end
+
+ unless @user.guest?
    if context == :created
      if @user.admin?
...

「非ゲストで管理者でのアクセス時のリダイレクトをブロックの外へ移動」

...
      if @user.admin?
        flash[:notice] = I18n.t('posts.created.by_admin')
-       return redirect_to admin_posts_path(@post)
      else
        flash[:notice] = I18n.t('posts.created.by_user')
...
      if @user.admin?
        flash[:notice] = I18n.t('posts.edited.by_admin')
-       return redirect_to admin_posts_path(@post)
      else
        if @user == @post.user
...
  end

+ if !@user.guest? && @user.admin?
+   redirect_to admin_posts_path(@post)
+ else
  redirect_to post_path(@post)
+ end
rescue AccessForbidden => e
  raise e
rescue
  render :new_post
end

「非ゲストで、自分の投稿を編集した時のリダイレクトをブロックの外側へ移動」

...
        else
          flash[:notice] = I18n.t('posts.edited.by_other')
-         return redirect_to user_posts_path
        end
     end
  end

   if !@user.guest? && @user.admin?
    redirect_to admin_posts_path(@post)
+ elsif !@user.guest? && (context == :edited) && @user == @post.user
+   redirect_to user_posts_path 
  else
    redirect_to post_path(@post)
  end
rescue AccessForbidden => e
...

「flash メッセージの組み立てを少しだけ動的に変更」

...
  unless @user.guest?
+   whom = if @user.admin?
+     :admin
+   elsif context == :created
+     :user
+   elsif @user == @post.user
+     :self
+   else
+     :other
+   end
+   flash[:notice] = I18n.t("posts.#{context}.by_#{whom}")

-   if context == :created
-     if @user.admin?
-       flash[:notice] = I18n.t('posts.created.by_admin')
-     else
-       flash[:notice] = I18n.t('posts.created.by_user')
-     end
-   else
-     if @user.admin?
-       flash[:notice] = I18n.t('posts.edited.by_admin')
-     else
-       if @user == @post.user
-         flash[:notice] = I18n.t('posts.edited.by_self')
-       else
-         flash[:notice] = I18n.t('posts.edited.by_other')
-       end
-    end
  end
...

「保存時のパラメタをより明示的に」

+ @post.save!(validate: !@user.guest?)
- if @user.guest?
-   @post.save!(validate: false)
- else
-   @post.save!
- end

「編集時のアクセス権判定を before_filter に移動」

+before_filter :guard_editable

def diff
  context = @post.new_record? ? :created : :edited

  if @user.guest?
    @post.guest_flag = true
+ elsif @user.admin? && context == :edited
- elsif context == :edited
-   @post.user = User.common_admin if @user.admin?
-  raise AccessForbidden if @post.editable_by?(@user)
    @post.user = User.common_admin
  end
...
    redirect_to post_path(@post)
  end
-rescue AccessForbidden => e
- raise e
rescue
  render :new_post
end

+private

+def guard_editable
+  raise AccessForbidden unless (@post.persisted? && @post.editable_by?(@user))
+end

などのような小さな変更を少しずつリリースしていくことで、最終的にはちょっとだけマシかもしれないコードになります。

テストが書きづらい分だけ、レビューのしやすさや元に戻しやすさに力をさきつつ、毎日少しずつリリースすることがお勧めです。

before_filter :guard_editable

def foo
  context = @post.new_record? ? :created : :edited

  # アクセス者によるデータの修正
  if @user.guest?
    @post.guest_flag = true
  elsif @user.admin? && context == :edited
    @post.user = User.common_admin
  end

  @post.save!(validate: !@user.guest?)

  # flash メッセージの組み立て(この処理だけ関数化してもいいかもしれません)
  unless @user.guest?
    whom = if @user.admin?
      :admin
    elsif context == :created
      :user
    elsif @user == @post.user
      :self
    else
      :other
    end
    flash[:notice] = I18n.t("posts.#{context}.by_#{whom}")
  end

  # リダイレクト先の分岐
  if !@user.guest? && @user.admin?
    redirect_to admin_posts_path(@post)
  elsif !@user.guest? && (context == :edited) && @user == @post.user
    redirect_to user_posts_path
  else
    redirect_to post_path(@post)
  end
rescue
  render :new_post
end

private

def guard_editable
  raise AccessForbidden unless (@post.persisted? && @post.editable_by?(@user))
end

方法6. イディオムの発見

同じことを別の方法でやっている場合は、まずは同じような書き方にするだけでも効果があります。

長く運用しているコードベースでは、最終的に同じことをしているけれど違う書き方をしているコードがたまにあります。

地味な例では下記のようなコードです。

class User
  has_many :posts
end

def foo
  @user # User インスタンス
  Post.build(user :@user)
end

def bar
  @user # User インスタンス
  @user.posts.build
end

def baz
  @user # User インスタンス
  post = Post.new
  post.user = @user
  post
end

同じことをするのに、特に理由なく複数通りの書き方をしているのであれば、書き方を揃えたほうが、将来の仕様変更に耐えやすくなります。また、 "greppability" も上がるでしょう。

方法7. パーシャルに切り出す

同じような HTML ブロックはパーシャルに切り出せないか検討してみましょう。

この方法はあまりサンプルをお見せすることができません。バラエティが豊富なので、簡単な例をすぐに考えることができないためです。

パーシャルは地味にレンダリングコストが高く、また、切り出したい HTML ブロックが JavaScript や CSS と密接に結びついている可能性が高いので一筋縄では行きません。

しかし、その分うまく切り出せた場合にはかなりの技術的債務の解消が期待できます。

WebComponents が使用可能であれば、チャレンジしてみるのも良いかもしれません。

方法8. ベストプラクティスにならう

Ruby on Rails や Ruby のベストプラクティスに倣っていないところを直していきましょう。

Ruby の静的解析ツールは幾つかあります。有名どころですと、「RuboCop (コーディング規約のチェックツール)」「rails_best_practices (Rails に特化したコードの臭い発見ツール)」「Reek (Ruby 向けのコードの臭い発見ツール)」あたりでしょうか。

また、SaaS としては「CODE CLIMATE」や「SideCI」などもあります。

運用されているコードベースに対して静的解析を当てると、たいていの場合大量の警告がなされることになります。

これらの警告を一気に直すのはリスキーですし、なによりも途中で心が折れかねません。

繰り返しになりますが、お勧めするのは「ボーイスカウトルール」です。

自分がこれから触るコードなどでなるべく警告を解消し、少なくとも新しくコミットする際に警告を増やさないように心がければ、長いスパンを経て全ての警告が解消される日が来るかもしれません。

方法9. spec を見直す

テストコードも定期的に見直したほうが良くなります。

テストコードはリファクタリングされる機会があまりなく、さらにすぐにコードベースが増えてしまいがちです。また、コードベースが増えることで実行時間も肥大化してしまうという副作用もあります。

RSpec は 3.3 以降、aggregate_failures が実装され、よりテストが書きやすくなりました。

例えば、下記のようなテストケースで見てみましょう。

RSpec.describe FooController do
  describe "#create" do
    subject { post :create, params }
    context '正しいパラメタでアクセス' do
      let(:params) { #略 }

      it "Fooが作成される" do
        expect{ subject }.to change{ Foo.count }.by(1)
      end

      it "関連される Bar も作成される" do
        expect{ subject }.to change{ Bar.count }.by(1)
      end

      it "メールが送信される" do
        expect{ subject }.to change{ ActionMailer::Base.deliveries.size }.by(1)
      end

      describe "作成されるオブジェクト" do
        before { subject }

        it "Foo のステータスが設定される" do
          expect(Foo.last.status).to eq Foo::BAR
        end

        it "Foo の barable は false" do
          expect(Foo.last.barable).to be_falsey
        end

        it "Bar の compoleted_at は nil" do
          expect(Bar.last.completed_at).to be_nil
        end

        it "メールのタイトルと Bar のタイトルが一致する" do
          expect(ActionMailer::Base.deliveries.last.subject).to eq(Bar.last.title)
        end
      end
    end
  end
end

7 個のテストケースが定義されており、post :create, params は 7 回実行されることになります。このケースで aggregate_failures を使用すると、テストケースの見通しも良くなり、また、 post :create, params の実行回数も 1 度になります。

RSpec.describe FooController do
  describe "#create" do
    subject { post :create, params }
    context '正しいパラメタでアクセス' do
      let(:params) { #略 }

      it "Foo 関連する Bar が作成され、メールが送信される" do
        aggregate_failures do
          expect { subject }.to \
            change{ Foo.count }.by(1) &
            change{ Bar.count }.by(1) &
            change{ ActionMailer::Base.deliveries.size }.by(1)

          foo = Foo.last
          bar = Bar.last
          mail = ActionMailer::Base.deliveries.last

          expect(foo.status).to eq Foo::BAR
          expect(foo.barable).to be_falsey
          expect(bar.completed_at).to be_nil
          expect(mail.subject).to eq bar.title
        end
      end
    end
  end
end

方法10. 使わなくなったツールの削除

使われなくなった設定ファイルや、各個人が使用する gem とその設定は、チームの合意が得られるならプロジェクトの管理下からは排除したほうが良いかもしれません。

Ruby には便利な gem がたくさんあります。 pry 上でのドキュメント参照ができるようになる pry-doc やブレークポイントを仕込める pry-byebug 、 ActiveRecord の中身が見やすくなる hirb 、 tap すると print もしてくれる tapp 、プリントデバッグで使いやすい awesome_print などです。

これらの gem は確かに便利ですし、関連する設定のベストプラクティスは確かに有用です。しかし、著者としてはプロジェクト内で共有すべきではないかなと思います。

個人の .pryrc もありますし・・・

また、過去にプロジェクトで使用していてそのままになっている Guard の設定ファイルなど、使用していないならば削除するのが良いでしょう。

Ruby やモダンな JavaScript の界隈では、ツールの移り変わりもかなり激しいのが昨今のトレンドです。

新しいツールを導入し、チーム内でデファクトスタンダードになったら、リポジトリの管理下から外してみるのはどうでしょうか？