リファクタリング未遂事件

※別に事件でもなんでもないのだけれど語呂が良いので。

こんなソースコードを見かけた。

変数a, b, c, d, hoge, fuga, hogehoge, fugafugaは全てString型。

1. if (null == hoge || null == fuga) {

2.     if(a.equals(b) && c.equals(d)) {

3.         // 処理A

4.     }

5. } else {

6.     if(a.equals(b) && c.equals(d)

          && hoge.equals(hogehoge)

          && fuga.equals(fugafuga)) {

7.         // 処理A

8.     }

9. }

やってることは、

　比較対象変数同士が等しい場合に「処理A」を実施する。

　hoge, fuga, hogehoge, fugafugaにはnullまたは空文字列が入る可能性があり、

　少なくともどちらか片方にnullが入る場合は比較対象外とする。

といった感じ。

nullが入った場合は2行目、空文字列が入った場合は6行目の処理を行うわけだけれども、なんとも分かりにくい。

特に、空文字来た場合に空文字同士を比較するというのは...ねぇ。

とりあえず空文字対応。

public String isNullOrEmpty(String value) {

    return (null == value || 0 == value.length);

}

こんなUtilityを作ってそれを呼ぶ形にしよう。

こんな感じ。

1. if (isNullOrEmpty(hoge) || isNullOrEmpty(fuga)) {

2.     if(a.equals(b) && c.equals(d)) {

3.         // 処理A

4.     }

5. } else {

6.     if(a.equals(b) && c.equals(d)

           && hoge.equals(hogehoge) && fuga.equals(fugafuga)) {

7.         // 処理A

8.     }

9. }

そして何より気になるコピペコードを消し去りたい。

こんな感じ。

1. if(a.equals(b) && c.equals(d)) {

2.     if ( (isNullOrEmpty(hoge) || isNullOrEmpty(fuga))

               || (hoge.equals(hogehoge) && fuga.equals(fugafuga))) {

3.         // 処理A

4.     }

5. }

ORとANDの結合が混じってるけど、まぁこのぐらいなら許容範囲ではなかろうか。

元のコードに比べれば幾分マシになった。

ここまで脳内のお話。

現実は「動いているコードは触るな」という御触れ光臨というオチ。

「一度リファクタリングを始めると、リファクタリングしたくてしょうがなくなる」

と聞いたことがあるが、成る程、依存性があるらしい。