複雑度を減らすには（phpmdのCyclomaticComplexityやNPathComplexityをリファクタするには）

2021.01.10

メソッドはシンプルですかー！メソッドをシンプルにすれば何でもできる！

ほんと、そうよね

CyclomaticComplexity、NPathComplexityとは

メソッドがどれくらい複雑かを数値で表すための手法です。

大ざっぱにいうと、if、while、for、switchなどの条件分岐の個数が多いほど、複雑度が高いと判定されます。

ほとんどのメソッドには条件分岐があるので、行数が長いと複雑度は高くなり、行数が短いと複雑度も低くなる傾向にあります。

CyclomaticComplexity（循環的複雑度）

メッセージ例

The xx yy() has a Cyclomatic Complexity of zz. The configured cyclomatic complexity threshold is aa.Code language: plaintext (plaintext)

抑止方法

/**
 *
 * @SuppressWarnings(PHPMD.CyclomaticComplexity)
 */Code language: PHP (php)

説明

Complexity is determined by the number of decision points in a method plus one for the method entry.
The decision points are 'if', 'while', 'for', and 'case labels'.
Generally, 1-4 is low complexity, 5-7 indicates moderate complexity, 8-10 is high complexity, and 11+ is very high complexity.

循環的複雑度は、if、while、for、caseラベルの数から計算します。
1-4は低、5-7は中、8-10は高、11+は非常に複雑です。

NPathComplexity（実行パス数）

メッセージ例

The xx yy() has an NPath complexity of zz. The configured NPath complexity threshold is aa.Code language: plaintext (plaintext)

抑止方法

/**
 *
 * @SuppressWarnings(PHPMD.NPathComplexity)
 */Code language: PHP (php)

説明

The NPath complexity of a method is the number of acyclic execution paths through that method.
A threshold of 200 is generally considered the point where measures should be taken to reduce complexity.

NPath複雑度は、実行パス数のこと。
200をこえていたら、かなり複雑。
（リファクタして）複雑さを軽減したほうがいいよ。

for文、foreach文のリファクタ

まず、メソッド内が空の場合、CyclomaticComplexity=1、NPathComplexity=1なので、どちらも最小値は1です。

次のコード例のfooメソッド、barメソッドのどちらも、CyclomaticComplexity=2、NPathComplexity=2です。

    // CyclomaticComplexity=2、NPathComplexity=2
    public function foo(array $arr)
    {
        foreach ($arr as $i => $val) {
            $arr[$i] = $val * 2;
        }
    }

    // CyclomaticComplexity=2、NPathComplexity=2
    public function bar(array $arr)
    {
        for ($i = 0; $i < count($arr); $i += 1) {
            $arr[$i] = $arr[$i] * 2;
        }
    }
Code language: PHP (php)

つまり、for文（またはforeach文）を１つ追加すると、CyclomaticComplexity、NPathComplexityどちらも、1増えます。

逆に考えると、for文（またはforeach文）を１つ削除することで、CyclomaticComplexity、NPathComplexityどちらも1減らすことができます。

聞いたこともないサイクロなんとかの数値を下げるためにリファクタしても、アプリケーションの動きは変わらないよ？

リファクタって、そういうものでしょ

for文、foreach文で配列の要素を回しているなら、array_walk、array_map、array_reduceを使えないか考えましょう。

array_walk

array_walkは、配列の要素を変更している場合に使います。array_mapでも同じことができます。

    // 各要素の値を2倍する
    for ($i = 0; $i < count($arr); $i += 1) {
        $arr[$i] = $arr[$i] * 2;
    }
Code language: PHP (php)
    // array_walkバージョン
    array_walk($arr, function (&$val) {
        $val = $val * 2;
    });

    // array_mapバージョン
    $arr = array_map(function ($val) {
        return $val * 2;
    }), $arr);Code language: PHP (php)

array_map

