« [イベント] TDD 道場 第4回 @わんくま名古屋勉強会#14 ~ リファクタリングやってみよう! Step by Step | トップページ | [コラム] リファクタリングとは、 最適化することじゃない。 理解しやすくすること。 »

2010年10月19日 (火)

[コラム] リファクタリングはどっちへ進むべきだろう? ~ 機械に優しく? 人間に優しく?

わんくま勉強会#15 の中で TDD 道場 (第5回) を開催させていただきました。
キーボードを打っていただいた 3名のかた、 ありがとうございました。 あいかわらずのグデグデでしたが、 参加していただいたみなさま、 いかがでしたでしょうか。

今回はリファクタリング超入門という感じで、 FizzBuzz をお題にして VB.NET でやりました。 開始時点でのコードはこのようになっていました。 (こちらからダウンロードできます。 ⇒ 勉強会などで使った資料 )

'【テストコード】

<TestFixture(), Description("FizzBuzz のテスト")> _
Public Class FizzBuzzTests

<TestCase(3)> _
<TestCase(6)> _
Public Sub SayTest01_3の倍数のときFizz(ByVal n As Integer)
Assert.AreEqual("Fizz", FizzBuzz.Say(n))
End Sub

<TestCase(5)> _
<TestCase(10)> _
Public Sub SayTest02_5の倍数のときBuzz(ByVal n As Integer)
Assert.AreEqual("Buzz", FizzBuzz.Say(n))
End Sub

<TestCase(15)> _
Public Sub SayTest03_3と5の倍数のときFizzBuzz(ByVal n As Integer)
Assert.AreEqual("FizzBuzz", FizzBuzz.Say(n))
End Sub

<TestCase(1)> _
<TestCase(2)> _
<TestCase(4)> _
<TestCase(7)> _
<TestCase(14)> _
<TestCase(16)> _
Public Sub SayTest04_それら以外のときは数字(ByVal n As Integer)
Assert.AreEqual(n.ToString(), FizzBuzz.Say(n))
End Sub

<TestCase(0, expectedExceptionName:="System.ArgumentOutOfRangeException")> _
<TestCase(-3, expectedExceptionName:="System.ArgumentOutOfRangeException")> _
Public Sub SayTest05_0と負のときは例外(ByVal n As Integer)
Console.WriteLine("n={0} のとき '{1}'", n, FizzBuzz.Say(n))
End Sub

End Class

 

'【製品コード】

Public Class FizzBuzz

Public Shared Function Say(ByVal n As Integer) As String
Dim s As String = n.ToString()

If (n > 0) Then
If (n Mod 3 = 0) Then
If (n Mod 5 = 0) Then
s = "FizzBuzz"
Else
s = "Fizz"
End If
ElseIf (n Mod 5 = 0) Then
If (n Mod 3 = 0) Then
s = "FizzBuzz"
Else
s = "Buzz"
End If
End If
Else
Throw New ArgumentOutOfRangeException("n", n, "…略…")
End If

Return s
End Function

End Class

※ このサンプルコードは、 TDD 三原則を破って、 まずテストコードを全部書き下してから製品コードを書きました。 三原則を守っていると、 こんなコードは書けません。

この製品コードに対して、 次のような課題を指摘できます。

  • 重複コード: (n Mod 3 = 0) の判定と (n Mod 5 = 0) の判定が 2回ずつ出てくる。
  • 重複コード: s = "FizzBuzz" という代入が 2回ある。
  • If の入れ子が 3重になっていて、 理解しにくい。
  • 変数 s のスコープが広い (メソッドの先頭から末尾まで)。 スコープが広いわりには、名前が意味不明。
  • 引数が何であれ n.ToString() を行っているが、 必要なのか?

この課題は、 無償の Visual Basic 2010 Express Edition と NUnit 2.5 で、 やってみることが出来ます。 ぜひやってみてください。

さて、 TDD 道場では時間内にリファクタリングは完了しませんでしたが、 進んでいた方向からすると、 次のような姿になったであろうと思われます。

Public Shared Function Say(ByVal n As Integer) As String
If (n <= 0) Then
Throw New ArgumentOutOfRangeException("n", n, "…略…")
End If

Dim output As StringBuilder = New StringBuilder()

If (n Mod 3 = 0) Then
output.Append("Fizz")
End If
If (n Mod 5 = 0) Then
output.Append("Buzz")
End If
If (output.Length = 0) Then
output.Append(n.ToString())
End If

Return output.ToString()
End Function

