弊社では、エンジニアの新人研修の一貫で1月程度、サンプルアプリケーションの作成を行います。
サンプルアプリケーションの規模は6-7画面程度で、作成期間中は日次で構成管理にコミットしてもらい教育係となった先輩エンジニアがコードレビューを実施します。

この記事ではコードレビュー時に指摘した内容をいくつかピックアップして紹介するのと、来年度以降への改善点として、プログラムレビューを実施して思ったことを記載します。

前提情報

以下、レビュー実施時どのような状況で実施しているのか前提となる情報を記載します。

サンプルアプリケーションについて

サンプルアプリケーションで使用しているプログラミング言語、サーバ等の構成要素は以下の通りです。
バージョン情報は割愛します。

プログラミング言語　　　　　　　　　
Java
J2EEサーバ
Glassfish
Webフレームワーク
Apache Wicket

DB層のライブラリ
一部JPA (EclipseLink)を使用しています。
大部分の処理は、社内開発したDBアクセス共通部品を使ってSQLを直接書いています。
apache/commons-dbutils: Mirror of Apache Commons DbUtils のような薄いライブラリです。

構成管理ツール

Subversionを使用しています。Vesionは以下の通りです。　

svnadmin --version
svnadmin, バージョン 1.7.14 (r1542130)
   コンパイル日時: Nov 20 2015, 19:25:09

静的解析ツール

SonarQubeでSubversionからソースコードを取得日次でチェックしています。
レビュー実施時に、静的解析結果も参考情報として参照します。

プログラムのレビュー指摘事項

以下の指摘をしました。

`System.out`を使うのではなく、`logger`に置き換える。

悪い例
プロダクションコード内で、System.outを使用してログを出力している。

   @Override
	public void getDelItem(TestDTO dto) {
		System.out.println("###Flow dto.getItemNumber() = " + dto.getItemNumber());
		getItemPrimitive().delItem(dto);
	}

良くない理由
デバッグのためにSystem.out.printlnを使用していますが、System.out.printlnは、APサーバ（Glassfish）のServer.logに出力されるので、APサーバ自体の起動、停止、エラー情報と混在します。
出力する文字列によっては、ログ監視システムが誤動作する可能性があります。
良い例
loggerを使用して、ログレベルを指定しログを出力してください。

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
	
   private Logger logger = LoggerFactory.getLogger(this.getClass());

   ...

   @Override
	public void getDelItem(TestDTO dto) {
      logger.info("###Flow dto.getItemNumber() = " + dto.getItemNumber());
		getItemPrimitive().delItem(dto);
	}

Loggerライブラリは設定ファイルでログの出力先を管理しています。
設定によっては、ログファイルにのみ出力する、コンソールにのみ出力する、ログファイル、コンソール両方に出力する等できます。
Loggerを使っていれば、デバッグのためのログ出力を残していても、設定ファイルを変更することで本番環境はログを出力しないこともできます。

Wicketの初期化処理は、Pageクラス、Panelクラスのコンストラクターではなく、`onInitialize()` に実装する

Wicket 1.5xから、初期化処理はコンストラクターではなく、onInitialize() に実装するのが推奨されています。

悪い例

    /**
     * コンストラクター
     *
     * @param params 入力パラメータ
     */
    public PracticeMyPage(PageParameters params) {
        super(params);
        super.renderJavaScriptFile("PracticeMyPage.js");
        add(new AbstractMyPage("MyPagePanel", params) {
            @Override
            protected AbstractMyPageMediator getMyPageMediator() {
                return kikakuMyPageMediator;
            }
        });
 
        String serverDate = DateUtil.dateTimeToStr(DateUtil.getSystemDate());
        this.setBodyOnLoad("setServerDate('" + serverDate + "');");
    }

良くない理由
- コンストラクターはsuper()を呼び出しを最初に書かなければいけない等、実装順序に制約が多いためです。
良い例

 /**
     * コンストラクター
     *
     * @param params 入力パラメータ
     */
    public PracticeMyPage(PageParameters params) {
        super(params);
    }

	/**
	 * 初期化処理
	 **/
	@Override
	public void onInitialize() {

      super.renderJavaScriptFile("PracticeMyPage.js");
      
      PageParameters params = super.getPageParameters();
      
      add(new AbstractMyPage("MyPagePanel", params) {
         @Override
         protected AbstractMyPageMediator getMyPageMediator() {
                return kikakuMyPageMediator;
         }
      });
      
      String serverDate = DateUtil.dateTimeToStr(DateUtil.getSystemDate());
      
      this.setBodyOnLoad("setServerDate('" + serverDate + "');");

      // 親のonInitialize()の呼び出しは処理の最後でも良い    
      super.onInitialize();
	}

