私的アンリーダブルコード―他人を発狂させるための 9 のテクニック

コードはたいてい一度しか書かれませんが、何度も何人も読むことになります。
普段何気なく書いているコードが他人の時間と精神を削っているかもしれません。

そんなわけで、個人的に辛いなと思うことを 9 つ挙げてみました。共感してもらえるものもいくつかあるんじゃないかと思います。

静的型付き言語を使うことで解消される問題もありますが、その選択肢はひとまずなしということで。
Ruby 前提になっていますが、他の言語にも言えることも多いと思います。

実体にそぐわない変数名

例えば Vehicle というクラスが定義されているプログラムで以下のコードのようなコードがあった場合、vehicles にはどんなオブジェクトが入っていると思いますか？

vehicles = fetch_vehicles_for(user)

続きのコードを読んでみた時に次のようになっていたらどうでしょう？予想通りだったでしょうか？？

vehicles = fetch_vehicles_for(user)
Vehicle.where(id: vehicles)  # vechicles は ID の配列！？！？

僕は最初のコードを見たら vehicles には Vehicle インスタンスの配列が入っていることを期待します。よって、こう書いてほしいと思うわけです。

vehicle_ids = fetch_vehicle_ids_for(user)
Vehicle.where(id: vehicle_ids)

もちろんスコープが狭ければ適当な変数名でも良いと思います。省略形の方が可読性の高くなるケースもあるでしょう。
ただ、このケースでいえば、_id というたった 3 文字を削ることによるメリットが全く感じられません。

見分けの付かない配列とハッシュの変数名

先ほどの例と似たようなコードで次のように書いてあったらどうでしょう？

vehicles = fetch_vehicles_for(users)
Vehicle.where(id: vehicles.values.flatten)  # !?!?

vehicles には一体何が入ってるんだ？？と思いませんか？変数名が複数形だから配列かと思いきや、ハッシュのようですね。しかも flatten をしているから、各要素の値は配列？

個人的には次のように書いてあるとすんなり読めます。

vehicle_ids_by_user_id = fetch_vehicle_ids_by_user_id_for(users)
Vehicle.where(id: vehicle_ids_by_user_id.values.flatten)

この命名規則は以前所属していたチームで使われていたもので、value_by_key, key_to_value のような命名規則にすることで、ハッシュ（マップ）だということがすぐにわかるし、キーや値が何であるか即座にわかります。
cf. language agnostic - What’s a good naming convention for a lookup map/hash? - Stack Overflow

# キーが user_id、値が vehicle の ID の配列
vehicle_ids_by_user_id
user_id_to_vehicle_ids

# キーが user_id、値が vehicle の ID
vehicle_id_by_user_id
user_id_to_vehicle_id

ダサいなとも思うんですが、可読性の高さという面で得られるメリットの方が遥かに上回っていると思います。

vehicles.values.flatten
vehicle_ids_by_user_id.values.flatten

どちらの方がコードの意図を汲み取りやすいですか？前者は vehicles の代入箇所まで遡らないと意図通りのコードか判断できないと思います。代入箇所でメソッド呼び出しがあれば、そのメソッドのコードまで確認することになります。
もちろん、「実体にそぐわない変数名」が使われていない前提です。

呼び出し元で true/false を指定するだけの引数

次の true は何を表しているでしょう？

render_vehicle(true)

正解は「プレーンテキストで表示する」です。わかるか！

def render_vehicle(plain)
  if plain
    puts vehicle.name
  else
    puts "<div class='vehicle'>#{vehicle.name}</div>"
  end
end

これは次のように書き直した方が格段に可読性が高くなるでしょう。

def render_vehicle(plain:)
  if plain
    puts vehicle.name
  else
    puts "<div class='vehicle'>#{vehicle.name}</div>"
  end
end

render_vehicle(plain: true)

あるいはこうとか。

def render_vehicle(format:)
  case format
  when :plain
    puts vehicle.name
  when :html
    puts "<div class='vehicle'>#{vehicle.name}</div>"
  else
    raise ArgumentError, "Unknown format: #{format}"
  end
end

render_vehicle(format: :plain)

cf. hall of api shame: boolean trap · ariya.io

暗黙の実行順序

次のコードは say_hello の後にしか age が呼ばれないという暗黙のルールが存在します。

class Person
  AGE_BY_NAME = {
    'Taro' => 20,
  }.freeze

  def initialize(name)
    @name = name
  end

  def say_hello
    @age = AGE_BY_NAME[@name]
    puts "Hello, my name is #{@name} and #{@age} old."
  end

  def age
    @age
  end
end

「知るかっ！」って感じですよね。もちろん、あるメソッドを呼ぶ前にはある操作をしなければならない、ということもあるでしょう。例えば、機械学習の分類器で、predict を呼ぶ前に train を呼んで学習しないといけないとか。

