レガシーコードで静的コード解析ツールの指摘を修正する時に気を付けた方がよいこと

この記事の内容

spotbugsやSonarQubeなど静的コード解析ツールは不具合の混入や良くない実装を人間が確認しなくても指摘してくれるとても便利なツールです。
しかしすでに出荷され運用しているソースコード（特にUnitTestの整備が不十分で設計が甘いことが多いレガシーコード）に対して指摘内容を修正する際には、意図しない動作変更が発生しないか注意する必要があります。

本記事では私が確認したMR(PR)で実際に意図しない動作変更が混入されていた事例の紹介と修正時に気を付けた方が良いことを記載します。
（事例については新しい事例が出てきた際には追記していこうと思っています）

事例

例外系の意図しない動作変更

以下のようなソースコード（社外公開できるように改変しています）で
spotbugsのNP_NULL_ON_SOME_PATH_EXCEPTIONが検出されました。

	public Map getCanSelectMap(int type) {
		Map canSelects = null;
		try {
			switch (type) {
			case TYPE.VALUE:
				canSelects = getCanSelectMapValue();
				break;
			case TYPE.PARENT_VALUE
				canSelects = getCanSelectMapParentValue();
				break;
			case TYPE.SELECTION
			case TYPE.DIVISION
				canSelects = getCanSelectMapRelationValue();
				break;
			default:
				canSelects = getCanSelectMapOther();
				break;
			}
		} catch (SQLException e) {
			e.printStackTrace();
		}

		canSelects.remove("0");
		return canSelects;
	}

色々と突っ込みどころのあるコードだと思いますが、引数で渡ってきたtypeに応じてcanSelectsというMapを生成し返すロジックのようです。
ここには記載できていませんが、canSelectsを生成する各ロジック（getCanSelectMapValueなど）はデータベースに接続するロジックでSQLExceptionをthrowしてきます。
Mapにジェネリクスがついていない辺りからJava1.4の時代から存在する20年近く前のロジックだと思われます。

このクラスに対するリファクタリングを実施している時（このメソッド自体はもともとリファクタリングしたい対象ではなかった）にspotbugsの指摘を受けて以下のような修正をされていました。

	public Map getCanSelectMap(int type) {
		Map canSelects = new HashMap();
		try {
			switch (type) {
			case TYPE.VALUE:
				canSelects = getCanSelectMapValue();
				break;
			case TYPE.PARENT_VALUE
				canSelects = getCanSelectMapParentValue();
				break;
			case TYPE.SELECTION
			case TYPE.DIVISION
				canSelects = getCanSelectMapRelationValue();
				break;
			default:
				canSelects = getCanSelectMapOther();
				break;
			}
		} catch (SQLException e) {
			e.printStackTrace();
		}

		canSelects.remove("0");
		return canSelects;
	}

最初に空のMapを持っておくことによって確かにNP_NULL_ON_SOME_PATH_EXCEPTIONは検知されなくなりましたが、SQLExceptionが発生した時のメソッドの仕様が NullPointerException を返すという挙動から 空のMapを返す という挙動に変わってしまっています。

SQLExceptionが発生した時に握りつぶして NullPointerException を返すというのは仕様と呼ぶのがはばかられるような邪悪な動作ではありますが（さらに言うと意図的な実装ではなさそうですが）それが現状のこのメソッドの仕様でした。
空のMapを返すように動作変更されたことによって、呼び出し元の実装によっては中断されていた処理が意図せず続行されるようになり、不整合データができたりエラーがユーザーに通知されなったりする可能性があります。
意図せずメソッドの仕様を変えることは危険です。
リファクタリングであれば対象のメソッドがどのような時にどのような例外を返すのかも含めて仕様を定義し、既存のアプリケーションの動作に影響がないように対応をする必要があります。

今回の例であれば、例外の実装規約にもよりますが呼び出し元の状況を調べ、SQLExceptionをthrowしたり、IllegalStateExceptionなど適切な実行時例外を返すようにするべきでしょう。

修正時に気を付けた方が良いこと

前提

前述の事例のようなリスクやコストはあるものの、静的解析ツールの指摘は不具合や設計に良くない点があることを示すものであり、しかもそのチェックが人の手を介さずに実施できるためうまく活用した際のメリットは非常に大きいと思います。
組織として対応しようと決めた検知結果に関しては修正するべきです。
しかしそこには意図しない仕様変更のリスクがあることを認識しましょう。

一つずつ直す

前述のように静的コード解析ツールの指摘の修正には意図しない仕様変更のリスクがあります。
そのため修正者もしっかりと検討した上で実装する必要がありますし、
レビュアーもしっかりと内容を確認する必要があります。

一つ一つの修正を最低でもコミットを分けて修正するべきだと思います。
例えば前述の事例のような修正がIDEの機能による機械的なリファクタリングと同じコミットで行われていると、レビュアーは内容を確認することが難しくなります。
参考：https://qiita.com/gengen0719/items/e74465e4b6c55674d07d

例外時の仕様も厳密に定義する

メソッドの設計の基本ではありますが、例外処理も含めて厳密に現状の仕様を定義しそれを踏まえた上で修正を行いましょう。
ソースコードを読んだだけで仕様を厳密に定義できない場合は仕様化テストを書きましょう。