phpmd cleancode解説

cleancode

The Clean Code ruleset contains rules that enforce a clean code base. This includes rules from SOLID and object calisthenics.

コードベースをクリーンにするルール。SOLIDの原則やオブジェクト指向エクササイズのルールも含みます。

https://phpmd.org/rules/index.html

SOLIDの原則

S 単一責任の原則（Single responsibility principle）
O 開放閉鎖の原則（Open–closed principle）
L リスコフの置換原則（Liskov substitution principle）
I インターフェイス分離の原則（Interface segregation principle）
D 依存性逆転の原則（Dependency inversion principle）

オブジェクト指向エクササイズ（Object calisthenics）

書籍「ThoughtWorksアンソロジー」の第5章で、Jeff Bay氏が提唱した9つのルールです。最初は「エクササイズ」として、1000行程度のプロジェクトで９つのルールを守ってみよう、と提案しています。矛盾するルール、ルールを適用できない場合もあるそうですが、Jeff Bay氏は実際のプロジェクト（Java）にも適用しているそうです。

1. １つのメソッドにつきインデントは1レベルまで
2. else句を使用しないこと
3. すべてのプリミティブ型と文字列型をラップすること（int型や文字列型をそのまま使わず、Hourクラスや、Moneyクラスを定義すること）
4. 1行に1アロー（ビルダーパターンのメソッドチェーンを除く）
5. 名前を省略しないこと
6. すべてのエンティティを小さくすること（1クラス50行まで、1パッケージ10ファイルまで）
7. 1つのクラスにつきメンバー変数は2つまでにすること
8. ファーストクラスコレクション（コレクションを持つクラスには、他のメンバー変数を持たせない）
9. Getter、Setter、プロパティを使用しないこと（外部からゲット、計算、セットするのではなく、オブジェクトに計算させよう）
10. （訳注によると、草稿にあったそうです）ファクトリメソッド以外のスタティックメソッドは作らないこと

BooleanArgumentFlag

メッセージ例

The method bar has a boolean flag argument $flag, which is a certain sign of a Single ResponsibilityCode language: plaintext (plaintext)

引数のbool変数でロジックを振り分けているなら、２つのメソッドに分けよう。

抑止方法

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

説明

A boolean flag argument is a reliable indicator for a violation of the Single Responsibility Principle (SRP).
You can fix this problem by extracting the logic in the boolean flag into its own class or method.

引数のboolフラグ変数を使っている場合、単一責任原則（SRP）を守っていないかもしれないね。（bookフラグ変数で振り分けている）２つのロジックを分けて、クラスやメソッドに分けよう。

コード例

class BooleanArgumentFlag
{
    public function main()
    {
        // ある場所で
        $this->saveJson(true);

        // 別の場所で
        $this->saveJson(false);
    }

    public function saveJson($json = true)
    {
        // 前処理

        if ($flag) {
            // jsonへ保存する
        } else {
            // csvへ保存する
        }

        // 後処理
    }
}Code language: PHP (php)

解説

引数がboolフラグのメソッドを全て否定しているのではないので、心配することはありません。

boolフラグをロジックの振り分けのために使っているなら、２つのロジックを別々のメソッドに分けたほうがいい場合があるから、リファクタしたほうがいいか考えよう、ということですね。

次のようなメソッドのことを言っているんだと思います。

例えば、JSONへの保存処理がすでに実装済みで、後からCSVへの保存処理が必要になったとします。

JSON保存処理を見ると、大部分を流用できそうですが、コピペはよくないですね。DRY（繰り返さない）に反しますから。

そこで、boolフラグでJSONかCSVを切り替えることにしました。このメソッドには、JSON用の処理、CSV用の処理が混在していきます。

ところが、この例は次のようにリファクタすることで、SRPにもDRYにも反しないコードになります。

もちろん、これ以外のリファクタ方法でもかまいません。

リファクタ例

class BooleanArgumentFlag
{
    public function main()
    {
        // ある場所で
        $this->saveJSON();

        // 別の場所で
        $this->saveCSV();
    }

    public function saveJSON()
    {
        $this->save(new JSONwriter());
    }

    public function saveCSV()
    {
        $this->save(new CSVwriter());
    }

    public function save($writer)
    {
        // 何か前処理

        $writer->save();

        // 何か前処理
    }
}Code language: PHP (php)

ElseExpression

メッセージ例

The method bar uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.Code language: plaintext (plaintext)

抑止方法

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

説明