先に挙げた 5つ課題のうち 4つが解消されています (広いスコープを持つローカル変数は、 s が消えた代わりに output が登場してしまいました)。 十分に改善されています。
条件判断は 5回だったもののが 4回に減っています。 If 文の 3重だったネストは無くなっています。 n.ToString() の呼び出しは、 必要なときだけになりました。 フラグ変数を導入したりもしていません。

FizzBuzz は短いコードで、 全体を把握するのも割と楽です。 このコードで問題無いでしょう。

しかし、 もしもこれが長い業務ロジックだったとすると、 疑問になるコードが登場しています。

FizzBuzz の仕様はこうです。

  • 3の倍数なら、 "Fizz" と言う。
  • 5の倍数なら、 "Buzz" と言う。
  • ただし、3の倍数 かつ 5の倍数なら、 "FizzBuzz" と言う。
  • それら以外は、 その数を言う。

では、 この条件式は何を表しているでしょう?

    If (output.Length = 0) Then

どの仕様と結びついているか、 分かりますか?
もちろん、 コードが短いので、 ちょっと考えれば分かると思います。 しかし、 これが長い業務ロジックの中にあったらどうでしょう?

この条件式は、 その前に出てきた複数の条件式のいずれも成立しなかった場合に True になります。 つまり、 最後の仕様の 「それら以外は、」 を表していることになります。
変数の数は増やさず、 同じ条件判定処理を繰り返さず、フラグも導入せずと、 機械には優しいコードになっていますが、 人間に対してはどうでしょう? コードを読み解いてみて、「あぁ、それら以外の場合か!」 と理解しなければならないというのは、 人間に対してはあまり優しくはないのではないでしょうか。 (繰り返しますが、 全体が短いコードなので、 FizzBuzz では大した問題ではありません。)

私はこのような場合、 人間に優しく、 つまり、 仕様とできるだけ一致するように書くのを好みます。
「それら以外は、」 を表現するには、 たとえば If ~ Else で書けますね。

If 条件A Then
    'A のときの処理
Else If  条件B Then
    'B のときの処理
Else
    'それら以外のときの処理
End If

場合によっては switch 文でも表現できます。

Select Case 条件変数
    Case A
        'A のときの処理
    Case B
        'B のときの処理
    Case Else
        'それら以外のときの処理
End Select

switch 文 (Select ステートメント) の方が分かりやすいですが、 いつでも使えるわけではありません。 If ~ Else If ~ Else If ~ の連鎖は、 switch より理解しにくいものですが、 処理を終わった時点でメソッドから抜けて良いなら、 把握しやすい形に書けます。

If 条件A Then
    'A のときの処理
    Return
End If
If  条件B Then
    'B のときの処理
    Return
End If

'それら以外のときの処理
Return

※ switch 文を使った場合と見比べてみてください。

そんなわけで、 私が書いた FizzBuzz は次のようになりました。

Public Shared Function Say0(ByVal n As Integer) As String
If (n <= 0) Then
Throw New ArgumentOutOfRangeException("n", n, "…略…")
End If

Dim is3の倍数 As Boolean = (n Mod 3 = 0)
Dim is5の倍数 As Boolean = (n Mod 5 = 0)

If (is3の倍数 AndAlso is5の倍数) Then
Return "FizzBuzz"
End If
If (is3の倍数) Then
Return "Fizz"
End If
If (is5の倍数) Then
Return "Buzz"
End If

Return n.ToString()

End Function

※ これも前に挙げた課題のうち 4つを解消しています。 また、 新しく導入した変数 is3の倍数 と is5の倍数 のスコープは多少狭くなっています。 (最初に値をセットしたらあとは読み出すだけなので Const にしたいくらいですが、 それはコンパイルエラー)

まぁほとんど趣味の世界のお話で、 どちらでも構わないようなことではあります。
ですが、 もうちょっと考えてみてください。
どちらが仕様変更のとき楽でしょう?

もとの仕様はこうでした。

  • 3の倍数なら、 "Fizz" と言う。
  • 5の倍数なら、 "Buzz" と言う。
  • ただし、3の倍数 かつ 5の倍数なら、 "FizzBuzz" と言う。
  • それら以外は、 その数を言う。

これに対して、 どんな変更が考えられるでしょう…

  • [A:変更] 3の倍数のときは "Bizz"、 15の倍数のときは "Bizz Buzz" ("BizzBuzz" ではない) 
  • [B:追加] ただし、1の位が 7のときは "seven!" と叫ぶ
  • [C:変更] 15の倍数のとき、 偶数なら "Bizz Buzz"、 奇数なら "Buzz Bizz"

