前回に引き続き、プログラムの品質確保のノウハウをまとめていきます。

例1:紛らわしい変数名
例2:フラグの多様
例3:否定の否定
例4:条件文のネスト
例5:長い条件文
例6:関数のネスト
例7:上から下に流れないロジック
例8:スパゲッティ

例6:関数のネスト

仕様

引数で文字列配列を受け取り、全ての文字列に対して以下の処理を行い、それぞれの行の結果をカンマ区切りの文字列で表示する。

・長さが10バイト以上あるときはエラー
・1桁目が”A”の場合
	2桁目が”1″の場合はエラー
	2桁目が”0″の場合
		3桁目以降に数値が入っている場合はエラー
		3桁目以降に”,”が含まれている場合は、文字列を”,”で区切った2つ目の文字列
		3桁目以降に”,”が含まれていない場合は、3桁目以降の文字列
	2桁目が上記以外の場合
		3桁目以降の文字列
・1桁目が”B”の場合
	2桁目が”A”の場合はエラー
	2桁目が”X”の場合
		3桁目が”@”以外の場合はエラー
		上記以外の場合
			4桁目以降に”,”が含まれている場合は、文字列を”,”で区切った2つ目の文字列。
			4桁目以降に”,”が含まれていない場合は、4桁目以降の文字列。
	2桁目が上記以外の場合
		3桁目以降の文字列
・1桁目が上記以外の場合は、エラー

実行例

> javac sample6.java
> java sample6 A0XXX,YYY BX?KKKK A1CCCC
YYY,KKKK,エラー

ロジック