An if expression with an else branch is basically not necessary.
You can rewrite the conditions in a way that the else clause is not necessary and the code becomes simpler to read.
To achieve this, use early return statements, though you may need to split the code it several smaller methods. For very simple assignments you could also use the ternary operations.

基本的にif文にelseは必要ないね。ifの条件を書き直してelseをなくそう、読みやすくなるよ。そのためには、アーリーリターンを使用すること。でも、コードを小さなメソッドに分けなければいけないだろうね。単純な割り当てなら、三項演算子を使うことができるよ。

コード例

class ElseExpression
{
    public function foo($x)
    {
        if ($x === '') {
            // 何か処理
        } else {
           // 別の処理
        }
    }
}Code language: PHP (php)

解説

オブジェクト指向エクササイズ９つのルールの一つです。本では、まずはエクササイズとして試してみよう、と控えめに言っていますが、Jeff Bay氏は、実際のプロジェクトでも使っているそうです。

（phpmdではなく）本のなかで、Jeff Bay氏は、

if 〜 elseのネストはやめて、簡潔にしよう。ネストが深くて追跡できない条件文やスクロールしなければ読めないcase文にはうんざりだよ。

else句を使わずに書き直す方法を考えてみよう、else禁止はいい練習になるよ。

早期リターン。（早期リターンは使いすぎるとわかりにくくなってしまうから注意してね）

三項演算子。

ポリモーフィズム。StrategyパターンやNullObjectパターンも試してみて。

と言っています。

なにがなんでもelse禁止、早期リターンせよ、とは「言っていない」ですよ。

複雑になった／なりそうなロジックは、else禁止にして、リファクタのアイデアを試そう、ということです。

アイデアの一つ、早期リターンは、ガード節（Guard）とペアで説明されることが多いです。

ガード節とは、nullチェックや範囲チェックのことです。あらかじめガード節でチェックしておくことで、メインロジック内でnullチェックしなくてもよくなり、メインロジックが読みやすくなります。

ベストセラー本のリーダブルコードでも「ガード節と早期リターン」が説明されています。多くの本でもガード節で早期リターンしようと言われています。

ですが、何がなんでも「ガード節と早期リターン」を使おう、ということではありません。

複雑になってしまった条件判断を少しでも紐解くために、手段の一つとして早期リターンを使おうということです。

リファクタ例

class ElseExpression
{
    public function foo($x)
    {
        if ($x === '') {
            // 何か処理
            return;
        }
      
        // 別の処理
    }
}
Code language: PHP (php)

さて、早期リターンにしてはいけないこともあります。

次の、barメソッドは、早期リターンにしてはいけない例です。

$width < 50、$width < 100、elseの３つの分岐は、メインロジックです。

if 〜 elseif 〜 else は、３つの分岐を表現します。logic 1、logic 2、logic 3のどれか一つしか通らないことがひと目でわかります。

早期リターンの書き方は、一見すっきりしたように見えます。しかし、logic 1、logic 2は例外的な処理で、メインロジックはlogic 3、と読めてしまいます。

なにより、2つのif文は独立しているので、（１つめのifの実行パス、true／falseの2個）×（２つめのifの実行パス、true／falseの2個）＝4つの実行パスがあります。logic 1 -> logic 2 -> logic 3 の3つとも通る実行パスがあるように見えるんですね。returnを見てから、３つの分岐ということがわかります。

また、もし、if ($width < 50) で returnがなかったとします。別の人が後から見たとき、returnを忘れたのか、returnしないのが正しいのか、前後のコードを追いかけて判断しなくてはなりません。

    public function bar($width)
    {
        // ★3つの分岐全体で、メインロジック
        if ($width < 50) {
            // logic 1
        } elseif ($width < 100) {
            // logic 2
        } else {
            // logic 3
        }
    }

    // 良くない例、早期リターンしてはいけない
    public function bar($width)
    {
        if ($width < 50) {
            // logic 1　　 ★例外的な処理？
                           // ★return忘れた？returnしないのが正しい？
        }

        if ($width < 100) {
            // logic 2    ★例外的な処理？
            return;
        }

        // logic 3      ★本筋のロジック？
        return;
    }Code language: PHP (php)

判断だけをメソッドに分けた例。

    public function bar($width)
    {
        $type = $this->judgeType($width);
        switch ($type) {
          case 1:
              // logic 1
              break;
          case 2:
              // logic 2
              break;
          case 3:
          default:
              // logic 3
              break;
        }
    }

    public function judgeType($width)
    {
        if ($width < 50) {
            return 1;
        } elseif ($width < 100) {
            return 2;
        } else {
            return 3;
        }
    }
