あらすじ

吾輩は末端エンジニアである。名前はしょった。先日、急遽あるRailsアプリの引継ぎ業務に駆り出された。吾輩はここで始めて、アプリのコードを見た。しかもこのコードがまあ追いにくい。ブチギレ寸前である。

アンチパターン

というわけで本題。
先に、ここでいうアンチパターンというのは一般に流布されているそれとはまるで別物であるということは先にお伝えしておきたい。あくまで個人的なベストプラクティスであり、引き継ぐならこうなっていて欲しい(欲しかった)という願望である。

1. 1つのディレクトリ内に無駄にファイルが多い

今、ここに猫がいる。猫はかわいい。神は猫をこう定義した。

class Cat
  def comment
    puts "かわいい"
  end
end

猫は猫耳と尻尾、そして肉球でできている。
神はかわいい猫をこう定義し直した。

class Cat
  def initialize
    @parts  = [
      CatEar.new,
      CatTail.new,
      CatPad.new
    ]
  end

  def comment
    @parts.each(&:comment)
  end
end

class CatEar
  def comment
    puts "猫耳かわいい"
  end
end

class CatTail
  def comment
    puts "しっぽもかわいい"
  end
end

class CatPad
  def comment
    puts "肉球までかわいい"
  end
end

これで猫を無限に愛することができる...猫はかわいい...猫は至高....................

...

......

うるせーーーーーーーーーーーーーーーーーーーーーーーーーー！！！！！！！！！！！！
猫を愛でる前にコードを片付けやがれ！！！！！！！！！

というわけで茶番はさておき解説。
現在のディレクトリ構造を図示するとこうだ。

./
├ cat.rb
├ cat_ear.rb
├ cat_tail.rb
└ cat_pad.rb

猫クラスと猫のパーツが同居した状態で入れられている。
例えば、ここに犬が増えたらどうなるだろうか。

./
├ cat.rb
├ cat_ear.rb
├ cat_tail.rb
├ cat_pad.rb
├ dog.rb
├ dog_ear.rb
├ dog_tail.rb
└ dog_pad.rb

ファイルの数がめっちゃ増えた...
さらにここに他の動物を加えていくと...まあ地獄が生まれるのは容易に想像できる。

じゃあどうするのか。そう、パーツをCatクラス以下に作るのである。

class Cat
  def initialize
    @parts  = [
      Ear.new,
      Tail.new,
      Pad.new
    ]
  end

  def comment
    @parts.each(&:comment)
  end
end

# rails的にディレクトリ内に入ってるものはモジュールまたはクラスでネームスペースを切るのが一般的
class Cat
  class Ear
    def comment
      puts "猫耳かわいい"
    end
  end
end

class Cat
  class Tail
    def comment
      puts "しっぽもかわいい"
    end
  end
end

class Cat
  class Pad
    def comment
      puts "肉球までかわいい"
    end
  end
end

となる。
ディレクトリ構造は

./
├ cat.rb
└ cat
  ├ ear.rb
  ├ tail.rb
  └ pad.rb

これによって、それぞれのパーツはcatに関わるものとしてまとめられた。
カレントディレクトリから見ると、catの定義とcatに関わるものが入ってるのみなので、このアプリは猫に関するものだということが0秒でわかる。素晴らしい。

と、少々無茶な説明をしたが、ちょっと具体的に。
今あなたはネットバンキングのサービスを作っている。このサービスでは以下の3つのモデルがある。

• 口座の情報
• 口座の入出金履歴
• 口座の振込履歴

あなたはこれをこういう構造で実装するかもしれない。

./
├ account.rb
├ money_history.rb
└ deposited_history.rb

これでも十分、客先には喜ばれ、上司には褒められるだろう。
ただ、ここでこれを

./
├ account.rb
└ account
  ├ money_history.rb
  └ deposited_history.rb

としておくだけで、ベターになれる。きっとあなたのプロジェクトに後から入ってきたメンバーにも尊敬されるだろうし、末代まで感謝され続けるだろう。私ならこうして欲しい。こうして欲しかった。ただそれだけのことである。

2. オブジェクトのインターフェースが整っていない

rubyはオブジェクト指向言語である。オブジェクト指向は素晴らしい。オブジェクトから枝葉のように伸びるメソッドを叩くことで、やりたいことが完結できるからである。

例えるならこうだ。
ここにボルトがある。

オブジェクト指向でない場合、このボルトを締めるにはレンチが必要だ。

だがしかし、オブジェクト指向だと...

STARTボタンを押すだけレンチが勝手にボルトを締めてくれる。素晴らしい...素晴らしい......

ではこれがアンチパターンになる場合をお見せしよう。
あなたは洗濯機を作った。次のような実装である。

