関数の見通しをよくする(プログラム)

以下、メンテナンスしているコードの中にあったとある関数。(差し支えがないように少々定数名をいじっている)
Context という型から「ははあ Android か」とわかる方もいらっしゃいますよね?

public static String shurinkString(final Context context, final String str) {
    String validateStr = str;

    if (validateStr == null) {
        validateStr = "";
    } else {
        validateStr = validateStr.replace("\r\n", "\n");

        final int maxLength = context.getResources().getInteger(R.integer.text_max_length);

        if (validateStr.length() > maxLength) {
            validateStr = validateStr.substring(0, maxLength);
        }
    }

    return validateStr;
}

関数の出口がひとつ、というのは微笑ましくも好ましい。
けれど、ちょっと長い。

ということで、僕だったらどうするかをお見せする。

* * *

まず関数の途中で出てくる Context (経由でリソース)から、文字列の最大長を引き出す部分を関数の外に放り出す。

public static String shurinkString(final String str, int maxLength) {
    String validateStr = str;

    if (validateStr == null) {
        validateStr = "";
    } else {
        validateStr = validateStr.replace("\r\n", "\n");

        if (validateStr.length() > maxLength) {
            validateStr = validateStr.substring(0, maxLength);
        }
    }

    return validateStr;
}

この呼び出し箇所で context を引数に渡しているのは明白。だから引数で context の代わりに context.getResources().getInteger(R.integer.text_max_length) を渡してもらうよう変更してもらう。

* * *

つぎは純粋に好みの問題ともいえるけれど、 validateStr の再代入をやめる。
手続き型言語は代入できることが大きな特徴だけれども、これはバグの温床だ。
処理の経過に沿って変化していくオブジェクトが、同じ変数でポイントされていると混乱する。とくにループ処理が長くなるような場合に。

public static String shurinkString(final String str, int maxLength) {
    final String validateStr;

    if (str == null) {
        validateStr = "";
    } else {
        final String replaced = str.replace("\r\n", "\n");

        if (replaced.length() > maxLength) {
            validateStr = replaced.substring(0, maxLength);
        }
    }

    return validateStr;
}

* * *

次は文字列長さの最大長設定部分。
以下の if 条件で分岐している箇所。もちろんこれで問題ないけれど……

if (replaced.length() > maxLength) {

こうする。

public static String shurinkString(final String str, int maxLength) {
    final String validateStr;

    if (str == null) {
        validateStr = "";
    } else {
        final String replaced = str.replace("\r\n", "\n");
        final int length = Math.min(replaced.length(), maxLength);
        validateStr = replaced.substring(0, length);
    }

    return validateStr;
}

つまり、無条件に部分文字列(substring)を取得して返す、とした。
(わざわざ自分と同じ長さの部分文字列を返すことには無駄がある。けれど切り出す文字長が同じなら単にもとの文字列 this を返す処理系実装もある(たとえば Android の String 実装がそうだ))

* * *

さあ、 validateStr を消してみよう。

public static String shurinkString(final String string, int maxLength) {
    if (string == null) {
        return "";
    } else {
        final String replaced = string.replace("\r\n", "\n");
        final int length = Math.min(replaced.length(), maxLength);
        return replaced.substring(0, length);
    }
}

* * *

いかがだろうか。

※構造化プログラミング手法の「出口はひとつ」というマントラを破っているけれど、この程度なら許してもらえないだろうか?

この記事が気に入ったらサポートをしてみませんか?