Code language: PHP (php)

logic 1やlogic 2が数行なら、switchの各caseに書きます。

5行をこえるようなら、logic 1、logic 2、logic 3をメソッドに分けます。

全体として100行をこえるようなら、barメソッドやjudgeType、logic 1〜3を新しいクラスにごっそり移動します。

logic 1やlogic 2がさらに長くなりそうなら、logic 1だけで新しいクラスにします。

このセクションの最後に白状すると、筆者は、ガード節と早期リターンを使わない傾向があります。

ある本に「ベテランの開発者は、構造化プログラミングの時代に入口と出口は一つ、と学んできたから、早期リターンを使いたがらない」と書いてありました。

あー、まさに、筆者もこれです。

phpmd design GotoStatement にも早期リターンを使わない例があります。

IfStatementAssignment

メッセージ例

Avoid assigning values to variables in if clauses and the like (line '8', column '13')Code language: plaintext (plaintext)

抑止方法

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

説明

Assignments in if clauses and the like are considered a code smell.
Assignments in PHP return the right operand as their result.
In many cases, this is an expected behavior, but can lead to many difficult to spot bugs, especially when the right operand could result in zero, null or an empty string and the like.

if条件で、変数に値を代入していると、あやしい臭いがするね。PHPで、値を代入すると、式の値として右辺の値を返します。多くの場合は問題ないかもしれないけど、右辺の値がゼロ、null、空の文字列などになる場合、見つけにくいバグになるかもしれないよ。

コード例

class IfStatementAssignment
{
    public function bar()
    {
        if ($foo = $baz) {
            // ...
        }

        if ($foo = bar()) {
            // ...
        }
    }
}Code language: PHP (php)

解説

ifの条件式で変数に値を代入してはいけません。

後から読む人が、== の書き間違いなのか、本当に = で代入したいのか、判断に迷いますよ。

なお、for・whileの条件式では、変数に値を代入してもこの警告はでませんでした。

リファクタ例

代入が == の書き間違いではなく、本当に代入したい場合は、if文の前で代入するか、代入式をカッコで囲んで、0やfalseなどと比較しましょう。

class IfStatementAssignment
{
    public function bar()
    {
        $foo = $baz
        if ($foo) {
            // ...
        }
       
        if (($foo = foo()) != 0) {
            // ...
        }
    }
}Code language: PHP (php)

StaticAccess

メッセージ例

Avoid using static access to class 'Bar' in method 'bar'.Code language: plaintext (plaintext)

抑止方法

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

説明

Static access causes unexchangeable dependencies to other classes and leads to hard to test code.
Avoid using static access at all costs and instead inject dependencies through the constructor.
The only case when static access is acceptable is when used for factory methods.

Staticアクセスすると、他のクラスへ交換できなくなり、コードのテストが難しくなるよ。Staticアクセスを使わないようにして、代わりにコンストラクターで依存性（オブジェクト）を注入しよう。唯一、Staticアクセスが許されるのは、ファクトリメソッドだけだよ。

コード例

class StaticAccess
{
    public function bar()
    {
        Bar::baz();
    }
}Code language: PHP (php)

解説

「else禁止」を守らなくてもいいように、「staticメソッド禁止」も守らなくてもいいと思います。

phpmdの説明文によると、DI（Dependency Injection、依存性注入）ができないから、だそうです。それはそのとおりですが、メソッド内で、new Bar() しても、DIできなくなりますが、new Bar() は警告されませんでした。Bar::baz()はダメで、new Bar()はよい理由がわかりません。（筆者の理解不足かもしれません）

さて、DIできない、という問題について考えます。

仮にbazメソッドがインスタンスメソッドだとしても、StaticAccessクラスbarメソッドのテストケースを作るときが問題です。

Bar::bazメソッドでIO処理（ネットワークやデータベース、ファイルなど）をしていて、本番と同等の環境を用意するのが面倒であれば、モックを使うために、コンストラクタでインスタンス注入したほうがいいです。

Bar::bazメソッドでIO処理をしていない、または、Bar::bazメソッドでIO処理をしているけどDockerで用意できるので、Barクラスのモックを作る必要がないのなら、staticアクセスのままでいいでしょう。

やがて開発していくなかで、条件で機能を振り分けたくなり、ダックタイピングやStrategyパターンを使おうかなと考えるかもしれません。そのとき、必要に応じてリファクタすればいいと思います。

