理解されやすいコードの書き方

コードレビューしていて、結構ヤバイな...ってなるコード多いので、やばそうなポイントをまとめました。
一応Railsを用いて説明していますが、web系だとほかの言語でも当てはまると思います。
ざっくりいうと「CUIを整えよう」です。ぜひ気をつけてください。

全コード共通

rescueを多用しない

一番大事。ほかの言語だとtry~catchですね。

hoge.rb

# BAD
def hoge
  raise 'hogehoge'
  rescue => e
  # エラーの握りつぶしは臭いものへの蓋と同じです。絶対やめてください。
end

ちゃんとprivateを使う

初歩的だが、例えば以下。

washing_machine.rb

# BAD
class WashingMachine
  def wash
    locking do
      inject_water # puts 注水します
      rolling      # puts 洗濯槽を回します
      eject_water  # puts 排水します
      ventilate    # puts 乾燥します
    end
  end

  def dry
    locking do 
      ventilate
    end
  end

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

  def inject_water
    puts "注水します"
  end

  ...
end

適当に洗濯機モデルを作った。実用的な例が浮かばなかったから許してほしい。
このモデルの推奨の使い方はこう。

> WashingMachine.new.wash
扉をロックします
注水します
洗濯槽を回します
排水します
乾燥します
扉のロックを解除します

だが、例えばこのモデルをこういう呼び出し方もできる。

> WashingMachine.new.inject_water
注水します

扉のロックなしに注水を始めている。あたり一面水浸しになるかもしれない。クレームもんである。
「安易に使ってはいけないメソッド」の意味も込めて、しっかりprivate宣言はしましょう。

washing_machine.rb

# GOOD
class WashingMachine
  def wash
    ...
  end

  def dry
    ...
  end

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

  def inject_water
    ...
  end
  ...

controller編

controllerでインスタンス変数を多用しない

例えば以下。

hoge_controller.rb

# BAD
class HogeController < ApplicationController
  def hoge
    @hoge = Hoge.new
    @data = @hoge.data
    @result = @hoge.result
  end
  ...

インスタンス変数を使っているviewに値を共有する例である。
この例はview側から見ると

views/hoge/hoge.html.erb

<%= @hoge %>
<%= @data %>
<%= @result %>

となる。
@hoge はいいとして、 @data や @result はデータや結果であることはわかるが、なんの結果なのか分からない。また、controllerには複数のactionが記述されるため、他のactionで同じ変数が使われる可能性もある。使われた場合、それらが同じ @data なのか、異なる @data なのか見分けるのは難しい。
よって以下のように書くのが好まれる。

hoge_controller.rb

# GOOD
class HogeController < ApplicationController
  def hoge
    @hoge = Hoge.new
  end
  ...

views/hoge/hoge.html.erb

<%= @hoge %>
<%= @hoge.data %>
<%= @hoge.result %>

これであれば、 @hoge には Hoge のインスタンスが入ってそうだし、view側から見ると @hoge.data は @hoge のデータであることが直感的に理解できる。

actionメソッド以外ではインスタンス変数で値の共有を行わない

例えば以下。

hoge_controller.rb

# BAD
class HogeController < ApplicationController
  before_action :set_main_hoge
  def set_main_hoge
    @main_hoge = Hoge.first
  end
  ...

views/hoge/hoge.html.erb

<%= @main_hoge %>

複数のviewをまたいで共通の値をセットする場合によく見る。
この例は例えば

hoge_controller.rb

# BAD
class HogeController < ApplicationController
  before_action :set_main_hoge
  def index
    @main_hoge = Fuga.first
  end

  def set_main_hoge
    @main_hoge = Hoge.first
  end
  ...

とされると挙動がおかしくなる。なにかのはずみでこういうコードを書かれた場合、発見がしづらいし、こういうコードに怯えながらコードを書きたくはない。あと普通にviewで @main_hoge が必要かどうかをcontrollerが知ってないといけないのは面倒すぎる。
よって以下のように書くと好ましい。

hoge_controller.rb

# GOOD
class HogeController < ApplicationController
  helper_method :main_hoge
  def main_hoge
    @main_hoge ||= Hoge.first
  end
  ...

views/hoge/hoge.html.erb

<%= main_hoge %>

これだと main_hoge にある定義が全てだし、@main_hoge が改変される心配も少ない。改変された場合はよっぽど悪意があるか名前が悪いかの二択である。これでよく使われるのは current_user とかだろう。もっとたくさん使っていい。

modelに書けるロジックはmodelに書く

例えば以下。

hoge_controller.rb

# BAD
class HogeController < ApplicationController
  def new
    @hoge = Hoge.new
    @hoge.fuga = 'fuga'
    @hoge.piyo = 'piyo'
  end
  ...

Hoge の初期値をセットする、などの場合である。
Railsの場合、開発が進んでくると色々なcontrollerであるいはactionで似たようなことを行うことになるが、そのたびにこれらを呼び出すのはいささか面倒である。
以下のように書くのが望ましい。

hoge_coontroller.rb

# GOOD
class HogeController < ApplicationController
  def new
    @hoge = Hoge.new
    @hoge.set_deafult
  end
  ...

