Rails view helperとテストのリファクタリング：モックをcode smellと感じること

## はじめに

自称、Ruby on Rails フロントエンドエンジニア:joy_cat:のnaofumiです。X [@naofumi](https://x.com/naofumi)では色々勝手なことを書いていますが、最近Qiitaも頑張り始めました。

## 先日のview helperテストのリファクタリング

先日、[Railsのviewテストを書こう](https://qiita.com/naofumik/items/9136c830ed3b8a16b4e8)って記事を書いたが、[view helperおよびそのテストのコード](https://qiita.com/naofumik/items/9136c830ed3b8a16b4e8#現状の並べ替えのステートによって並べ替え矢印の表示を変えるヘルパー)があまりイケてなかった。

そこで今日は、view helperテストのリファクタリングを一緒に考えていきたいと思う。

## 対象のコード

作ろうとしている`ordering_arrow` helperメソッドは、下記のようなテーブルの並べ替えに使う三角ボタンの箇所。

![image.png](https://qiita-image-store.s3.ap-northeast-1.amazonaws.com/0/3507700/c3bfd394-d0f1-6782-085a-b786b81a2b59.png)

これを汎用的に使えるコンポーネントにしたいので、helperメソッドに切り出している。下記のようにviewの中で使う
```erb
<%= ordering_arrow('started_at_order', :asc) do %>
  <span>▲</span>
<% end %>
```

### Helperのコードの課題

まずはHelperのコード。問題箇所は
1. `(params[param_name] == direction.to_s) || (params[param_name].blank? && default)`がわかりにくい
1. `==`とか`||`とか`&&`とか三項演算子とかがあって、見かけ以上に条件分岐が多い。これはわかりにくい

```ruby
# ヘルパー

  def ordering_arrow(param_name, direction, default: false, &block)
    selected = (params[param_name] == direction.to_s) || (params[param_name].blank? && default)

    html_options = selected ? {} : { style: 'color: #CCC;' }
    link_to({ param_name.to_sym => direction }, html_options, &block)
  end
```

### テストコードの問題

後述するようにHelperのコードの問題に起因するのだが、パッとみてこのテストコードには下記の問題がある。

1. Railsのviewにある`params`メソッドをモックしなければならなくなっている
1. Railsの`link_to`ヘルパーメソッドをモックして、どのような引数で呼び出されたかを確認することでassertionをしている

わかりにくい以前の問題として、モックをいくつも用意しないといけないこと自体がcode smell (コードの臭い）と考えて良い。基本的には避けた方が良い。

でもモックを用意しなければならないのは、元のview helperのコードがよくないからであって、テスト手法の技巧で解決できない。そこでview helperとテストをセットでリファクタリングしたいと思う。

```ruby
# テスト

RSpec.describe OrderingHelper do
  describe '#ordering_arrow' do
    before { allow(view).to receive(:link_to) }

    it 'params[:order] = nil, default true should not be grayed' do
      allow(params).to receive(:[]).with('order').and_return(nil)

      helper.ordering_arrow('order', :desc, default: true) { 'link text' }

      expect(view).to have_received(:link_to).with({ order: :desc }, {})
    end

    it 'ordering_arrow :asc, params[:order] = asc should not be grayed' do
      allow(params).to receive(:[]).with('order').and_return('asc')

      helper.ordering_arrow('order', :asc, default: true) { 'link text' }

      expect(view).to have_received(:link_to).with({ order: :asc }, {})
    end

    it 'ordering_arrow :desc, params[:order] = asc should be grayed' do
      allow(params).to receive(:[]).with('order').and_return('asc')

      helper.ordering_arrow('order', :desc, default: true) { 'link text' }

      expect(view).to have_received(:link_to).with({ order: :desc }, { style: 'color: #CCC;' })
    end
  end
end
```

## ロジックとプレゼンテーションを分離する

もう一度、view helperのコードを示す。コードの問題点をコメントで記載している。

```ruby
# ヘルパー

  def ordering_arrow(param_name, direction, default: false, &block)
    # 条件文が複雑すぎる。
    selected = (params[param_name] == direction.to_s) || (params[param_name].blank? && default)

    html_options = selected ? {} : { style: 'color: #CCC;' }
    # 複雑な条件判断とHTMLの出力を同じ関数の中に書いてしまっている
    link_to({ param_name.to_sym => direction }, html_options, &block)
  end
```

### view helperをリファクタリングした結果

下記は複雑な条件分岐とHTMLの出力を分けるようにview helperをリファクタリングしたものである。

1. 複雑な条件分岐のロジックは`ordering_arrow_selected?`に集約した
1. `ordering_arrow` helperメソッドの中にあるのは１つのif-else文だけで、これだったらテストを書くまでもなく、パッと見で何をやっているかがわかる
1. なお`ordering_arrow_selected?`は`private`としているが、Moduleをincludeするときにprivate宣言するのはほぼ意味がない。今回はコメント代わりに`private`と書くことで、「`ordering_arrow_selected?`は外から使わないでね！」って警告している。(そもそもRubyの`private`はドキュメンテーションの代わり程度だという話もある）

```ruby
  def ordering_arrow(param_name, direction, default: false, &block)
    html_options = if ordering_arrow_selected?(params[param_name], direction, default)
                     {}
                   else
                     { style: 'color: #CCC;' }
                   end

    link_to({ param_name.to_sym => direction }, html_options, &block)
  end

  private

  def ordering_arrow_selected?(param_value, direction, default)
    (param_value == direction.to_s) || (param_value.blank? && default)
  end
```

## テストの書き方にどういうインパクトがあるか？

上記のように view helperをリファクタリングすると、複雑な条件分岐をモックなしでテストできるようになる。

### テストのリファクタリング

view helperをリファクタリングしたおかげで、下記のようなテストが書けるようになった。
1. `#ordering_arrow_selected?`はモックを全く必要としないので、非常に簡単にテストが書ける
1. 結果として、より網羅的なテストを書く意欲も湧いた（リファクタリングついでにテストを追加した）
1. `#ordering_arrow`の中のロジックは随分と減ったので、本当はテストもいらないぐらいになった。一応、今回は１つだけ残した

なお条件分岐のすべてのケースを網羅しようと思うと面倒になるので、「ここまで書いておけば十分だろう」って留めていることもわかっていただけるかと思う。

テストの目的はあくまでも正しく動くコードを書くためなので、そこにさえ到達できれば、100%のテストカバレッジを目指すのは割と不毛だと私は思っているし、一般にそう言われている。

```ruby
RSpec.describe OrderingHelper do
  describe "#ordering_arrow_selected?" do
    it 'should return true if params_value is nil and default is true' do
      # privateなメソッドを呼び出すのは`send`を使うだけで良い
      expect(helper.send(:ordering_arrow_selected?, nil, "asc", true)).to be_truthy
    end

    it 'should return false if params_value is nil and default is false' do
      expect(helper.send(:ordering_arrow_selected?, nil, "asc", false)).to be_falsey
    end

    it 'should return true if params_value matches direction and default is false' do
      expect(helper.send(:ordering_arrow_selected?, "asc", "asc", false)).to be_truthy
    end

    it 'should return false if params_value does not match direction and default is true' do
      expect(helper.send(:ordering_arrow_selected?, "desc", "asc", true)).to be_falsey
    end
  end

  describe '#ordering_arrow' do
    before { allow(view).to receive(:link_to) }

    it 'ordering_arrow :desc, params[:order] = asc should be grayed' do
      allow(params).to receive(:[]).with('order').and_return('asc')

      helper.ordering_arrow('order', :desc, default: true) { 'link text' }

      expect(view).to have_received(:link_to).with({ order: :desc }, { style: 'color: #CCC;' })
    end
  end
end
```

## 結論

今回は下記のことを実施した

1. テストコードにモックが２つも入っているのを見て、「これはcode smellだ」って判断した。要は、テストの書きにくさをきっかけに、リファクタリングの必要性を痛感した形になっている
1. モックが２つも必要だったのは、テスト対象のview helperメソッドの中で、複雑な条件分岐とHTML出力が同居しているためと考え、そこを分離した
1. テストの方では、複雑な条件分岐とHTML出力を別々にテストした。おかげで特に条件分岐のテストがわかりやすくなった

このように、テストが書きにくいのは、しばしば元のコードに原因がある。テストを先に書いたわけではないけど、テスト駆動開発(設計)の思想に則り、テストしやすさを指標としてコードを改善できた。

viewをテスト駆動開発でやるのはなかなか難しく、私もテストを先に書くことは基本的にしない。でもcode smellに敏感になることで、設計の改善というメリットは結果として享受できると考えている。

皆さんもテストを書くのがしんどいと感じることは多いと思う。その時、設計をどう改善するとテストが書きやすくなるかを考えて、実際にそのようにリファクタリングすることをお勧めする！

今回はこれまで！

また何か取り上げて欲しい話題があれば、コメントに残すか、Xでお知らせください！