Javaプログラミング能力認定試験課題プログラムのレビュー

コードレビューの練習

勉強がてら、Javaプログラミング能力認定試験課題プログラムのコードを独断と偏見でレビューしてみようと思う。
コードは１級の問題に使われるサンプル問題のプログラムソースで、上記サイトから誰でもダウンロードできる。

プログラム仕様は上記サイトのテーマプログラムが詳しい。
簡単に説明すると、コマンドラインに機能を表示して、ユーザ入力に応じてcsvデータの追加・更新処理を行うプログラム。

　▼動かすとこんな感じ。昔なつかしい。

全般

コードを見た率直な感想としては、まずはコメントが少ない。
まぁ少ないだけなら試験が簡単になりすぎないようにしているんだろうな、と思えるけど
そもそもJavadocコメントの書き方間違えてたりしてびっくりする。

なんというか、そもそも最近のjavaの実務経験がない人の印象を受ける。
ここ10年ぐらいのオブジェクト指向とかコーディング規約に疎い感じ。

個人的には、このレベルのソースはjavaの初級かなと思う。
資格のサイトの3級には「javaの簡単なプログラムが書ける。」と書いてあったけどそのレベル。
まぁ、このコードがかけても即実践投入は難しいと思う。
少なくとも１級のレベルではない。最上位なら、ラムダ式とはいわないから、
せめて最低限スレッド、ジェネリクスくらいは触ってほしい。

話は逸れるけど、実務能力を示すなら、javaの言語仕様よりも、フレームワーク、ユニットテスト、リポジトリ管理、
自動化（Antとか）、IDEの使い方を知ってるほうが大事な気がする。
それこそjavadocの書き方とか、そーゆー能力を資格の中で試験してくれると、実務能力が示せるかなと思う。

レビュー：SystemManager クラス（フィールド宣言部分）

コーディング規約はJava コーディング規約 2004に従うことにする。

クラス数は全部で23クラス。
まずはmainクラスから。

/** SystemManager
 */
public class SystemManager {
	/**
	 * フィールド
	 */
	private PersonList plist;  // 従業員のリスト
	private ClientList clist;  // 顧客のリスト
	private WorkList wlist;    // 稼働のリスト

	private String pfilename = "person.csv";
	private String cfilename = "client.csv";
	private String wfilename = "work.csv";

	private ConsoleStatus sts1, sts2;
	private DisplayPersonStatus sts5, sts5_2;
	private DisplayPersonsByTypeStatus sts4;
	private TypeSelectionStatus sts3;
	private DisplayPersonsByNameStatus sts7;
　　　 ...

No	クラス	指摘内容
1	SystemManager	1行目、流用を考えてpackage宣言したほうが良い。
2	SystemManager	クラスにはクラスの責務がわかるようクラスコメントを書いたほうが良い。
3	SystemManager	「DisplayPersonStatus 　sts5, sts5_2」では変数の意味がわからないので、意味が伝わる変数名を使うべき。例「displayPersonStatusFromType」「displayPersonStatusFromName」
4	SystemManager	csvファイル名(person.csv)がハードコーディングされている。設定ファイルやプロパティファイルに外部化すべき。

レビュー：SystemManager クラス（statusSettingメソッド）

