LoginSignup
67
66

More than 5 years have passed since last update.

@jnchito(Junichi Ito)in株式会社ソニックガーデン

「テストが書きやすいコードは良いコード」をリファクタリングしてみた

Last updated at Posted at 2015-06-19

はじめに

先日 @choripon さんが「テストが書きやすいコードは良いコード」という興味深いQiita記事を投稿されていました。

団子状態になった実装を適切にメソッド分割するとコードも読みやすくなり、さらにテストも書きやすくなる、という内容については僕も同意します。

ただ「改善例として載せられているコードにもまだリファクタリングできる余地がある！」と思ってしまったので、こちらのコードをさらに改善してみようと思います。
（あつかましい記事を書いてすいません、 @choripon さん。。）

最初のコード

例として挙げられていたプログラムの仕様はざっくり言うとこんな感じです。

  • lastname を与えると、設定ファイルの内容に従って firstname を返す。
  • 設定ファイルにない名前であれば "(Unknown)" を返す。
  • "オレオレ"という lastname であれば "詐欺かも" を返す。
  • 設定ファイルが未存在であればメッセージを出力する。

まず、実装コードは以下のようになっています。

ore.rb
require 'yaml'

class Ore
  def self.firstname(lastname)
    ore = load_file 'ore.yml'
    find_ore = ore.find { |row| row['lastname'] == lastname }
    find_ore ? find_ore['firstname'] : 怪しむ(lastname)
  end

  def self.load_file(filepath)
    begin
      YAML.load_file filepath
    rescue Errno::ENOENT
      print_message filepath
    rescue
      raise
    end
  end

  def self.怪しむ(lastname)
    lastname == 'オレオレ' ? '詐欺かも' : '(Unknown)'
  end

  def self.print_message(filepath)
    puts "'#{filepath}' is not found."
  end
end

ちなみに ore.yml の中身はこのようになっています。

ore.yml
---
-
  firstname: 'Hideyuki'
  lastname: 'Matsuyama'
-
  firstname: '秀行'
  lastname: '松山'

次に、テストコードは以下のようになっています。

ore_spec.rb
describe Ore, type: :model do
  describe '.firstname' do
    subject { Ore.firstname lastname }

    context 'when lastname is "Matsuyama"' do
      let(:lastname) { 'Matsuyama' }
      it { is_expected.to eq 'Hideyuki' }
    end

    context 'when lastname is "松山"' do
      let(:lastname) { '松山' }
      it { is_expected.to eq '秀行' }
    end

    context 'when lastname is "Foo"' do
      let(:lastname) { 'Foo' }
      it { is_expected.to eq '(Unknown)' }
      it 'should receive .怪しむ' do
        expect(Ore).to receive(:怪しむ).with(lastname); subject
      end
    end
  end

  describe '.load_file' do
    let(:target_filepath) { 'ore.yml' }

    subject { Ore.load_file target_filepath }

    it { is_expected.to be_present }

    context 'when the loading filepath is nil' do
      let(:target_filepath) { nil }

      it 'should does not printing message, and raise error' do
        expect do
          expect(Ore).not_to receive(:print_message)
          subject
        end.to raise_error
      end
    end

    context 'when the loading filepath does not exist' do
      let(:target_filepath) { 'not_exist_filepath.yml' }

      it 'should printing message, and does not raise error' do
        expect do
          expect(Ore).to receive(:print_message).with(target_filepath)
          subject
        end.not_to raise_error
      end
    end
  end

  describe '.怪しむ' do
    subject { Ore.怪しむ lastname }

    context 'when lastname is "オレオレ"' do
      let(:lastname) { 'オレオレ' }
      it { is_expected.to eq '詐欺かも' }
    end

    context 'when lastname is "だれだれ"' do
      let(:lastname) { 'だれだれ' }
      it { is_expected.to eq '(Unknown)' }
    end
  end
end

さて、このコードを見たときに「僕だったらこうする」と思った点がいくつかありました。
以下でその内容を説明していきます。

「僕だったらこうするな」と思った点

1. テストは public なメソッドに対してのみ行う

テストを書く場合は原則として public なメソッドに対してのみ行います。
僕の場合、特殊な事情がない限り、 private メソッドのテストは書きません。
これは private メソッドを全くテストしないのではなく、 「private メソッドもちゃんと実行されるようなテストを public メソッドに対して行う」 ということを意味しています。

