はじめに
先日 @choripon さんが「テストが書きやすいコードは良いコード」という興味深いQiita記事を投稿されていました。
団子状態になった実装を適切にメソッド分割するとコードも読みやすくなり、さらにテストも書きやすくなる、という内容については僕も同意します。
ただ「改善例として載せられているコードにもまだリファクタリングできる余地がある！」と思ってしまったので、こちらのコードをさらに改善してみようと思います。
（あつかましい記事を書いてすいません、 @choripon さん。。）
最初のコード
例として挙げられていたプログラムの仕様はざっくり言うとこんな感じです。
- lastname を与えると、設定ファイルの内容に従って firstname を返す。
- 設定ファイルにない名前であれば "(Unknown)" を返す。
- "オレオレ"という lastname であれば "詐欺かも" を返す。
- 設定ファイルが未存在であればメッセージを出力する。
まず、実装コードは以下のようになっています。
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 の中身はこのようになっています。
---
-
firstname: 'Hideyuki'
lastname: 'Matsuyama'
-
firstname: '秀行'
lastname: '松山'
次に、テストコードは以下のようになっています。
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' もしくは 'だれだれ' のどちらか一方をテストすれば十分なはずです。
「僕だったらこう書く」という実装例
さて、あんまり偉そうなことばっかり言ってると、「じゃあお前ならどう書くんだ」とツッコまれそうなのでそろそろ僕のリファクタリング例を紹介しようと思います。
まず、実装はこんな感じにしてみました。
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__) のようになっていますが、これは下図のようなディレクトリ構成になっているためです。
次に、テストコードはこんなふうに書いてみました。
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%なのでテストとしてはこれで十分だと言えます。
（「いや、テストはカバレッジだけ見ても不十分だ！」といった議論はここではスキップさせてください）
まとめ
というわけでこの記事ではRubyのコードやテストコードを僕なりに改善する方法を紹介してみました。
「どんなコードがベストなのか」という感覚は人によって異なるので、必ずしも僕の書いたコードが一番いいというわけではありませんが、みなさんにとってどこか参考になる点があれば幸いです。
最後に、興味深い記事を書いてくださった @choripon さん、どうもありがとうございました！
（そしてリファクタリング記事のネタにしてしまって申し訳ありません。。）
あわせて読みたい
RSpecをゼロから学びたいという方はこちらの記事をどうぞ。
このテストでも登場した「モック」の使い方はこちらの記事が参考になるかもしれません。
output マッチャのようなRSpec 3から登場したマッチャについて知りたい方はこちらの記事をどうぞ。
テストコードって何をどうテストすればいいんだろう？と悩んでいる方は、僕が先日開催した勉強会の内容が参考になるかもしれません。
最後に、ネット記事ではなく技術書を読んでRSpecやテストの書き方をしっかり学びたいという方には Everyday Rails をオススメします！