どれもたいした仕様変更ではなさそうですよね?
では、 A, B, C の順に、 それぞれのコードを修正してみてください。 あるいは、 この他の仕様変更も考えてみてください。

いかがだったでしょう?
どちらが仕様変更しやすかったですか? 変更範囲はどうでしたか?

仕様変更のときは、 まず、 元の仕様に対しての変更を考えるものです。 変更要求を出す人も、 コードを修正しようとする人も。
それでアタリを付けておいてから、 実際のコードに取り組むでしょう。 そのときに、 仕様の表現とコードの表現が似ているほうが楽か、 そうでないほうが楽か…?
仕様変更に強い (変更しやすい) コードという観点からは、 どちらが良いか明白ですね。

他のシステムとやりとりするコード (UI とか DB アクセスとか通信とか) は、 他システムの都合に合わせたコードを書いたほうがおそらく良いのです。
しかし、 業務ロジックのコードでは、 人間の思考に合わせた書き方をした方が、 たぶんお得でありましょう。

|

« [イベント] TDD 道場 第4回 @わんくま名古屋勉強会#14 ~ リファクタリングやってみよう! Step by Step | トップページ | [コラム] リファクタリングとは、 最適化することじゃない。 理解しやすくすること。 »

*コラム」カテゴリの記事

<NUnit>」カテゴリの記事

コメント

(株)GWPのおがたんです。最近ラムダ式を覚えて、とにかく短いコードを書くことにはまっています。

私のリファクタリング案をC#で書きます。
static string say(int n) {
if (n Is倍数 = (x) => n % x == 0;
return ((Is倍数(3)) ? "Fizz" : "") + ((Is倍数(5)) ? "Buzz" : "") + ((!Is倍数(3) && !Is倍数(5)) ? n.ToString() : "");
}

変態的ですが、真の仕様をそのまま書いた記述になっていると思います。
biacさんの例では、倍数判定に重複が見られること、ifブロックが多発していること、returnで返る位置が複数あって後での見直し時に時間がかかることが気になります。

仕様変更に強い弱いの議論については思うところもありますが、1行直すだけならそんなに考える時間もかからないかなと思います。
さらに言うならsay関数を使う場所が1箇所であるなら、ラムダ式にしてしまって使用する関数内にいれてしまえば、ソース的に起り得ない例外処理の1行を省くことができます。(for分で1以上の整数しかはいらないとかの局面で)
リファクタリングを進めると、教材的なFizzBuzzが仕様的に非線形で仕様自体の変更を推奨されるところに行き着きます。(わざとそういう問題を作ってるからですが)

投稿: おがたん(GWP) | 2010年10月20日 (水) 10時28分

おっと、ラムダ式つかったのでタグにひっかかってしまった。
static string say(int n) {
if (n <= 0) throw new ArgumentOutOfRangeException("n", n, "--");
Func<int, bool> Is倍数 = (x) => n % x == 0;
return ((Is倍数(3)) ? "Fizz" : "") + ((Is倍数(5)) ? "Buzz" : "") + ((!Is倍数(3) && !Is倍数(5)) ? n.ToString() : "");
}

投稿: おがたん(GWP) | 2010年10月20日 (水) 10時44分

> とにかく短いコードを書くことにはまっています

いやそれ、短小グランプリであってリファクタリングじゃないし~w

※ コード見るのは帰ってからね f(^^;

投稿: biac | 2010年10月20日 (水) 13時06分

てことで、 コードを見ました、 ってゆーか MSIL を見ました。
ある程度は予想してたけど、 ラムダ式の負荷は想像以上にデカかった f(^^;

http://www.tdd-net.jp/2010/10/post-a5a3.html
> [コラム] リファクタリングはどっちへ進むべきだろう? (続)

※ あと、 おがたんには、 仕様変更 A, B, C をぜひやってもらいたいw

投稿: biac | 2010年10月21日 (木) 00時57分

コメントを書く



(ウェブ上には掲載しません)




トラックバック


この記事へのトラックバック一覧です: [コラム] リファクタリングはどっちへ進むべきだろう? ~ 機械に優しく? 人間に優しく?:

« [イベント] TDD 道場 第4回 @わんくま名古屋勉強会#14 ~ リファクタリングやってみよう! Step by Step | トップページ | [コラム] リファクタリングとは、 最適化することじゃない。 理解しやすくすること。 »