Java7からはダイヤモンド演算子が使える。

Java7からは、ダイヤモンド演算子が使えます。
Java6以前で書かれたコードがあり、ダイヤモンド演算子を使用していないコードもありますが、新規作成する場合は、積極的に使用していきましょう。

悪い例

   List<String> strings = new ArrayList<String>();  
   Map<String,List<Integer>> map = new HashMap<String,List<Integer>>();

良い例

   List<String> strings = new ArrayList<>();
   Map<String,List<Integer>> map = new HashMap<>();

補足
ダイヤモンド演算子に関連する機能として以下があります。
Javaがバージョンアップすると、新しい構文で実装できるので定期的に情報収集しましょう。
- 大刷新リリース Java 8の新機能 (14) その他の新機能(1) - 便利なユーティリティ、ジェネリクスの改善 | マイナビニュース
  Java8ではメソッドの型パラメーターも省略できるようになりました。
- Java varメモ(Hishidama's Java var Memo)
  Java10ではvar変数が導入されました。

必要な変数のみメンバー変数として定義する。

流用元ソースの記述がそのまま残っているパターンが多いのかと思いますが、メンバー変数にする必要のない変数がメンバー変数として定義されています。

悪い例

public class AbstractRegistItem extends BaseNewUIPanel {

	/** Form */
	private HImForm<Void> form = null;
	
	/** 商品コード */
    private HImTextField<String> code = null;
    
    /** 商品名 */
    private HImTextField<String> syouhin = null;
    
    /** 入数/単位 */
    private HImTextField<String> num = null;
    ......

}

良くない理由
- 変数の定義箇所と使用箇所が離れるとプログラムの可読性が落ちます。
良い例

public class AbstractRegistItem extends BaseNewUIPanel {

	/**
	 * ページの初期化処理
	 */
	private void initPage(PageParameters params) {

		/** Form */
		HImForm<Void> form =  new HImForm<>("form", this);

      // 1番目のテキストボックス
		HImTextField txtItemName = new HImTextField<>("txtItemName");
		form.add(txtItemName);
      this.add(form);

      ...
   }

switch文では、default句を定義する。

switch文にはdefault句を必ず記載してください。

悪い例


   switch(dto.getSelectedValue()){
		case "1":
			sbSql.append("UPD_DATE DESC ");
			break;
		case "2":
			sbSql.append("UPD_DATE ASC ");
			break;
		case "3":
			sbSql.append("ITEM_NUMBER DESC ");
			break;
		case "4":
			sbSql.append("ITEM_NUMBER ASC ");
			break;
	}

良くない理由
- 実装後の保守フェーズで、default句がないのは実装漏れなのか、あえてそうしているのか判断ができない。
- 経験上、default句の実装漏れのバグはたまに出て、悲しい思い出となるケースがある。
良い例

	switch(dto.getSelectedValue()){
		case "1":
			sbSql.append("UPD_DATE DESC ");
			break;
		case "2":
			sbSql.append("UPD_DATE ASC ");
			break;
		case "3":
			sbSql.append("ITEM_NUMBER DESC ");
			break;
		case "4":
			sbSql.append("ITEM_NUMBER ASC ");
			break;
		default:
         // 場合に寄りけりだが、値が設定されないことがありえないなら例外をスローする。
			throw new llegalArgumentException("dto.getSelectedValue() is illegal...");
	}

補足
default句の実装漏れはPMD、SonarQubeで機械的にチェックできます。
静的解析ツールのサポートを受けて、実装漏れをなくしましょう。
- PMD Design ルール
- Java: "switch" statements should have "default" clauses

未使用のimport文は削除する

実装後の保守フェーズでノイズになるので、未使用のimport文は削除しましょう。
WindowsのEclipseではショートカットctrl + shift + o インポートの編成ができます。

変数名は制約がない限りキャメルケースを使う

変数名はフレームワークの制約等がない限り、コーディング規約上定められているキャメルケースを使いましょう。

悪い例

	private String SelectedValue = "";

良い例

	private String selectedValue = "";

補足
- このケースは、変数名の変更のリファクタリングを適用しましょう。変数の使用箇所も含めて軒並み変更が適用できます。
- プログラムエディター上で、対象の変数を選択 > 右クリック > 変換 を選択すると、Eclipseで実施可能な、定型変換処理の一覧が表示されます。
- 新人研修でドヤ顔で披露したらウケたEclipseのショートカット集 - Qiita にスネークケースとキャメルケースの切り替えのショートカットが記載されています。この悪い例の場合は、ショートカット2回でキャメルケースになります。

プログラムのコメントアウト行は削除する

プログラムのコメントアウト行は削除しましょう。
削除したほうが良い理由は以下になります。

理由
- 作成時、作成者はコメントアウトの意味は分かるが、3月後の変更時には過去に実施したことを忘れているのでノイズになる。
- 作成者以外には、コメントアウトの意味が分からない。プログラムの横断的な調査時にノイズとなり、作業効率が悪くなる。

未使用の変数は削除する

未使用の変数は削除してください。これもプログラムの調査時のノイズになるためです。
SonarQubeでは以下のような警告が出力されます。

Remove this unused "selLineCnt" private field.

何らかの理由で変数を残す必要がある場合は、@SuppressWarnings("unused") を付与すると警告を抑制できます。

inputタグには、labelタグを付与できるか検討する

これは、HTMLに関する指摘になります。inputタグには、対となるlabel属性が付与できます。
label化可能な情報がある場合は、labelを付与し、labelクリックで、input属性にフォーカスが当たるようにしましょう。

悪い例

<th colspan="2">
    <p class="disp-ic"><img height="13" width="23" class="ic-required" alt="必須" src="/web2/jp/images/ic-must.gif"></p>
    商品コード
    <div class="letter-control">(半角20文字以内)</div>
</th>
<td>
    <span class="cstm-tx">
        <span class="cstm-tx-bg">
            <input wicket:id="txtCode" type="text" maxlength="20" rel="半角20文字以内" class=" w100 noticeTip ime-actv" id="txtCode" name="c1-29" tabindex="40" />
        </span>
    </span>
</td>

良い例

<th colspan="2">
    <p class="disp-ic">
        <img height="13" width="23" class="ic-required" alt="必須" src="/web2/jp/images/ic-must.gif">
    </p>
    <!-- 商品コードにラベルを付与 -->
    <label for="c1-29">商品コード</label>
    <div class="letter-control">(半角20文字以内)</div>
</th>
<td>
    <span class="cstm-tx">
        <span class="cstm-tx-bg">
            <input wicket:id="txtCode" type="text" maxlength="20" rel="半角20文字以内" class=" w100 noticeTip ime-actv" id="txtCode" name="c1-29" tabindex="40" />
        </span>
    </span>
</td>

補足
そもそも、テーブルレイアウトなのでそれは良いの？
という問題はあります。。

プログラムレビューを実施して思うこと

プログラムレビューを通して、思うことを記載します。
単なる悩みとも言います。

ベストプラクティス、アンチパターンどちらを教えるべきか？

プログラムレビューは、アンチパターンに対してこうしたほうがいい。という指摘が多くなり、こう書いたほうがいいを教える機会が少ないように感じました。
ペアプロ、モブプロで実装を進めて、実装の意味がわからない部分を説明する等するとベストプラクティス系を教える機会がもてるのではと思っています。
また、「ベストプラクティス、アンチパターンどちらを教えるほうが、成長速度が速いのだろう？」と考えますが、現状答えは出ておりません。

静的解析ツールの警告内容がわからないというレベルがある。

これは個人的には大きい気づきでした。
静的解析ツールは、アンチパターンを自己学習には最適なツールかと思います。
新人研修では一部の方に、警告内容が日本語化されている SpotBugs Eclipseプラグインの使い方 — spotbugs 4.0.0-beta4 ドキュメントを導入してもらっていたのですが、警告の内容を見ても何をしていいのかわからない。ので見なかったことにする。 という状況に陥っていました。
このケースは警告の内容の読み方を教える、情報処理関連の語彙を覚えてもらう、分からない場合の調べ方を教える等が必要かと思います。

良くないプログラムを流用して、良くないプログラムを作る。

研修用のアプリケーションは、実際に稼働中のサービスのコードを流用、スケルトン化して作成しています。
開発時の都合等もあり、品質が安定している箇所と安定していない箇所が混在しており、流用元が良くないというレビュー指摘になるケースがありました。
理想を教えるという意味では研修用のアプリケーションの品質を高めたほうが良いように思いました。

※新人エンジニアとはいえ、流用元を精査する責任はあり、それも含めての研修という考えもあります。

最後に

来年度研修では、今年の失敗ポイントを活かしてもう少し良いレビューが実施できればと思います。
ペアプロ、モブプロは数回実施しましたが、もう少し数を増やしたいです。

以上です。