上記のようなコードは次のように書き換えることで暗黙のルールをなくすことができます。

class Person
  AGE_BY_NAME = {
    'Taro' => 20,
  }.freeze

  def initialize(name)
    @name = name
  end

  def say_hello
    puts "Hello, my name is #{@name} and #{age} old."
  end

  def age
    @age ||= AGE_BY_NAME[@name]
  end
end

[] メソッドの定義・Array の継承

Ruby の場合、[] が使われていれば Array か Hash のインスタンスだと思うのが普通の感覚でしょう。ところが、Array にも Hash にもないメソッドが使われていたらどうでしょう？

first_element = my_list[0]
my_list.render  # !?!?

render って何だ？という感じですよね。実は my_list は MyList という独自クラスなのでした。

class MyList
  def initialize(elements)
    @elements = elements
  end

  def [](index)
    @elements[index]
  end

  def render
    puts @elements.join(', ')
  end
end

僕は思うわけです。「それ、[] 定義する必要ある？」
もし @elements を外部から操作されないようにする配慮であれば、次のように定義した方がずっと混乱がないんじゃないかと。

class MyList
  def initialize(elements)
    # 場合によっては deep copy する必要もある（activesupport には deep_dup がある）
    @elements = elements.dup.freeze
  end

  def elements
    @elements
  end

  def render
    puts @elements.join(', ')
  end
end

先ほどのコードは次のように書き換わります

first_element = my_list.elements[0]
my_list.render

Array を継承するクラスを導入するのも同様のことが言えます。もちろん継承した方が良いケースもあるでしょうが、多くの場合は配列の要素を返すメソッドを定義したり、Ruby の場合は Enumerable を include したりすることで事足りるでしょう。

ハッシュの乱用

例えば以下のような場合にハッシュを用いると辛いです。

def build_recommended_news
  news = {}
  news[:title] = 'This is a recommended news for you'
  news[:content] = 'blah blah blah...'
  news
end

def build_popular_blog_post
  blog_post = {}
  blog_post[:title] = 'This is a popular blog post'
  blog_post[:content] = 'blah blah blah...'
  blog_post
end

何故か？
2 つのメソッドの返り値の関連性がわからないからです。たまたま同じ形式のハッシュを返しているのか、意図的に揃えているのか。
酷い場合には、返り値のハッシュに対して新しい要素を追加することもあるでしょう。
そのうち次のような微妙に形式の異なるハッシュを返すコードと、それを吸収するために分岐の入ったコードも出てくるかもしれません。

def build_technical_report
  report = {}
  report[:title] = 'This is a technical report'
  report[:body] = 'blah blah blah...'
  report
end

def render_article(article)
  puts article[:title]
  if article.has_key?(:content)
    puts article[:content]
  else
    puts article[:body]
  end
  puts "#{total_size(article)}文字"
end

def total_size(article)
  size = article[:title].size
  if article.has_key?(:content)
    size += article[:content].size
  else
    size += article[:body].size
  end
end

スコープが広い場合はハッシュではなくクラスを定義することで I/F が保証できて良いです。安心感が全然違います。適切にインスタンスメソッドも定義できます。

def build_recommended_news
  news = {}
  news[:title] = 'This is a recommended news for you'
  news[:content] = 'blah blah blah...'
  Article.new(title: news[:title], content: news[:content])
end

def build_popular_blog_post
  blog_post = {}
  blog_post[:title] = 'This is a popular blog post'
  blog_post[:content] = 'blah blah blah...'
  Article.new(title: blog_post[:title], content: blog_post[:content])
end

def build_technical_report
  report = {}
  report[:title] = 'This is a technical report'
  report[:body] = 'blah blah blah...'
  Article.new(title: report[:title], content: report[:body])
end

def render_article(article)
  puts article.title
  puts article.content
  puts "#{article.total_size}文字"  # `def total_size; title.size + content.size; end`
end

密結合した mixin

Ruby では module を定義し、クラスで include すれば mixin が実現できます。
これを乱用すると辛いことになります。例えば以下のような感じです。

module MessageDecoratable
  def decorate(msg)
    <<~"MSG"
      #{build_rule}
      * #{msg} *
      #{build_rule}
    MSG
  end
end

module MessageBuildable
  def build
    msg = ('a'..'z').to_a.shuffle.first(10).join('')
    decorate(msg)
  end

  def build_rule
    '-----------------'
  end

  def build_date
    Time.now.strftime('%F')
  end
end

module MessagePresentable
  def show
    puts to_s
    puts build_date
  end
end

class Message
  include MessageDecoratable
  include MessageBuildable
  include MessagePresentable

  def to_s
    @message ||= build
  end
end

Message.new.show

MessageDecoratable は MessageBuildable に依存し、
MessageBuildable は MessageDecoratable に依存し、
MessagePresentable は MessageBuildable と Message に依存するという実装です。