Hoge.rb

# GOOD
class Hoge < ApplicationModel
  def set_default
    self.fuga = 'fuga'
    self.piyo = 'piyo'
  end
  ...

そもそもcontrollerは使い回しに向いていない。controller以外に居所がないロジックは仕方がないが、それ以外はcontrollerから切り離してmodelに入れるよう心がけるべきである。
要は「ファットコントローラーは避けましょう。」ということだ。

余談 ActiveRecord::Base#read_attribute/write_attribute/[ ]/[ ]=を避ける

例えば以下。

hoge_controller.rb

class HogeController < ApplicationController 
  def hoge
    @hoge = Hoge.new

    fuga =  @hoge.read_attribute(:fuga)
    @hoge.write_attribute(:fuga, :fuga)
    fuga = @hoge[:fuga]  # read_attribute(:fuga) と等価
    @hoge[:fuga] = :fuga # write_attribute(:fuga) と等価
  end

これらの正確な挙動は

• #read_attribute  / #[]   ... recordの値をtypecastしたものを取得
• #write_attribute / #[]= ... 値をtypecastしたものをrecordに書き込み

である。
ここで大事なのが、modelの値を取得/書き込み、とは異なる。例えば

hoge.rb

class Hoge < ApplicationRecord
  def fuga
    super.to_s # read_attribute(:fuga).to_s と等価
  end
  ...

とされた場合に、 Hoge#fuga と Hoge#read_attribute(:fuga) の結果は異なる。
正直思うんだが、このコードを見た人の100人に99人くらいはそんな認識持たないと思うし、このコードを書いた人の100人に99人はそんなこと知らずに書いていると思う。そういう意味で大変紛らわしいし危険なコードである。
よって以下のように書くのが望ましい。

hoge_controller.rb

class HogeController < ApplicationController 
  def hoge
    @hoge = Hoge.new

    # read_attribute/write_attributeはmodel外では使わない
    fuga = @hoge.fuga
    @hoge.fuga = :fuga
  end

hoge.rb

class Hoge < ApplicationRecord
  def fuga
    super.to_s # read_attribute(:fuga).to_s と等価
  end

  # どうしても生の値が必要なら専用のメソッドを用意する
  def fuga_raw
    read_attribute(:fuga)
  end
  ...

model編

大きすぎるロジックは適度に分割する

例えば以下。

hoge

# BAD
class Hoge
  def fetch_fuga
    ready_fetch_fuga
    start_fetch_fuga
    finalize_fetch_fuga
  end

  def ready_fetch_fuga
    # そこそこに重めの処理
  end

  def start_fetch_fuga
    # そこそこに重めのsy(ry
  end

  def finalize_fetch_fuga
    # そこそk(ry
  end

fetch_fuga を実行するためには3つの処理を実行するなどのような場合。
このような場合、ロジックとして大きいため、分割したほうがいい場合がある。例えば以下。

hoge

# GOOD
class Hoge
  def fetch_fuga
    FetchFuga.run
  end
  ...

hoge/fetch_fuga.rb

# GOOD
class Hoge
  class FetchFuga
    class << self
      def run(hoge)
        new(hoge).tap(&:run)
      end
    end

    def initialize(hoge)
      @hoge = hoge
    end

    def run
      ready
      start
      finalize
    end
    ...

railsだとServiceパターンって名前で浸透してたりもするが、正直Commandパターンの焼き直しだし腹たつので、普通にCommandパターンということで紹介する。

ロジックが探しにくくならないようにする

例えば以下。

washing_machine.rb

# BAD
class WashingMachine < Machine
  include DoorModule
  include ValveModule
  include AirValveModule

  def run
    locking do
      inject_water
      ventilate
    end
  end
end

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

このクラスはventilateが探しにくい。もしかしたらAirValveModuleの中に入っているかもしれないし、ValveModuleの中に入っているかもしれないし、DoorModuleの中に入っているかもしれないし、Machineクラスの中に入っているかもしれない。
なんでもかんでもincludeしているが故に、探さなければならない場所が多すぎるのだ。そして最終手段・全検索に走る。
一度や二度ならいいがそうなんどもやってるとコードを書くスピードも下がる。士気が下がる。呪いのコードである。

まあ場合にはよるのだが、以下のように書くのがおすすめである。

waching_machine.rb

# GOOD
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

これで、 ventilate を定義しているのは Valve 内であることが圧倒的確証を持って明言できる。コーディングも捗るだろう。

余談 複数のインスタンスにまたがる処理の場合

例えば以下。

hoge.rb

class Hoge < ApplicationModel
  attr_reader :fuga
  ...

hoge_controller.rb

# BAD
class HogeController < ApplicationController
  def index
    @hoges = Hoge.all
    fugas = fetch_fugas(@hoges.pluck(:id))
    @hoges.zip(fugas) { |hoge, fuga| hoge.fuga = fuga }
  end

  def fetch_fugas(ids)
    # hogeのidの配列を渡すと対応するfugaの配列を返す。例えばAPI経由で取得してくる。
  end
  ...