// メイン処理
public void main(String[] arg){
    List dispList = new ArrayList();
    for(int i=0; i<arg.length; i++){ dispList.add(getResultStr(arg[i])); } System.out.println(dispList); } // パラメータごとの処理 private String getResultStr(String arg){ if(arg.length>10){
        return "エラー";
    }
    if(arg.substring(0,1).equals("A"){
        return getA_Function(arg.substr(1));
    }
    if(arg.substring(0,1).equals("B"){
        return getB_Function(arg.substr(1));
    }
    return "エラー";
}
// 数値が含まれているかどうかをチェック
private boolean hasNum(String arg){
    return arg.match("^.*[0-9]*.*$");
}
// 1桁目が"A"の場合の処理
private String getA_Function(String arg){
    if(arg.substring(1,1).equals("1")){
        return "エラー";
    }
    if(arg.substring(1,1).equals("0")) {
        return getARetStr(arg.substr(1));
    }else{
        return arg.substr(1);
    }
}
// 2桁目が"0"の場合の処理
private String getARetStr(String arg){
    if(hasNum(arg)) [
        return "エラー";
    }
    if(arg.indexOf(",")<0){
        return arg;
    }
    return arg.split(",")[1];
}
// 1桁目が"B"の場合の処理
private String getB_Function(String arg){
.....

問題点

関数が複数階層にネストしており、ロジックを追うことが困難となっています。

上記の例ではロジックの流れる順にメソッドが実装されているので、まだわかりやすいですが、メソッドが処理順に並んでなかったり、別クラスに実装されていると追いきれなくなります。

解説

コーディング基準などで1つのメソッドの行数を縛る案件も実際に存在するのでやむを得ない場合はありますが、基本的に長くなるという理由でメソッドを分けるのはお勧めできません。

メソッドやクラスは役割を明確にし、役割上必要な機能を配置するようにしましょう。

下の改善ロジックではメインメソッドはコントローラ、個々の処理として”A”、”B”の処理メソッド、ユーティリティメソッドとして数値判定処理という役割で実装しています。

“A”,”B”の処理を条件の違いを引数で受け取って1つの共通メソッドにすることもできるかもしれませんが、

  • “C”処理の追加などの仕様変更があると複雑化する
  • “B”処理に不具合や仕様変更があると”A”処理のテストも必要

など、変更に弱いプログラムになります。

共通化・部品化は実装工数を大きく下げることができる重要な手法ですが、取り入れる時には注意が必要です。

// メイン処理
public void main(String[] args){
    List dispList = new ArrayList();
    for(int i=0; i<args.length; i++){ String param = getResultStr(args[i]); // 10桁以上はエラー if(param.length>10){
            dispList.add("エラー");
            continue;
        }
        // 1桁目が"A"の場合
        if(param.substring(0,1).equals("A"){
            dispList.add(getA_Function(param.substr(1)));
            continue;
        }
        // 1桁目が"B"の場合
        if(param.substring(0,1).equals("B"){
            dispList.add(getB_Function(param.substr(1)));
            continue;
        }
        // 上記以外はエラー
        dispList.add("エラー"));
    }
    System.out.println(dispList);
}
// 数値が含まれているかどうかをチェック
private boolean hasNum(String param){
    return param.match("^.*[0-9]*.*$");
}
// 1桁目が"A"の場合の処理
private String getA_Function(String param){
    if(param.substring(1,1).equals("1")){
        return "エラー";
    }
    if(param.substring(1,1).equals("0")) {
        if(hasNum(param)) [
            return "エラー";
        }
        if(param.indexOf(",")<0){
            return param;
        }
        return param.split(",")[1];
    }else{
        return param.substr(1);
    }
}
// 1桁目が"B"の場合の処理
private String getB_Function(String param){
.....

例7:上から下に流れないロジック

仕様

文字列:[“1234567890”, “AAAAAA”, “G”, “ajslga”, “あいうえお”, “9999999999”, “0”, “”]に対して、それぞれ以下の処理をランダムに呼び出して処理結果を出力する。

  • “値=[文字列]”と出力
  • 長さが5以上の場合、”値=[文字列の1~4文字目]:[文字列の5文字目以降]”と出力
  • “値=”[文字列]””と出力
  • “値=[文字列×100+5]$”と出力、数値変換できない場合は”値=[文字列]”と出力

ロジック

public class Test {

    private static String getReturn(String p){
        try{
            Integer.parseInt(p);
            return "\"" + p + "\"";
        }catch(NumberFormatException e){
            return p;
        }
    }
    private static String[] getA001(){
        return new String[]{"1234567890", "AAAAAA", "G", "ajslga", "あいうえお", "9999999999", "0", ""};
    }
    private static String getB001(String p){
        return "値=" + p;;
    }
    private static String getC001(String p){
        String ret = "";
        if(p.length()>5){
            ret = p.substring(0,4) + ":" + p.substring(5); 
        }
        return "値=" + ret;
    }
    public static void main(String[] args) {
        Random rnd = new Random();
        int ran = rnd.nextInt(10);

        String[] a001 = getA001();
        for(String a : a001){
            switch (ran) {
            case 0:
                System.out.println(getB001(a));
                break;
            case 1:
                System.out.println(getC001(a));
                break;
            case 2:
                System.out.println(getD001(a));
                break;
            case 3:
                System.out.println(getE001(a));
                break;
            default:
                break;
            }
        }
    }
    private static String getD001(String p){
        return "値=" + getReturn(p);
    }
    private static String getE001(String p){
        String ret = "";
        try{
            int i = Integer.parseInt(p);
            ret = (i*100+5) + "$";
        }catch (NumberFormatException e) {
            ret = p;
        }
        return "値=" + ret;
    }
}

問題点

メインメソッドから、いくつかのメソッドが呼び出されていますが、ソース上のメソッドの配置が処理の順番とあっていないため読みにくいプログラムとなっています。

これぐらいの行数であれば、それほど苦労することはありませんが、1クラスが1000行を超えるような場合、実装されている箇所が離れていたり、ステップ実行で実行箇所が激しく飛ぶとデバッグがやりにくくなり、2重実装などの発見も困難です。

解説

PL/SQLなど、言語によっては呼び出し元を先に実装できないものもありますが、各機能単位、共通メソッドなどで実装個所をまとめるようにしたほうが可読性が向上します。

改善ロジック

public class Test {
    public static void main(String[] args) {
        Random rnd = new Random();
        int ran = rnd.nextInt(10);

        String[] a001 = getA001();
        for(String a : a001){
            switch (ran) {
            case 0:
                System.out.println(getB001(a));
                break;
            case 1:
                System.out.println(getC001(a));
                break;
            case 2:
                System.out.println(getD001(a));
                break;
            case 3:
                System.out.println(getE001(a));
                break;
            default:
                break;
            }
        }
    }
    private static String[] getA001(){
        return new String[]{"1234567890","AAAAAA","G","ajslga","あいうえお","9999999999","0",""};
    }
    private static String getB001(String p){
        return = "値=" + getReturn(p);
    }
    private static String getC001(String p){
        String ret = "";
        if(p.length()>5){
            ret = p.substring(0,4) + ":" + p.substring(5); 
        }
        return "値=" + ret;
    }
    private static String getD001(String p){
        return = "値=" + getReturn(p);
    }
    private static String getE001(String p){
        String ret = "";
        try{
            int i = Integer.parseInt(p);
            ret = (i*100+5) + "$";
        }catch (NumberFormatException e) {
            ret = p;
        }
        return "値=" + ret;
    }
    private static String getReturn(String p){
        try{
            Integer.parseInt(p);
            return "\"" + p + "\"";
        }catch(NumberFormatException e){
            return p;
        }
    }
}

例8:スパゲッティ

仕様

DBよりフラグを取得し、残区分のデフォルト表示を、”0″の場合は、”0:残なし”、”1″の場合は”1:残あり”に切り替える

ロジック

.....
getDispFlg(){
    // DBよりフラグを取得する
    .....
}
.....
<?>
.....
if(${DispFlg()}{
    System.out.println("<input type=\"hidden\" name=\"check_flg\" value=\"0\">");
}else{
    System.out.println("<input type=\"hidden\" name=\"check_flg\" value=\"1\">");
}
.....
</?>
.....
getSelectFlg(){
    return request.getParameter("check_flg").toString().equals("1");
}
.....
<?>
.....
System.out.println("<select name=\"ZanKubun\">");
if(${SelectFlg()}{
    System.out.println("<option value=\"0\">残なし</option>");
    System.out.println("<option value=\"1\" selected>残数あり</option>");
}else{
    System.out.println("<option value=\"0\" selected>残なし</option>");
    System.out.println("<option value=\"1\">残数あり</option>");
}
System.out.println("</select>");
.....
</?>

問題点

JSP1画面でDBよりフラグを取得し、HIDDENで保持し、それをServlet2の呼び出しで送信し、その内容を使用してデフォルト表示の切り替えを行います。

JSP2でフラグを取得しているメソッドが”getSelectFlg”ということはJSPを見ればわかりますが、Servlet2のgetSelectFlgを見てもDBアクセスを行っていないことから、フラグの値に何が入るのかが分かりません。

JSP1以外のJSPから遷移した場合の動きも考慮すると解析にかなり時間がかかってしまいます。

残区分の初期表示に仕様変更が発生すると、どのプログラムでフラグの値を取得し、どのようにJSP2に渡すのかを決定することが困難となります。

解説

新規に作成した場合に、このような状態となることはありませんが、仕様変更が複数回行われた結果このような状態となることがあります。

例えば

作成当初:JSP1でフラグを入力していた

仕様変更1回目:JSP1でDBから取得した値を初期表示するようになった。

仕様変更2回目:JSP1の入力を行わず、DBの値でJSP2の初期表示を行うようになった。

など。

特に、仕様変更1回目、2回目で受注した会社が違うときなどは、工数をできるだけ削減するためJSP1の改修のみで済ませるため、恒久的な回収を行わないというやり方をすると、このような状態に陥ることがあります。

もし、このような改修を要求された場合は、依頼元と交渉し恒久的な対応を行ったほうが、その後の維持工数も下がりますし、改修不具合による障害についても防止できます。

改善ロジック

.....
getSelectFlg(){
    // DBよりフラグを取得する
    ......
}
.....
<?>
.....
System.out.println("<select name=\"ZanKubun\">");
if(${SelectFlg()}{
    System.out.println("<option value=\"0\">残なし</option>");
    System.out.println("<option value=\"1\" selected>残数あり</option>");
}else{
    System.out.println("<option value=\"0\" selected>残なし</option>");
    System.out.println("<option value=\"1\">残数あり</option>");
}
System.out.println("</select>");
.....
</?>

品質向上のコツ

可読性によるプログラム品質(バグ、不要ロジックの有無、作業工数など)への影響はかなり大きいと思われます。

しかし、可読性の低いロジックが実際に稼働しており、結果としてコストに反映されていることは、あまり重要視されていません。(このレベルのことを経営層が知るはずがないから)

バグが発覚時にテストエビデンスを持ち出してテストシナリオが原因などと理由付けはしますが、バグの原因がこういったところにあることが理解されないため、プログラマの苦労が経営層に理解されないのです。

今回ご紹介した内容や、諸先輩のコード、サンプルなど、あらゆるコードを自分なりに評価し、もっともよいと思われるコードの書き方を自分の形としてもつことで、可読性の高いプログラムを自然と書けるようになっていただければと思います。