何が辛いかというと

  • Message で全ての module が include されるという暗黙の前提
  • 依存関係のわかりにくさ
  • MessageDecoratable と MessageBuildable の相互依存
  • よくわからない役割分担
  • 不必要な mixin 化
  • どの module からも全てのメソッドが呼べてしまうカオスっぷり
  • 各 module のテスト困難さ

絵に描いたようなスパゲッティコードです。
これであれば mixin ではなく 1 つのクラスに詰め込んだ方が潔いです。

同じような感じで実装するのであれば、例えば以下のようにした方がずっと良いでしょう。

class MessageDecorater
  def decorate(msg)
    <<~"MSG"
      -----------------
      * #{msg} *
      -----------------
    MSG
  end
end

class MessageBuilder
  def initialize(decorator)
    @decorator = decorator
  end

  def build
    msg = ('a'..'z').to_a.shuffle.first(10).join('')
    @decorator.decorate(msg)
  end
end

class MessagePresenter
  def show(msg)
    puts msg
    puts build_date
  end

  def build_date
    Time.now.strftime('%F')
  end
end

class Message
  def to_s
    @message ||= MessageBuilder.new(MessageDecorater.new).build
  end

  def show
    MessagePresenter.new.show(to_s)
  end
end

Message.new.show

先ほどの実装と比べて次の点で良いでしょう

  • MessageDecorater, MessageBuilder, MessagePresenter の依存関係がなくなり、テストしやすくなった
  • Message と各コンポーネントの依存関係の見通しが良くなった

mixin をカジュアルに使うとスパゲッティコードができあがるので気を付けましょう。もちろんプレーンなクラスでも密結合したスパゲッティコードを作ることはできますが、mixin ほどではないと思います。

過剰な nil guard

nil guard という言葉は Ruby でしか使われてない気がしますが、要は値が nil (null) だった場合にエラーにならないように配慮する処理のことです。
例えば以下のような処理です。

# nil.to_s は空文字列になる（nil.split はエラーになる）
words = sentence.to_s.split(' ')
# nil.to_i は 0 になる（nil > 0 はエラーになる）
value.to_i > 0
# value が nil だと DEFAULT_VALUE を返す
value || DEFAULT_VALUE
# try! はレシーバが nil なら nil を返し、そうでなければ指定されたメソッドを呼ぶ
# activesupport のメソッド
user.try!(:name)

nil guard すれば次のような処理もエラーにならない！素晴らしい！！

user_by_id = User.where(id: user_ids).index_by(&:id)
user_ids.each do |user_id|
  user = user_by_id.try!(:[], user_id)
  puts "こんにちは#{user.try!(:name)}さん"
end

ここで考えてほしいんですよね

  • そもそも nil guard してよいのか？（意図的に例外を投げるべき箇所では？）
  • そもそも nil になるのか？

例えば上記の例、index_by(&:id) が nil になることはまずないです。よって user_by_id.try!(:[], user_id) で try! を使う必要はないでしょう。
そして user が nil だった場合、「こんにちはさん」と表示されるのは明らかにダメでしょう。nil が許容できないのであれば RuntimeError を投げ、許容できるのであれば「こんにちは名無しさん」と表示するなど、処理を変えるべきでしょう。

よく笑い話で次のようなコードがネタにされますが、過剰に nil guard するのも大差ないです。

a = 1
a = 1  # 念のためもう一度代入　

個人的には nil guard するためだけの to_s, to_i も nil を空文字列や 0 にしたいのか、シンボルを文字列にしたり、文字列を整数にしたりしたいのか、意図が不明確になるので好きではないですが、まぁ許容範囲じゃないでしょうか。

条件によって異なる返り値の型

次のコードは何が問題でしょう？

def users(user_ids)
  return [] if user_ids.empty?
  User.where(id: user_ids)
end

user_ids が空配列の場合は Array インスタンス、そうでない場合は ActiveRecord::Relation インスタンスが返ります。この違いによってハマることもあるでしょう。
よって、次のように書くのが望ましいです。

def users(user_ids)
  return User.none if user_ids.empty?
  User.where(id: user_ids)
end

あるいは

def users(user_ids)
  return [] if user_ids.empty?
  User.where(id: user_ids).to_a
end

のように一貫して配列を返すようにするとかでしょうか。

推薦図書

ベタですが、「リーダブルコード」を読んだことがなければ絶対に読むべきだと思います。分量も少ないし、難しいことも書かれていないのでスラスラ読めます。

Amazonで詳細を見る

「デザインパターン何それ？」な人は「Java言語で学ぶデザインパターン入門」がオススメです。全てのパターンを覚える必要はないでしょうが、Adapter パターン、Template Method パターン、Strategy パターン辺りはよく使うんじゃないかと思います。また、色んなパターンを学ぶことでどのようなことに気を付けるべきかわかってくると思います。

Amazonで詳細を見る