読者です 読者をやめる 読者になる 読者になる

潜伏バグからのロングフリーズ

Javaっぽいエンジニアの徒然草

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

Java

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

 

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

変数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の結合が混じってるけど、まぁこのぐらいなら許容範囲ではなかろうか。

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

 

ここまで脳内のお話。

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

 

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

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