class WashingMachine
  def wash
    locking do
      inject_water
      ventilate
    end
  end

  def dry
    locking do 
      ventilate
    end
  end

  def locking
    puts "扉をロックします"
    yield
    puts "扉のロックを解除します"
  end

  def inject_water
    puts "注水します"
  end

  def ventilate
    puts "風を送ります"
  end
end

> WashingMachine.new.wash
扉をロックします
注水します
扉のロックを解除します

この実装の問題は、以下のようなことが容易に起こり得ることにある。

> WashingMachine.new.inject_water
注水します

いや、扉閉まってないやん！！！

まあこれは初歩的な話なので、 今更かよ...って話だけど。
オブジェクト指向で大事なことはただ一つ、インターフェースを整えることだ。インターフェースが整ってないオブジェクトは洗練されていないリモコンと同じ。使い勝手は手続き型言語より悪いだろう。
オブジェクトのインターフェースの整える指針は以下

• メソッド名、必要な引数はわかりやすくする
• 必要なメソッド以外は隠す

先ほどの実装であれば、

class WashingMachine
  def wash
    ...
  end

  def dry
    ...
  end

  private # これ以降は内部仕様なので隠す
  def locking
    ...
  end

  def inject_water
    ...
  end

  def ventilate
    ...
  end
end

こうしておけば先ほどのような操作ミスは起きないだろう。

余談だが、同じようなミスを実はRoRでもやってたりする。

ActiveRecord::Base#read_attribute(attr_name) というメソッドがある。
このメソッドは、「sqlが取ってきた[attr_name]の値をそのまま取得する」という挙動をし、任意のゲッターを作るときに利用価値がある。痒いところに手がとどくメソッドである。
ただこのメソッドは公開範囲がpublicなのが問題だ。例えば、以下のような実装ができる。

class Hoge < ApplicationRecord
  attribute :name, :string
end

<%= Hoge.first.read_attribute(:name) %>

この実装は問題なく動くだろう。
ここで、nameカラムが null または空文字だった場合、 'Unknown' と返そう、となった。

class Hoge < ApplicationRecord
  def name
    read_attribute(:name).presence || 'unknown'
  end
end

すると views/hoge.html.erb をレンダリングしたときにnameが null だった場合、出力される値はnilである。いや、viewではnameを表示してるはずなのに？？？なんで？？？ってなる。なった。は〜〜〜〜〜〜〜〜〜💢💢💢💢💢💢💢💢

read_attributeメソッドは本来であればprotectedメソッドであるべきだ。なぜなら、クラスの外から使うにはあまりに抽象的すぎるからである。また、クラス外で使うとプロジェクト内検索では見落としやすくなるだろうし、変更にも弱い。マジで使うべきではない。

ちなみに、この話は実際に弊社の尊敬する先輩がやってたコードである。先輩のことは恨んでいない。RoRのことは恨んでいる。許さん。

3. ロジックが分散している

class WashingMachine < Machine
  include DoorModule
  include ValveModule
  include AirValveModule

  def run
    locking do
      inject_water
      ventilate
    end
  end
end

このクラスを見せられたときに、送風する部分を改修して欲しいと言われたら、あなたはどこを探すだろうか？
送風(ventilate)ということは、ventilateメソッドを探せばいいのだろう。ということはventilateメソッドを定義している場所を探せばいいのだな。と、ここであなたはventilateでプロジェクト内を検索するだろう。

ここでの問題は、ventilateというメソッドがどこからきたのか、このクラスだけでは判断しにくいことだ。基本的にinclude、excludeに加えて、継承にも言えることだが、それらを経由してやってきたメソッドは往往にして所在が不明瞭になりやすい。
使わないべきとは言わないまでも、ある程度の制限を設けて、コードの可読性を損なわないように使って欲しい(願望

例えば上記の場合、モジュールを使わない代わりに以下のようにするのをオススメしたい。

class WashingMachine < Machine
  def initialize
    @door = Door.new
    @valve = Valve.new
    @air_valve = AirValve.new
  end

  def run
    @door.locking do
      @valve.inject_water
      @air_valve.ventilate
    end
  end

  # delegate :locking, to: :@door
  # などでメソッド定義しても良い。とにかくメソッドの定義がファイル内に明示されていることが重要
end

これだけで、 AirValve#ventilate を見ればいいことは一目瞭然である。勝ち申した。

本当はこれが一番書きたいところなんだけど、疲れたのでこの辺で。

おわりに

つらつらと好き勝手述べてきたが、結局言いたいのはもっとコードの可読性を上げてくれ、というところだ。
どの言語でも言えることだけど、キャメルがどうだ〜、if文の後ろの中括弧はこうだ〜みたいなどっちでもいい議論は聞くけれど、こういう構造で書いた方がいいよ！とか、こうすれば管理がしやすいよ！みたいな議論の方が大事だと思うので、もっと盛んに議論してください。よろしくおねがいします。