array_mapは、配列の要素から、新しい要素を計算して、新しい配列を返します。

    // 'id'値の配列をつくる
    $data_arr = [
        [ 'id' => 1, 'name' => 'foo' ],
        [ 'id' => 2, 'name' => 'bar' ],
    ];

    $id_arr = [];
    foreach ($data_arr as $data) {
        $id_arr[] = $data['id'];
    }

    // $id_arr = [ 1, 2 ]Code language: PHP (php)
    $id_arr = array_map(function ($data) {
        return $data['id'];
    }, $data_arr);
Code language: PHP (php)

array_reduce

array_reduceは、ほとんどのfor/foreach処理を置き換えることができます。

array_sum、array_filterやarray_mapなど、目的に合った関数がすでにあれば、それらを使います。なければ、array_reduceを使います。

foreachで、2より小さい要素の個数、4より小さい要素の個数を数える例です。

    // 2より小さい要素の個数、4より小さい要素の個数を数える
    $arr = [1, 2, 3, 4, 5, 6];

    $a = 2;
    $b = 4;
    $a_cnt = 0;
    $b_cnt = 0;

    foreach ($arr as $val) {
        $a_cnt += ($val < $a) ? 1 : 0;
        $b_cnt += ($val < $b) ? 1 : 0;
    }

    // $a_cnt = 1
    // $b_cnt = 3Code language: PHP (php)

まず、foreach内で使う変数の初期値を配列でラップします。これが array_reduceに渡す初期値 $initial であり、初回のコールバック関数に渡される $carry です。

    $initial = [
        'a' => 2,
        'b' => 4,
        'a_cnt' => 0,
        'b_cnt' => 0,
    ];
Code language: PHP (php)

foreach内の2行の処理を関数にします。配列の要素は2つめの引数$valで受け取ります。それ以外の変数は、1つめの引数$carryで受け取ります。

$carryを変更した／しないにかかわらず、$carryをreturnします。最後の要素でreturnする$carryが、array_reduceの返り値です。

    function ($carry, $val) {
        $carry['a_cnt'] += ($val < $carry['a']) ? 1 : 0;
        $carry['b_cnt'] += ($val < $carry['b']) ? 1 : 0;

        return $carry;
    }Code language: PHP (php)

array_reduceを使って、まとめると

    // 2より小さい要素の個数、4より小さい要素の個数を数える
    $arr = [1, 2, 3, 4, 5, 6];

    $initial = [
        'a' => 2,
        'b' => 4,
        'a_cnt' => 0,
        'b_cnt' => 0,
    ];

    $results = array_reduce($arr, function ($carry, $val) {
        $carry['a_cnt'] += ($val < $carry['a']) ? 1 : 0;
        $carry['b_cnt'] += ($val < $carry['b']) ? 1 : 0;

        return $carry;
    }), $initial);

    // $results['a_cnt'] = 1
    // $results['b_cnt'] = 3Code language: PHP (php)

for／foreach内でbreakしている場合も、array_reduceで置き換えることができます。

    // 3が見つかるまでの個数
    $arr = [1, 3, 4, 5, 6];

    $a = 3;
    $cnt = 0;
    foreach ($arr as $val) {
        $cnt += 1;
        if ($val === $a) {
            break;
        }
    }
    // $cnt = 2;Code language: PHP (php)
    // 3が見つかるまでの個数
    $arr = [1, 3, 4, 5, 6];

    $carry = [
        'a' => 3,
        'cnt' => 0,
        'found' => false,
    ];

    $results = array_reduce($arr, function ($carry, $val) {
        if (!$carry['found']) {
            $carry['cnt'] += 1;
            $carry['found'] = ($carry['a'] === $val);
        }

        return $carry;
    }, $carry);

    // $results['cnt'] = 2
    // $results['found'] = trueCode language: PHP (php)

switch文のリファクタ

CyclomaticComplexityやNPathComplexityは「switch」ではなく「case」と「default」のラベルの個数を数えます。caseやdefaultは合わせて3〜4個以上あることが多いので、switch文があると、CyclomaticComplexityもNPathComplexityもすぐに値がはね上がってしまいます。