	// 状態遷移の設定
	/**
	 * statusSetting
	 */
	public void statusSetting() {

		// システム起動時の，機能選択の状態
		sts1 = new ConsoleStatus(
		    "_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/\n" +
		    "            従業員派遣管理システム\n" +
		    "                メニュー\n" +
		    "  従業員検索(S)\n" +
		    "  従業員管理(JI：追加 JU：更新 JD：削除)\n" +
		    "  稼働状況管理(KI：追加 KD：削除)\n" +
		    "  終了(X)\n" +
		    "_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/\n",
		    "どの機能を実行しますか？\n[S,JI,JU,JD,KI,KD,X]>",
		    false
		 );
		// 起動時から"S"入力時の状態
		sts2 = new ConsoleStatus(
		    "検索方法を指定してください。\n" +
                    "N->氏名から検索      T->職種から検索\n" +
                    "E->従業員検索終了(メニューに戻る)",
                    "[N,T,E]>",
		    false
		 );
		/*******   省略   *******/
		sts9.setNextStatus( " ", sts1 );
		sts10.setNextStatus( " ", sts1 );
		sts11.setNextStatus( " ", sts1 );
		sts12.setNextStatus( " ", sts1 );
	}

No	クラス	指摘内容
5	SystemManager	statusSettingメソッドの行数が148行ある。これは規約の「1 メソッドの行数は約 20 行以下.」に大きく違反しているので、メソッドを分割する。
6	SystemManager	「従業員派遣管理システムメニュー」のような画面の表示内容がハードコーディングされている。画面の表示文言が変更になった際にソース修正が不要となるよう、設定ファイル等に外部化すべき。

レビュー：AddWorkStatus クラス

次はAddWorkStatusクラス。
これはConsoleStatusを継承していて、Workを追加する機能を実現してる。
とりあえずこれで最後。

/** AddWorkStatus
 */
public class AddWorkStatus extends ConsoleStatus {
	// フィールド
	private ClientList cl;
	private WorkList wl;
	private String[] messages = {
		"従業員IDを入力してください。\n>",
		"顧客IDを入力してください。\n>",
		"稼働開始年月日を入力してください。\n>",
		"稼働終了年月日を入力してください。\n>",
		"単価を入力してください。\n>"
	};
	private String[] data = new String[ 5 ];

	/*******   省略   *******/

	// 最初に出力するメッセージを表示する
	// このクラスでは稼働のデータの入力処理
	// のみを行う
	/** displayFirstMess
	 * @throws Exception
	 */
	public void displayFirstMess() throws Exception {
		// messagesの各文字列を順に表示しながら
		// キーボードから氏名，住所などを得ていく
		System.out.print( messages[ 0 ] );
		data[ 0 ] = inputMessage();
		cl.allDisplay();
		System.out.print( messages[ 1 ] );
		data[ 1 ] = inputMessage();
		for( int idx = 2; idx < messages.length; idx++ ) {
			System.out.print( messages[ idx ] );
			data[ idx ] = inputMessage();
		}
		try {
			int pid = Integer.parseInt( data[ 0 ] );  // 従業員ID
			int cid = Integer.parseInt( data[ 1 ] );  // 顧客ID
			int pr = Integer.parseInt( data[ 4 ] );   // 契約単価

			Work new_w = new Work(
			  wl.size(), // 現在のWorkListのレコード数を
			             // 新しいレコードのIDとする
			  pid, cid, data[ 2 ], data[ 3 ], 
			  pr, false
			);
			// 新しいレコードを追加
			wl.add( new_w );
			System.out.println( "ID:" + new_w.getID() + "で登録されました。" );
		} catch( NumberFormatException e ) {
			System.out.println( "数値に変換できないデータが入力されています。" );
			System.out.println( "再入力してください。" );
			displayFirstMess();
			return;
		}
       ...

No	クラス	指摘内容
7	AddWorkStatus	「int cid = Integer.parseInt( data[ 1 ] ); // 顧客ID」 1がマジックナンバーになっている。 Enumで宣言するか、データクラス作るのが良いのでは。
8	AddWorkStatus	「System.out.println」が散在していて、出力先が変わった場合に修正個所が多くなってしまっている。まずはprintlnを呼び出すstaticメソッドを１つ作って、各クラスはそれを呼び出す形式に修正し、コード中のprintlnが一カ所になるようにするのが良いのでは。もう一歩進めると、各クラスは表示文字列だけを返すようにして、出力先や出力タイミングは呼び出し側で決めるところまでいければ、画面出力が一カ所にまとまってよいと思う。
9	AddWorkStatus	displayFirstMessメソッドの例外が「throws Exception」になっているが、実際はIOExceptionしかthrowされないはずので、「throws IOException」にする。そちらのほうがキャッチ側で適切に処理できる。

おわり

とりあえず素直に思ったことを書きなぐった。
コードレビューというか、リファクタリングを考えるのは結構好きな作業だったりする。
今後、時間ができたらテスト作ってリファクタリングしたい。

リファクタリングの課題としてはいいソースコードだと思う。
本当に残念なのは、実際のJavaプログラミング能力認定試験課題プログラム1級の試験では、変更できるクラスが限定されていること。
「このソースをより保守性が高くなるようリファクタリングしなさい(制限時間60分)」とかだったら喜んで試験受けてた。