「ThoughtWorksアンソロジー」の訳注によると、「staticメソッド禁止」は、オブジェクト指向エクササイズの草稿にあったそうです。残念ながら、草稿は検索しても見つかりませんでした。

推測ですが、Javaではstaticメソッドとインスタンスメソッドでは継承したときの挙動が違い、DIしたときに問題になるため、このルールを作ったのかもしれません。

Javaのこの問題もあって、DIコンテナが注目されていた時期は、DIできないからstatic禁止という風潮がありました。

PHPにはこの問題はありませんが、こういうルールもある、と気には留めておきましょう。

状態をもたないstaticメソッドは、状態をもつオブジェクトよりも、テストケースを作るのが簡単です。この警告に神経質になることはないと思います。

リファクタ例

new StaticAccess(new Bar());Code language: PHP (php)

class StaticAccess
{
    private $bar;

    public function __construct($bar)
    {
        $this->bar = $bar;
    }

    public function bar()
    {
        $this->bar::baz();
    }
}Code language: PHP (php)

DuplicatedArrayKey

メッセージ例

Duplicated array key xx, first declared at line yy.Code language: plaintext (plaintext)

抑止方法

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

説明

Defining another value for the same key in an array literal overrides the previous key/value, which makes it effectively an unused code.

If it's known from the beginning that the key will have different value, there is usually no point in defining first one.

配列リテラルで同じキーを定義すると、最初のキー／値が上書きされるよ。つまり、最初の値はどこから使われないってこと。キーの値が違うとわかっているなら、最初のものを定義することはないよ。

コード例

function createArray() {
    return [
        'non-associative 0-element',
        0 => 'associative 1-element',
        false => 'associative 2-element', // ★残る
        'foo' => 'bar',
        "foo" => 'baz', // ★残る
    ];
}
var_export(createArray());

$conditions = [
  'xx !=' => 1,
  'xx !=' => 2, // ★残る
];
var_export($conditions);
Code language: PHP (php)

解説

コード例を実行すると、次の結果になります。

falseをキーにすると、キーは0になるんですね。

array (
  0 => 'associative 2-element',
  'foo' => 'baz',
)

array (
  'xx !=' => 2,
)
Code language: PHP (php)

先日、まさにこの問題で1時間ほどハマったことがありました。

CakePHPのDB検索条件のconditionsです。

新しいカラムを追加して、テストケースを作っていました。xx = 1 や xx = 2のレコードはヒットしないはずですが、xx = 1のレコードがヒットしてしまいます。

コードを見てもさっぱりわかりません。

xdebugで$conditionsを表示してみると、'xx !=' => 2 しかありません。ようやく同じキーを指定していることに気づきました。

CakePHPのconditions限定の対策ですが、キー／値を一つづつを配列でくるみます。

リファクタ例

// 正当な方法
$conditions = [
  ['xx !=' => 1],
  ['xx !=' => 2],
];

// やっつけな方法
$conditions = [
  'xx !=' => 1,
  'xx  !=' => 2,
];
Code language: PHP (php)

ErrorControlOperator

メッセージ例

Remove error control operator '@' on line xx.Code language: plaintext (plaintext)

抑止方法

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

説明

Error suppression should be avoided if possible as it doesn't just suppress the error, that you are trying to stop, but will also suppress errors that you didn't predict would ever occur.
Consider changing error_reporting() level and/or setting up your own error handler.

エラー抑制子は使用禁止。あなたが止めようとしているエラーだけでなく、あなたが予測していないエラーも止めてしまうから。error_reporting()のレベルを変更するか、独自のエラーハンドラーを設定したほうがいいよ。

コード例

function foo($vars)
{
    $x = '' . @$vars['x'];

    @unlink('foo.txt');
}Code language: PHP (php)

解説

筆者は、マップ型配列のUndefined index抑制と、unlinkによく使っていました。

Undefined index抑制の使い方は、目くじらをたてることはないと思っていますが、phpmdにかけると、この警告はでます。かといって、

 @SuppressWarnings(PHPMD.ErrorControlOperator)Code language: CSS (css)

として、phpmdの警告を抑制してしまうのも、どうも抵抗があります。

なので、phpmdをかけるのなら、isset($vars['xxx']) ? $vars['xxx'] : '' に置き換えることに落ち着きます。

unlinkの@の場合、file_exists()でファイル存在チェックすることで、ファイルが存在しないエラーは発生しなくなります。

しかし、設定ミスでファイルのwrite権限がない場合、unlinkでエラーになります。しかし、@でエラー抑制していると、エラーにならないので、設定ミスに気づきません。