ネストが深いif文や、長いif 〜 else if をswitchで置き換えて読みやすくリファクタしても、複雑度の数値はおそらく下がりません。読みやすくしたことで、変更やリファクタをしやすくなったので、switch文への置き換えで、いったんリファクタ完了としてもいいと思います。

ここで説明するのは、switch文を関数テーブルに変形していく方法です。関数テーブルを使えば、複雑度の数値を下げることができますが、switch文と関数テーブルのどちらが読みやすいかは、メソッドの処理にもよりますし、人にもよります。何がなんでもswitch文を関数テーブルに置き換えることを推奨しているわけではありません。

（１）元の状態です。

    // CyclomaticComplexity=7、NPathComplexity=6
    public function foo()
    {
        switch ($a) {
            case 1:
            case 2:
                // 処理1
                break;
            case 3:
            case 4:
                // 処理2
                break;
            case 5:
            case 6:
                // 処理3
                break;
        }
    }Code language: PHP (php)

（２）処理１から処理３を別メソッドにします。

処理1〜処理3にif文やfor文がある場合、処理1〜処理3を別メソッドにすることで、元のメソッドのCyclomaticComplexityやNPathComplexityも減ります。

また、switch文が縦に短くなるので、とても読みやすくなります。

スクロールしなければ読めないcase文にはうんざりだよ。

「ThoughtWorksアンソロジー」第5章　オブジェクト指向エクササイズ Jeff Bay氏
    // CyclomaticComplexity=7、NPathComplexity=6
    public function foo()
    {
        switch ($a) {
            case 1:
            case 2:
                $this->process1();
                break;
            case 3:
            case 4:
                $this->process2();
                break;
            case 5:
            case 6:
                $this->process3();
                break;
        }
    }

    public function process1()
    {
        // 処理1
    }Code language: PHP (php)

次に、関数テーブルの形式に変形していきます。

（３）中間変数の導入

いくつかのcaseが同じ処理をしている場合、中間変数を導入します。

この例では、case 1とcase 2がどちらも処理1を読んでいるので、中間変数=1にまとめます。

同じように、case 3とcase 4は中間変数=2、case 5とcase 6は中間変数=3にまとめます。

本来の$a から中間変数$bへの変換は、変換用の配列を用意しておきます。

    // CyclomaticComplexity=4、NPathComplexity=3
    public function foo()
    {
        $b = $this->getCase($a);
        switch ($b) {
            case 1:
                // 処理1
                break;
            case 2:
                // 処理2
                break;
            case 3:
                // 処理3
                break;
        }
    }
    
    // CyclomaticComplexity=2、NPathComplexity=2
    public function getCase($a)
    {
        $b = [
            1 => 1,
            2 => 1,
            3 => 2,
            4 => 2,
            5 => 3,
            6 => 3,
        ];

        if (isset($map[$a])) {
            return $map[$a];
        }
        return 0;
    }
Code language: PHP (php)

（４）処理のswitch文を配列におきかえます

中間変数$bと各処理のswitch文も、配列に定義します。これが関数テーブルです。javascriptっぽいですね。

    // CyclomaticComplexity=2、NPathComplexity=2
    public function foo()
    {
        $map = [
            // ここの各関数の引数並びは、$map[$b]()と揃えること
            1 => function () {
                // 処理1
            },
            2 => function () {
                // 処理2
            },
            3 => function () {
                // 処理3
            },
        ];

        $b = $this->getCase($a);
        if (isset($map[$b])) {
            $map[$b]();
        }

        return 0;
    }Code language: PHP (php)

リファクタ後の複雑度は、fooメソッドとgetCaseの複雑度を合計します。

リファクタfoo()
Cyclomatic
Complexity		foo()
NPath
Compexity		getCase()
Cyclomatic
Complexity		getCase()
NPath
Compexity		Cyclomatic
Complexity
合計		NPath
Compexity
合計
（２）リファクタ前7676
（３）中間変数の導入4 32265
（４）switch文と処理を配列でおきかえ222244

