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

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

# 全コード共通
### rescueを多用しない

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

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

### ちゃんとprivateを使う

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

```ruby: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宣言はしましょう。

```ruby:washing_machine.rb
# GOOD
class WashingMachine
  def wash
    ...
  end

  def dry
    ...
  end

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

  def inject_water
    ...
  end
  ...
```

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

例えば以下。

```ruby:hoge_controller.rb
# BAD
class HogeController < ApplicationController
  def hoge
    @hoge = Hoge.new
    @data = @hoge.data
    @result = @hoge.result
  end
  ...
```

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

```erb:views/hoge/hoge.html.erb
<%= @hoge %>
<%= @data %>
<%= @result %>
```

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

```ruby:hoge_controller.rb
# GOOD
class HogeController < ApplicationController
  def hoge
    @hoge = Hoge.new
  end
  ...
```

```erb:views/hoge/hoge.html.erb
<%= @hoge %>
<%= @hoge.data %>
<%= @hoge.result %>
```

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

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

例えば以下。

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

```erb:views/hoge/hoge.html.erb
<%= @main_hoge %>
```

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

```ruby: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が知ってないといけないのは面倒すぎる。
よって以下のように書くと好ましい。

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

```erb:views/hoge/hoge.html.erb
<%= main_hoge %>
```

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

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

例えば以下。

```ruby:hoge_controller.rb
# BAD
class HogeController < ApplicationController
  def new
    @hoge = Hoge.new
    @hoge.fuga = 'fuga'
    @hoge.piyo = 'piyo'
  end
  ...
```

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

```ruby:hoge_coontroller.rb
# GOOD
class HogeController < ApplicationController
  def new
    @hoge = Hoge.new
    @hoge.set_deafult
  end
  ...
```

```ruby: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/[ ]/[ ]=を避ける

例えば以下。

```ruby: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&nbsp; / #[]&nbsp;&nbsp; ... recordの値をtypecastしたものを取得
- #write_attribute / #[]= ... 値をtypecastしたものをrecordに書き込み

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

```ruby: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人はそんなこと知らずに書いていると思う。そういう意味で大変紛らわしいし危険なコードである。
よって以下のように書くのが望ましい。

```ruby:hoge_controller.rb
class HogeController < ApplicationController 
  def hoge
    @hoge = Hoge.new
    
    # read_attribute/write_attributeはmodel外では使わない
    fuga = @hoge.fuga
    @hoge.fuga = :fuga
  end
```

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

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


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

例えば以下。

```ruby: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つの処理を実行するなどのような場合。
このような場合、ロジックとして大きいため、分割したほうがいい場合がある。例えば以下。

```ruby:hoge
# GOOD
class Hoge
  def fetch_fuga
    FetchFuga.run
  end
  ...
```

```ruby: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パターン](https://www.techscore.com/tech/DesignPattern/Command.html/)ということで紹介する。

### ロジックが探しにくくならないようにする
例えば以下。

```ruby: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しているが故に、探さなければならない場所が多すぎるのだ。そして最終手段・全検索に走る。
一度や二度ならいいがそうなんどもやってるとコードを書くスピードも下がる。士気が下がる。呪いのコードである。

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

```ruby: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` 内であることが圧倒的確証を持って明言できる。コーディングも捗るだろう。

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

例えば以下。

```ruby:hoge.rb
class Hoge < ApplicationModel
  attr_reader :fuga
  ...
```

```ruby: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に入れた方がいい。
以下のように書くのが望ましい。


```ruby: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
  ...
```

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

  ...
```

あるいは [extending](https://apidock.com/rails/ActiveRecord/QueryMethods/extending) などを用いて、自動で展開されるようにしてもいいかもしれない

# view編

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

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

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

超大事。
例えば以下。

```erb: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回してるのとか見かけるけど論外である。
以下のようにするのが望ましい。

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

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

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

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

# js編

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

例えば以下。

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

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

```js: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内のセレクタは全部、全てのエレメントから検索しており、パフォーマンスの低下にも繋がるので避けるべきである。
以下のようにするのが望ましい。

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

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

```js: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` に依存するだろ、と言う場合は、

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

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

# おわり

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