unlinkでも@を使うのはよくないですね。

リファクタ例

function foo($vars)
{
    $x = isset($vars['x']) ? $vars['x'] : '';

    $path = 'foo.txt';
    if (file_exists($path)) {
        unlink('foo.txt');
    }
}
Code language: PHP (php)

MissingImport

メッセージ例

Missing class import via use statement (line 'xx', column 'yy').Code language: plaintext (plaintext)

抑止方法

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

説明

Importing all external classes in a file through use statements makes them clearly visible.

useステートメントでファイル内のすべての外部クラスをインポートしよう。（外部クラスを使っていることが）明確に示せるよ。

コード例

function foo()
{
    $x = new \Foo\Bar();
}

class X
{
    function foo()
    {
        $x = new \Foo\Bar();
    }
}Code language: PHP (php)

解説

関数内、メソッド内で、外部クラスを呼ぶ場合は、\（ルート）からnamespaceとクラス名を指定するのではなく、useで宣言してから、クラス名だけを使いましょう。

ファイル先頭のuseを見ることで、どれくらいの数の外部クラスを参照しているかがわかりやすくなります。

また、レガシープロジェクトでは、各ファイルでrequire_onceでクラスを読み込んでいて、useを使っていないので、この警告がでます。

レガシープロジェクトのクラスをcomposerのautoloadの対象にするには、なかなか手間がかかる作業です。FooBarクラスのファイル名は、FooBar.phpでなければいけません。foo_bar.php だったり、FooBar.class.phpだったりすると、地道にリネームする必要があります。

リファクタ例

use Foo\Bar;  // ★コレ

function foo()
{
    $x = new Bar();
}

class X
{
    function foo()
    {
        $x = new Bar();
    }
}Code language: PHP (php)

UndefinedVariable

メッセージ例

Avoid using undefined variables such as 'xx' which will lead to PHP notices.Code language: plaintext (plaintext)

抑止方法

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

説明

Detects when a variable is used that has not been defined before.

定義されていない変数が参照されたよ。

コード例

function foo()
{
    echo $x;
}

function bar($y)
{
    if ($y != 0) {   // $y == 0のときの$x未定義は、phpmdは検出しない
        $x = $y + 1;
    }

    echo $x;
}

function baz()
{
    $a = [];

    echo $a['x'];  // phpmdは検出しない
}Code language: PHP (php)

解説

値を設定してから、参照しましょう。

または、isset()やarray_key_exists()で値が設定済みか確認してから、参照しましょう。

未設定の変数への参照は、つまり、どの実行パスもその変数に代入していない、ということです。バグかもしれません。

未定義	phpmd	PhpStorm	error_log
foo() $xの未定義	検出した	検出した	E_NOTICE
bar() $y == 0のとき $xの未定義	しない	検出した	E_NOTICE
baz() $a['x']未定義	しない	しない	E_NOTICE

phpmdは、foo()の$xの未定義は検出します。

しかし、bar()の$y == 0の場合の$x未定義は、phpmdは検出しませんでした。PhpStormは黄色アイコンで「Variable '$x' is probably undefined」と検出しました。

baz()の$a['x']未定義は、phpmdもPhpStormも検出しませんでした。

そこで、開発環境では、error_reportingでE_NOTICEをオンにしておきます。テストケースを走らせた後、error_logにPHP Noticeがないか確認することで、検出できます。

話しはそれますが、10年以上前のPHP 5.2のころ、Smarty動的Webサイト構築入門という本を共著で出版したことがあります。amazonレビューには、undefined indexが激しい．．．という声が。

最近、そのCDROMをgithubに公開して、PHP5.6、PHP7.4でも動くようにリファクタしました。そのとき、一番多かった警告がこれでした。

リファクタ例

値を設定するバージョン

function foo()
{
    $x = 0;

    echo $x;
}

function bar($y)
{
    $x = 0;

    if ($y != 0) {
      $x = $y + 1;
    }

    echo $x;
}Code language: PHP (php)

isset()で確認するバージョン

function foo()
{
    echo isset($x) ? $x : '';
}
Code language: PHP (php)

PHP Mess Detector (phpmd)の警告の一覧

phpmdとは PHPコードの静的解析ツールです。バッドプラクティスな箇所を指摘してくれます。どこからリファクタリングすればいいかの目安にもなります。 ソースを書いた人も、レビューする人にも便利なツールです。 古くは、2012年に1.4.0...

www.ninton.co.jp

2020.11.13