private メソッドを直接テストしない理由

private メソッドを直接テストしないのはなぜか？
それは public メソッドに比べると API が安定しないからです。
もっと端的に言うと、 リファクタリングで変更される可能性が高いから です。

API が変わったり、メソッドがなくなったりすると、テストが壊れて修正する必要が出てきます。
「壊れやすいテスト」はメンテナンスの工数を押し上げます。
なので、private メソッドは直接テストしない方が良い、という結論になります。

2. Oreクラスをオブジェクト指向で実装する

さて、上の指摘では public メソッドと private メソッドという用語が出てきましたが、そもそも Ore クラスの public メソッドと private メソッドはいったい何なのでしょうか？

・・・と思ってコードを見ても public メソッドと private メソッドの違いはわかりません。
実際は firstname メソッドが事実上の public メソッドだと思いますが、僕が最初にコードを見たときはどれが public メソッドなのかぱっとわかりませんでした。

ここはオブジェクト指向っぽくコードを実装し、public メソッドと private メソッドを明示的に分けた方がベターだと思います。

3. rescue 節の使用を極力避ける

load_file メソッドの実装は次のようになっています。

  def self.load_file(filepath)
    begin
      YAML.load_file filepath
    rescue Errno::ENOENT
      print_message filepath
    rescue
      raise
    end
  end

「ファイルが未存在であれば Errno::ENOENT を rescue してメッセージを出力する」という実装になっていますが、これは rescue を使わなくても次のようにすれば実現できます。

  def self.load_file(filepath)
    if File.exists?(filepath)
      YAML.load_file filepath
    else
      print_message filepath
    end
  end

rescue を多用するとロジックの見通しが悪くなったり、補足すべきでないエラーを補足してしまう、もしくは補足すべきエラーを補足できない、といった問題を起こします。
なので、 rescue なしで実現できるロジックは rescue を使わない方がベターです。

4. ありえないテストケースをテストしている

load_file メソッドのテストで「引数の filepath が nil の場合」をテストしています。
しかし、firstname メソッドを public メソッドとして考えるのであれば、load_file メソッドには毎回 'ore.yml' が渡されるはずです。
なので nil を想定したケースはほとんど意味がありません。

通常の運用や使い方ではあり得ないテストケースを作ると、テストの工数を増やしたり、他の開発者に「こんな呼ばれ方をすることが実際にあるんだろうか？」という無用な不安を抱かせたりする原因になるので、なくしてしまう方が望ましいです。

5. ほぼ同じテストを2回書いている

テストコードでは '(Unknown)' が返ってくることを2ヶ所で検証しています。

    context 'when lastname is "Foo"' do
      let(:lastname) { 'Foo' }
      it { is_expected.to eq '(Unknown)' }
      it 'should receive .怪しむ' do
        expect(Ore).to receive(:怪しむ).with(lastname); subject
      end
    end

    context 'when lastname is "だれだれ"' do
      let(:lastname) { 'だれだれ' }
      it { is_expected.to eq '(Unknown)' }
    end

もちろん「テストケースは絶対に重複してはいけない」というルールはありませんし、僕自身も場合によっては異なるテストで同じような検証をすることもあります。
ただ、このケースは private なメソッドまでテストしようとしたためにこのような重複が発生したと思われます。
public なメソッドだけテストするのであれば 'Foo' もしくは 'だれだれ' のどちらか一方をテストすれば十分なはずです。

「僕だったらこう書く」という実装例

さて、あんまり偉そうなことばっかり言ってると、「じゃあお前ならどう書くんだ」とツッコまれそうなのでそろそろ僕のリファクタリング例を紹介しようと思います。

まず、実装はこんな感じにしてみました。

ore.rb
require 'yaml'

class Ore
  def initialize(last_name)
    @last_name = last_name
  end

  def first_name
    return '詐欺かも' if suspicious?

    if File.exists?(file_path)
      name_table[@last_name] || '(Unknown)'
    else
      puts "'#{file_path}' is not found."
    end
  end

  private

  def suspicious?
    @last_name == 'オレオレ'
  end

  def file_path
    File.expand_path("../../config/ore.yml", __FILE__)
  end

  def name_table
    YAML.load_file(file_path).map(&:values).to_h.invert
  end
end

file_path の実装が File.expand_path("../../config/ore.yml", __FILE__) のようになっていますが、これは下図のようなディレクトリ構成になっているためです。

