今回はライフゲームTDDその4、その1、その2と、その3で誕生の実装ができました。基本的な枠組みはできているので、完成までは間近です。
しかしあらためてコードを眺めてみると、特にテストコードのほうがゴチャゴチャして読みにくくなっています。プロダクトコードにも、リファクタリングの余地があるかもしれません。TODOリストにも「テストをわかりやすく修正する」とあるので、先に進む前に直してしまいたいと思います。
テストコードを眺めて、まず気になるのがテストのフィクスチャを作成するところ、すなわちテストに必要な準備をするところで、特にテスト用のデータとしてCellをいくつか作っている箇所が、似たような処理をコピー＆ペーストで書いているのがイヤな感じです。
- なにをしてるのか読み取りにくい
- 大事な情報が、雑多なコードに埋もれてしまって、把握できない
- コピー＆ペーストのため後で直すのが大変
とりたてて難しいところはなさそうなので、ひとつずつ退治していきたいと思います。
まず、以下のような部分に着目します。
life_test.py
def test_will_born_true(self):
cell = Cell()
cell.neighbours = [Cell() for i in range(8)]
cell.neighbours[0].live()
cell.neighbours[1].live()
cell.neighbours[2].live()
self.assertTrue(cell.will_born())Cellオブジェクトを複数組み立てて、cell.neighboursに設定しています。似たようなコードがいま3箇所登場するので、メソッドを抽出して共通化します。テストによって異なるのは、「いくつ生きた状態にするか(live()を呼ぶか)」だけなので、その情報を強調できるようにしたいところです。今の状態は、パラメータっぽい情報が「8」「0, 1, 2(配列のインデックス)」「live()呼び出し」などありますが、このテストのコンテキストで大事なのはどの点なのかわかりません。上の部分は、以下のように書き換えました。
def test_will_born_true(self):
cell = Cell()
fill_neighbours(cell, alive=3)
self.assertTrue(cell.will_born())fill_neighbours()がフィクスチャ準備のファンクションになります。サポートとかヘルパとも呼びます。パラメータはcellと、生きているセルの数(alive)です。名前付きパラメータを使っていることもあり、これなら「生きている数が重要」だということがわかります。fill_neighbours()の実装はシンプルです。
def fill_neighbours(cell, alive):
cell.neighbours = [Cell() for i in range(8)]
for i in range(alive):
cell.neighbours[i].live()fill_neighbours()を使ってあと2つのテストも書き換えました。
さて別の箇所に着目します。以下のテストはいかにも読みにくく理解しにくくなっています。
def test_build_cells(self):
initial = '''
oo.
o..
...
'''
cells = GameOfLife.build_cells(initial)
self.assertEqual(cells[(0, 0)].is_alive(), True)
self.assertEqual(cells[(1, 0)].is_alive(), True)
self.assertEqual(cells[(2, 0)].is_alive(), False)
self.assertEqual(cells[(0, 1)].is_alive(), True)
self.assertEqual(cells[(1, 1)].is_alive(), False)
self.assertEqual(cells[(2, 1)].is_alive(), False)
self.assertEqual(cells[(0, 2)].is_alive(), False)
self.assertEqual(cells[(1, 2)].is_alive(), False)
self.assertEqual(cells[(2, 2)].is_alive(), False)文字列をパースした処理が正しく動作するかテストしていますが、このような確認をしているのはここ1箇所です。1箇所だけではありますが、やはり読みにくいのはよくない。なんとか工夫してみます。テストとして必要な情報は、座標(cellsのキー)とそのセルが生きているかどうかです。これならもうちょっと見やすく書けそうです。
def test_build_cells(self):
initial = '''
oo.
o..
...
'''
cells = GameOfLife.build_cells(initial)
expected = {
(0, 0): True, (1, 0): True, (2, 0): False,
(0, 1): True, (1, 1): False, (2, 1): False,
(0, 2): False, (1, 2): False, (2, 2): False,
}
actual = { pos:cells[pos].is_alive() for pos in cells.keys() }
self.assertEqual(actual, expected)expectedを連想配列にして、キーが座標でセルの生死をTrue/Falseで表現しています。セルの配置に合わせてコードも書いているので、元よりは把握しやすくなりました。またcellsを同じ形式に変換してactualとして、直接assertEqual()で比較しています。書き換えた余録として、以前は不要なキーが紛れ込んでいても検出できませんでしたが、今は余分なキーがあっても失敗になります。それに、失敗したときも比較的わかりやすい表示をしてくれます。1箇所わざと間違えててテスト実行すると、以下のように表示され、どこが間違っているかすぐわかります。
.......F....
======================================================================
FAIL: test_build_cells (__main__.GameOfLifeTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "life_test.py", line 103, in test_build_cells
self.assertEqual(actual, expected)
AssertionError: {(0, 1): True, (1, 2): False, (0, 0): True, (2, 1): False, (1, 1): False, (2, 0) [truncated]... != {(0, 1): True, (2, 0): False, (0, 0): True, (2, 2): False, (1, 0): True, (1, 2): [truncated]...
{(0, 0): True,
(0, 1): True,
- (0, 2): False,
? ^^^^
+ (0, 2): True,
? ^^^
(1, 0): True,
(1, 1): False,
(1, 2): False,
(2, 0): False,
(2, 1): False,
(2, 2): False}
----------------------------------------------------------------------なお、actualに変換してassertEqual()する部分をメソッドとして抽出すると良さそうですが、今回はこのままにしておきます。アサートする部分を共通化してしまうと、どのテストのどの行で失敗したかわかりにくくなってしまいます。失敗した理由も、共通メソッドの中まで読まないと把握できなくなりがちです。またテストケースを一読しただけで、どこで何をアサートしているか不明です。
アサートの部分を共通化するなら、assertXxxの形式を踏襲したメソッドを書きます。失敗したら自前でAssertExceptionを投げ、メッセージの内容もわかりやすくします。またPythonならPyHamcrest、JavaならJUnit4などで使えるassert_that(actual, matcher)の形式なら、カスタムマッチャを実装すればいいので、もっと容易になります。いずれにせよ、アサートを含む処理を共通化するときは気をつけましょう。
今の変更と似たようなことを、test_dump_cells()にも適用できそうです。
def test_dump_cells(self):
cells = {
(0, 0): Cell(), (1, 0): Cell(), (2, 0): Cell(),
(0, 1): Cell(), (1, 1): Cell(), (2, 1): Cell(),
(0, 2): Cell(), (1, 2): Cell(), (2, 2): Cell(),
}
cells[(0, 0)].live()
cells[(0, 2)].live()
cells[(2, 1)].live()
expected = '''
o..
..o
o..
'''
actual = GameOfLife.dump_cells(cells)
self.assertEqual(actual, pattern(expected))ここでは、連想配列を作ったものをdump_cells()に渡して、文字列表現に変換できるかテストしています。連想配列を作ってから一部のセルを生きた状態にしている(live())ので、見通しが悪いです。与えているデータがわかりやすくなるよう、書き換えます。
def test_dump_cells(self):
cell_states = {
(0, 0): True, (1, 0): False, (2, 0): False,
(0, 1): False, (1, 1): False, (2, 1): True,
(0, 2): True, (1, 2): False, (2, 2): False,
}
cells = { pos:Cell(state=cell_states[pos]) for pos in cell_states.keys() }
expected = '''
o..
..o
o..
'''
actual = GameOfLife.dump_cells(cells)
self.assertEqual(actual, pattern(expected))もう一点、些細な問題なのですが気になるのがこういう箇所です。
def test_set_initial_pattern(self):
initial = '''
oo.
o..
...
'''
game = GameOfLife(pattern=initial)
expected = '''
oo.
o..
...
'''
actual = game.dump()
self.assertEqual(actual, expected)Pythonで複数行文字列リテラルを書く形式なので、仕方ないのですが、どうにも見た目が悪い。簡単なトランスレータで行頭の空白を吸収するようにしておきましょう。リファクタリング後は上のテストメソッドが以下のようになりました。まあマシにはなりましたね。
def test_set_initial_pattern(self):
initial = '''
oo.
o..
...
'''
game = GameOfLife(pattern=pattern(initial))
expected = '''
oo.
o..
...
'''
actual = game.dump()
self.assertEqual(actual, pattern(expected))追加したサポートファンクションはこちらです。
def pattern(pattern_with_spaces):
return '\n'.join([line.strip() for line in pattern_with_spaces.split('\n')])Pythonっぽく書いてますが、ようは1行ずつ前後の空白(改行も)を除去して、あらためて改行を挟んで連結しています。
ほかにリファクタリングする余地はないかなと見渡すと、CellTestで毎回Cellを新規作成しています。これはsetUp行きですね。ただしsetUp()でselfに持たせると、Pythonでは毎回self.cellと書かないといけなくなるので、見た目が気になる場合もあります。
ながながとリファクタリングしてきました。リファクタリングは、どこまでもやりすぎてしまうこともあるので、注意が必要です。個人的なルールとして、テストケースから共通化するのはOK、共通化したものをさらに共通化したくなったら、やりすぎかもしれないと思うようにしています。
今回はここまでです。次回は、プロダクトコードのリファクタリングにも挑戦しましょう。コードはこちら https://github.com/yattom/tdd-life です。