この場合、controllerに処理を書いてしまう人をよく見かけるが、これもmodelに入れた方がいい。
以下のように書くのが望ましい。

hoge.rb

# GOOD
class Hoge < ApplicationModel
  attr_reader :fuga

  class << self
    def set_fugas(hoges)
      fugas = fetch_fugas(hoges.pluck(:id))
      hoges.zip(fugas) { |hoge, fuga| hoge.fuga = fuga }
    end

    def self.fetch_fugas(ids)
      # hogeのidの配列を渡すと対応するfugaの配列を返す。例えばAPI経由で取得してくる。
    end
  end
  ...

hoge_controller.rb

# GOOD
class HogeController < ApplicationController
  def index
    @hoges = Hoge.all.tap(&:set_fugas)
  end

  ...

あるいは extending などを用いて、自動で展開されるようにしてもいいかもしれない

view編

パーツごとに分けて作る。パーツ間は改行多めにする。

htmlは関数がない分コードのメリハリがつきにくい。例えばヘッダーとメインとフッター、あるいはコンテンツの説明と中身など、それぞれの項目に意味付けしてパーツとみなし、そのパーツ間は改行を少し多めに入れておくと胃もたれしない。コードを見る人の心と身体に優しくなる。

難しいjsをhtml上に書かない

超大事。
例えば以下。

hoge.html.erb

<!-- BAD -->
<div class='hoge'>
  ここをクリック
</div>

<script>
  var hoge = <%= j(@hoge.data.to_json) %>;
  $('.hoge').click(function() {
    // クリックされたらajax通信して結果を表示するとか、結構分量のあるやつ。
  })
</script>

erbの性質上、コードの行数が変わりやすいので、これだとデバッグが大変になる。たまにscript内でerbのeach回してるのとか見かけるけど論外である。
以下のようにするのが望ましい。

hoge.html.erb

<!-- GOOD -->
<div class='hoge' data-data='<%= j(@hoge.data.to_json) %>'>
  ここをクリック
</div>

<script>
  setupHoge($('.hoge'));
</script>

hoge.js

// GOOD
function setupHoge(el) {
  const hoge = $(el).data('data');
  $('.hoge').click(function() {
    // クリックされたらajax通信して結果を表示するとか、結構分量のあるやつ。
  })
}

こうすればデバッグがしやすい。あと個人的にデータを渡すときはscriptに直接渡すのではなくて、datasetで渡す方がそのエレメントに関係するデータだってことがわかるのでいいと思う。

js編

パーツなどの初期化関数は通常、第一引数にElementを指定する
セレクタの検索を何度も行わない

例えば以下。

hoge.html

<!-- BAD -->
<div class='hoge'>
  <div class='fuga'>
    <span class='hoge-fuga-text'>
      ほげふが
    </span>
  </div>
</div>

<script>
  setupHoge(); // この定義から見て、setupHogeがどのElementに依存するかがわからない。
</script>

hoge.js

// BAD
function setupHoge() {
  // 毎回Elementを全検索している。かなりパフォーマンスが悪い。
  $('.hoge').click(function() { $('.hoge-fuga-text').text('ほげ'); });
  $('.fuga').click(function() { $('.hoge-fuga-text').text('ふが'); });
}

こうすると、script上のsetupHogeからは、この関数がどのElementに依存するものなのかがわからない。この依存というのは非常に大事で、関数スコープみたいなものと考えれば良い。この例は、globalスコープを使うのと同じようなものだ。変更に弱く非常に危険である。
あとついでに言うと、hoge.js内のセレクタは全部、全てのエレメントから検索しており、パフォーマンスの低下にも繋がるので避けるべきである。
以下のようにするのが望ましい。

hoge.html

<!-- GOOD -->
<div class='hoge'>
  <div class='fuga'>
    <span class='hoge-fuga-text'>
      ほげふが
    </span>
  </div>
</div>

<script>
  setupHoge($('.hoge')); // $('.hoge')に依存していることがわかりやすい。
</script>

hoge.js

// GOOD
function setupHoge(el) {
  // hogeをrootとして、hoge以下のものを使う。もしくはdocument,window,bodyなどを使う。
  // 何度もSelectorの検索を行わない。
  const $hoge = $(el);
  const $fuga = $hoge.find('.fuga');
  const $hogeFugaText = $hoge.find('.hoge-fuga-text');
  $hoge.click(function() { $hogeFugaText.text('ほげ'); });
  $fuga.click(function() { $hogeFugaText.text('ふが'); });
}

こうすると setupHoge が $('.hoge') に依存してそうなことは直感的にわかる。
もちろん setupHoge の中で .hoge 以下のエレメント、または document, window, bodyなどのglobalなElement以外は使わないことも留意しなければならない。じゃないと無意味だからね。
setupHoge だから普通 .hoge に依存するだろ、と言う場合は、

// GOOD
function setupHoge(el=$('.hoge')[0]) {
  ...

までなら許されると思う。それ以上はコードの中身を読まなきゃいけないので許されない。

おわり

レビュワーの人と引き継ぎの人に優しいコードづくりを心がけましょう。