Screen Shot 2015-06-20 at 5.37.41.png

次に、テストコードはこんなふうに書いてみました。

ore_spec.rb
require 'spec_helper'

describe Ore do
  describe '#first_name' do
    let(:ore) { Ore.new(last_name) }
    subject { ore.first_name }

    context 'when Matsuyama' do
      let(:last_name) { 'Matsuyama' }
      it { is_expected.to eq 'Hideyuki' }
    end

    context 'when 松山' do
      let(:last_name) { '松山' }
      it { is_expected.to eq '秀行' }
    end

    context 'when Foo' do
      let(:last_name) { 'Foo' }
      it { is_expected.to eq '(Unknown)' }
    end

    context 'when file is missing' do
      let(:last_name) { '松山' }

      before do
        path = File.expand_path("../../config/foo.yml", __FILE__)
        allow(ore).to receive(:file_path).and_return(path)
      end

      it { is_expected.to be_nil }

      it 'prints message' do
        expect{ subject }.to \
          output(/foo\.yml' is not found\./).to_stdout
      end
    end
  end
end

具体的な改善点

前述の「僕だったらこうするな」と思った点はすべて改善しています。

  • public メソッドである first_name メソッドだけをテストしている
  • オブジェクト指向らしいコードにして public メソッドと private メソッドを明確に分けている
  • rescue 節を使わずに実装している
  • 「 file_path が nil の場合」は通常あり得ないのでテストしていない
  • 結果が '(Unknown)' になる場合のテストとして、 'Foo' だけをテストしている（'だれだれ' はテストしていない）

また、public メソッドだけをテストしているので内部の実装も全体的に変更しています。
これはつまり、private メソッドはいくらリファクタリングしてもテストは壊れないというメリットの表れです。

あと、ちょっと変わったところでは以下の部分で少し凝ったテストコードが登場しています。

    context 'when file is missing' do
      let(:last_name) { '松山' }

      before do
        path = File.expand_path("../../config/foo.yml", __FILE__)
        allow(ore).to receive(:file_path).and_return(path)
      end

      it { is_expected.to be_nil }

      it 'prints message' do
        expect{ subject }.to \
          output(/foo\.yml' is not found\./).to_stdout
      end
    end

allow(ore).to receive(:file_path).and_return(path) の部分はモックを使って file_path メソッドがデタラメなファイルパスを返すようにしています。
file_path メソッドがデタラメなファイルパスを返すので、設定ファイルが存在しない場合の振る舞いをテストできます。
ちなみに、 モックが埋め込みやすくなるというのも適切にメソッド分割した場合に現れるメリットのひとつ です。

また、 expect{ subject }.to output(/foo\.yml' is not found\./).to_stdout の部分はRSpec 3で登場した output マッチャを使って標準出力への出力内容を検証しています。

その他、このコードはGitHubにプッシュしてあるので、気になる方は実際に手元で動かしてみてください。

参考：コードカバレッジを確認する

リファクタリング後のテストコードでは first_name メソッドしかテストしてませんが、実装コードのカバレッジ（網羅率）は100%なのでテストとしてはこれで十分だと言えます。
（「いや、テストはカバレッジだけ見ても不十分だ！」といった議論はここではスキップさせてください）

Screen Shot 2015-06-20 at 5.58.58.png

まとめ

というわけでこの記事ではRubyのコードやテストコードを僕なりに改善する方法を紹介してみました。
「どんなコードがベストなのか」という感覚は人によって異なるので、必ずしも僕の書いたコードが一番いいというわけではありませんが、みなさんにとってどこか参考になる点があれば幸いです。

最後に、興味深い記事を書いてくださった @choripon さん、どうもありがとうございました！
（そしてリファクタリング記事のネタにしてしまって申し訳ありません。。）

あわせて読みたい

RSpecをゼロから学びたいという方はこちらの記事をどうぞ。

このテストでも登場した「モック」の使い方はこちらの記事が参考になるかもしれません。

output マッチャのようなRSpec 3から登場したマッチャについて知りたい方はこちらの記事をどうぞ。

テストコードって何をどうテストすればいいんだろう？と悩んでいる方は、僕が先日開催した勉強会の内容が参考になるかもしれません。

最後に、ネット記事ではなく技術書を読んでRSpecやテストの書き方をしっかり学びたいという方には Everyday Rails をオススメします！

67
66
6

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
67
66