（４）のリファクタ後は、CyclomaticComplexity=4、NPathCompelxity=4です。

注目すべきは、元の処理のcaseが増えても（４）の複雑度は増えないことです。

注意点は（４）の各処理の関数は、$map[$b]()として呼ばれるので、これと同じ引数並びで揃える必要があります。

ここでは、クロージャを関数テーブルにしましたが、メソッドを関数テーブルにしたり、クラスをfactoryする場合もあります。

リファクタ前のswitch文のほうが読みやすかったよ

そうかもね。テストケースを書いて、決定表をコメントに残しておいたらどうかしら

たしかに、リファクタ前のswitch文のほうがわかりやすいと感じるかもしれません。次のような決定表をコメントやIssueに残しておきます。

    /**
     *  | $a    | 処理  |
     *  | ----- | ----- |
     *  | 1     | 処理1 |
     *  | 2     | 処理1 |
     *  | 3     | 処理2 |
     *  | 4     | 処理2 |
     *  | 5     | 処理3 |
     *  | 6     | 処理3 |
     */
    public function foo()
    {Code language: PHP (php)

関数テーブルもリファクタしたいけど

関数テーブルに、似たようなメソッド名、クラス名が並んでいると、関数テーブルもリファクタしたくなりますよね。

呼び出すメソッド名やクラス名に規則性をもたせて、

public function foo_case1()
public function foo_case2()
public function foo_case3()

class FooCase1
class FooCase2
class FooCase3Code language: PHP (php)

関数テーブルを使わずに、式で生成することができます。

$func = 'foo_case' . $a;
if (function_exists($func)) {
    $func();
}

$class = 'FooCase' . $a;
if (class_exists($class)) {
    $obj = new $class();
}Code language: PHP (php)

プラグインでもないのに、ここまでやったほうがいいのか、よく考えたほうがいいです。

１つめの問題は、選択されたメソッドやクラスがなくてもエラーになりません。

何かしらエラーを返すとしても、入ってきた選択肢がありえない値で入ってきたのか、選択肢は正しいのに、メソッドやクラスを用意し忘れたか（メソッド名をクラス名をタイポしてしまったのか）がわかりません。

２つめの問題は、メソッド名やクラス名の定義側でどこで使っているか調べるために、メソッド名やクラス名で検索しても、どこで使っているかわからないことです。

useでクラスを指定してあれば、IDEのfind usageや全ファイル検索で検索できます。

しかし、そのクラスは使われていないので、use行はグレー表示です。うっかり、削除してしまうかもしれませんし、新しい選択肢のクラスを追加するとき、useを記述しないかもしれません。

<?php
namespace Ninton\page4;

use Ninton\page4\plugins\Foo1; // グレー表示で、使われていないので、削除してしまうかも
use Ninton\page4\plugins\Foo2;

class Page4
{
    public function foo($a)
    {
        $class = 'Ninton\page4\plugins\Foo' . $a;
        if (class_exists($class)) {
            $obj = new $class();
            $obj->baz();
        }
    }
}
Code language: PHP (php)

include_once/require_onceでクラスを読み込んでいる場合は、クラス名で検索してもヒットしません。クラス名の一部、"Foo" で検索しなければわかりません。

<?php
namespace Ninton\page4;

class Page4
{
    public function foo($a)
    {
        $class = "Foo" . $a;
        $path = __DIR__ . "/plugins/$class.php";
        if (file_exists($path)) {
            include_once $path;
        }
        if (class_exists($class)) {
            $obj = new $class();
            $obj->baz();
        }
    }
}
Code language: PHP (php)

if文のリファクタ

ここで説明するのは、if文を関数テーブルに変形していく方法です。関数テーブルを使えば、複雑度の数値を下げることができますが、if文と関数テーブルのどちらが読みやすいかは、メソッドの処理にもよりますし、人にもよります。何がなんでもif文を関数テーブルに置き換えることを推奨しているわけではありません。

長いif文を関数テーブルに変形していきます。

    // CyclomaticComplexity=6、NPathCompelxity=6
    public function foo()
    {
        // この時点で、$aと$bを決定できるかどうか？

        if ($a < 10) {
            // 処理1
        } elseif ($a < 20) {
            // 処理2
        } elseif ($a < 30) {
            // 処理3-1
            if ($b < 10) {
                // 処理3-2
            } else {
                // 処理3-3
            }
        } elseif ($a < 40) {
            // 処理4
        }
    }
Code language: PHP (php)

処理3-1の結果として、$bが決まる場合は、処理3-1〜処理3-2〜処理3-3の範囲を「処理3」とみて、別メソッドに分けてから、リファクタします。

    // CyclomaticComplexity=5、NPathCompelxity=5
    public function foo()
    {
        if ($a < 10) {
            // 処理1
        } elseif ($a < 20) {
            // 処理2
        } elseif ($a < 30) {
            $this->process3(); // 処理3
        } elseif ($a < 40) {
            // 処理4
        }
    }

    // CyclomaticComplexity=2、NPathCompelxity=2
    public function process3()
    {
        // 処理3-1
        if ($b < 10) {
            // 処理3-2
        } else {
            // 処理3-3
        }
    }
Code language: PHP (php)

ここでは、fooメソッドのif文の前に、$a、$bも決まるものとして、リファクタします。

まず、条件判断を別メソッドgetCaseに分けます。

$aや$bをIO処理から取得する場合、getCaseにIO処理オブジェクトを渡してしまうと、getCaseのユニットテストにIO処理オブジェクトかモックが必要になります。IO処理から値を取得してから、getCaseに値を渡すようにします。

残念ながら、複雑度は減りませんが、このgetCaseメソッドのユニットテストを書きやすくなりました。ユニットテストで充分なテストケースを書いておきましょう。

    // CyclomaticComplexity=6、NPathCompelxity=6
    public function getCase($a, $b)
    {
        if ($a < 10) {
            return 10;
        } elseif ($a < 20) {
            return 20;
        } elseif ($a < 30) {
            if ($b < 10) {
                return 32;
            } else {
                return 33;
            }
        } elseif ($a < 40) {
            return 40;
        }

        return 0;
    }
Code language: PHP (php)

ifの条件式が長い場合は、条件式を別メソッドにして、条件式のユニットテストを書きます。

    public function getCase($a, $b)
    {
        if ($this->isCase10($a)) {
            return 10;
        } elseif ($this->isCase20($a)) {
            return 20;
        } elseif ($this->isCase32($a, $b)) {
            return 32;
        } elseif ($this->isCase33($a, $b)) {
            return 33;
        } elseif ($this->isCase40($a)) {
            return 40;
        }

        return 0;
    }

    public function isCase10($a)
    {
        return $a < 10;
    }

    public function isCase20($a)
    {
        return $a < 20;
    }

    public function isCase32($a, $b)
    {
        return $a < 30 && $b < 10;
    }

    public function isCase33($a, $b)
    {
        return $a < 30 && !($b < 10);
    }

    public function isCase40($a)
    {
        return $a < 40;
    }
Code language: PHP (php)

最後に、if文を配列で置き換えます。fooメソッドの行数が長くなってしまう場合は、fooメソッド自体を別クラスにしたり、処理1、処理2の本体を、別メソッドや別クラスに実装します。

    // CyclomaticComplexity=2、NPathCompelxity=2
    public function foo()
    {
        $map = [
            10 => function () {
                // 処理1
            },
            20 => function () {
                // 処理2
            },
            32 => function () {
                // 処理3-1
                // 処理3-2
            },
            33 => function () {
                // 処理3-1
                // 処理3-3
            },
            40 => function () {
                // 処理4
            },
        ];

        $c = $this->getCase($a, $b);
        if (isset($map[$c])) {
            $map[$c]();
        }
    }
Code language: PHP (php)

タイトルとURLをコピーしました