phpmd design警告の解説

design

design

The Design Ruleset contains a collection of rules that find software design related problems.

ソフトウェアデザイン（設計）のルールです。

ExitExpression

メッセージ例

The xx yy() contains an exit expression.Code language: plaintext (plaintext)

抑止方法

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

説明

An exit-expression within regular code is untestable and therefore it should be avoided.
Consider to move the exit-expression into some kind of startup script where an error/exception code is returned to the calling environment.
exit()はテストできないので、禁止。
exit()の代わりに、ErrorまたはExceptionをスローして、呼び出し元に戻ろう。

コード例

class Foo {
    public function bar($param)  {
        if ($param === 42) {
            exit(23);
        }
    }
}Code language: PHP (php)

解説

メソッド内で、exit()やdie()すると、テスト全体がそこで終了してしまいます。

Exceptionをスローしても、MyAppExitExceptionを用意してスローしても、途中で catch (Exception $e)があると、そこでキャッチされてしまうので、呼び出し元に戻すのは簡単ではないかもしれません。

その場合は、対象のメソッドを、bar()とbar_body()の2段階構成にします。bar_bodyメソッドでExceptionをスローするようにしておき、bar_bodyメソッドをテストします。

barメソッドでは、Exceptionをcatchしたら、これまでどおりexitします。

この警告と関連しますが、フレームワークのredirect()もメソッド内部でexitしてしまい、呼んだメソッドに戻ってきません。そこで、redirect()メソッドは、エンドポイントのメソッドでだけ呼ぶようにします。

リファクタ例

class Foo
{
    public function bar($param)
    {
        if ($param === 42) {
            throw new Exception("23");
            // または
            throw new Error("23");
        }
    }
}

// 上位にthrowできないとき
class Foo
{
    public function bar($param)
    {
        try {
          $this->bar_body($param);
        } catch (Exception $e) {
          $ret_code = intval($e->getMessage());
          exit($ret_code);
        }
    }

    public function bar_body($param)  {
        if ($param === 42) {
            throw new Exception("23");
            // または
            throw new Error("23");
        }
    }
}Code language: PHP (php)

フレームワークを使っていたら、exitやdieを使うことはないよね

EvalExpression

メッセージ例

The xx yy() contains an eval expression.Code language: plaintext (plaintext)

抑止方法

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

説明

An eval-expression is untestable, a security risk and bad practice.
Therefore it should be avoided.
Consider to replace the eval-expression with regular code.

eval()はテストできないし、セキュリティリスクがあるし、バッドプラクティス。
eval()は禁止。
eval()を普通のコードに置き換えよう。

コード例

class Foo {
    public function bar($param)  {
        if ($param === 42) {
            eval('$param = 23;');
        }
    }
}Code language: PHP (php)

解説

PHPサイトのeval説明ページに、非常に危険、とあります。

警告 eval() は非常に危険な言語構造です。というのも、任意の PHP コードを実行できてしまうからです。 これを使うことはおすすめしません。 いろいろ検討した結果どうしても使わざるを得なくなった場合は、細心の注意を払って使いましょう。 ユーザーから受け取ったデータをそのまま渡してはいけません。 渡す前に、適切な検証が必要です。
https://www.php.net/manual/ja/function.eval.php

学習のためにいろいろ試すのはありですが、本番用コードでは使わないようにしましょう。ユーザが悪意のあるコードを自由に実行できるということです。

eval("system('cat /etc/passwd');");Code language: JavaScript (javascript)

を実行されたら、たいへんです！

見られたくないファイルを見られちゃうね！

データベースだって見られちゃうわよ

GotoStatement

メッセージ例

The xx yy() utilizes a goto statement.Code language: plaintext (plaintext)

抑止方法

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

説明

Goto makes code harder to read and it is nearly impossible to understand the control flow of an application that uses this language construct.
Therefore it should be avoided. Consider to replace Goto with regular control structures and separate methods/function, which are easier to read.

gotoのコードは読みにくいし、gotoを使った制御フローを理解することは、ほぼ不可能です。
goto禁止。
gotoを、読みやすい通常の制御構造に置き換え、メソッド/関数に分けてください。

コード例

class Foo {
    public function bar($param)  {
        A:
        if ($param === 42) {
            goto X;
        }
        Y:
        if (time() % 42 === 23) {
            goto Z;
        }
        X:
        if (time() % 23 === 42) {
            goto Y;
        }
        Z:
        return 42;
    }
}Code language: PHP (php)

解説

意外なことにPHP5.3でgotoが導入されました。

（elseのない）if文とgotoだけで、if〜elese文、while文、for文、do〜while文と同等のフローを実装してみるのは勉強になるでしょう。

しかし本番では、goto禁止です。

PHPでgotoの使いどころは、ただ一つ、ネストした深いfor文から抜けるときだけです。おそらく、そのコードも、メソッドに分けて、ネストしたfor文からreturnするようにリファクタできます。

PHPでgoto文は見たことないよ

ねんのためってことだと思うわ

AppleのSSLバグ（CVE-2014-1266）は、"goto fail bug" とも呼ばれています。しかし、これはgotoが原因というよりは、if文に波カッコがないことのほうが、バグに気づけなかった原因のように思います。

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
    OSStatus        err;
    ...
 
    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
        goto fail;
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
        goto fail;
        goto fail;
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
        goto fail;
    ...
 
fail:
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;
}
Code language: C/AL (cal)

インデントしてあるので、次のように見えてしまったのかもしれません。2つめのgoto failは到達しないはずだから、放置しておいても問題ないだろうと思えたんでしょうね。

    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
        goto fail;
        goto fail;
    }
Code language: C/AL (cal)

さて、gotoを使わず、判断処理を関数に分けたとします。波カッコなしなので、さきほどのgotoのコードと同じように見えます。2つめのreturnに気づけるかどうか．．．コードを見ていても気づけないかもしれません。（しかし gotoのときとは違って、IDEやコンパイラが6行目以降に到達しないと警告してくれるので、気づきますね）

    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
        return -1;
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
        return -1;
        return -1;
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
        return -1;

    return 0;
Code language: C/AL (cal)

常にif文に波カッコをつけていたなら、2つめのgoto fail; に気づけたかもしれません。

    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) {
        goto fail;
    }
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
        goto fail;
    }
        goto fail;
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
        goto fail;
    }

Code language: C/AL (cal)

もう一歩すすめて、if 〜 else if 〜にします。こう書くと、ifとifの間に、2つめのgotoやreturnを書く余地がありません。

    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) {
        goto fail;
    } else if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
        goto fail;
    } else if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
        goto fail;
    }
Code language: C/AL (cal)

このように考えていくと、一行のif文にも波カッコをつけるようになり、やがて一行で書かなくなり、

/* 波カッコなし */
    if (xxx) x = 1;
    if (yyy) y = 1;

/* 波カッコあり */
    if (xxx) { x = 1; }
    if (yyy) { y = 1; }

/* 1行で書かない */
    if (xxx) {
        x = 1;
    }
    if (yyy) {
        y = 1;
    }
Code language: plaintext (plaintext)

そして、continue や break 、早期リターンをなるべく使わないようになるんです。

gotoの説明だったのに

if文の話しに飛んでいるわね

NumberOfChildren

メッセージ例

The xx yy has zz children. Consider to rebalance this class hierarchy to keep number of children under aa.Code language: plaintext (plaintext)

抑止方法

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

説明

A class with an excessive number of children is an indicator for an unbalanced class hierarchy. You should consider to refactor this class hierarchy.
子の数が多すぎるクラスは、クラス階層のバランスが悪いかもしれない。
クラス階層をリファクタリングを検討してみて。
子クラスは15未満（最大14個）

解説

子クラスが多いということは、それぞれの子クラスで、少しづつメソッドが違うんですね。メソッドの振る舞いの違いを取り出すことができれば、子クラスを減らすことができます。

リファクタの参考に、HeadFirstデザインパターンの第1章、第3章を紹介します。

第1章では、鴨シミュレーションゲームのDuckクラスを設計します。最初は、鳴き方の違いでサブクラス化していました。仕様追加で、飛ぶ飛ばないが仕様に追加されて、サブクラスは増え、サブクラスごとに修正しなくてはなりませんでした。

この鴨ゲームでは、振る舞いの違いをStrategyパターンで解決しました。

第3章では、スターバズコーヒーのメニューを設計します。抽象クラスBeverageをスーパークラスとして、あらゆるメニューをサブクラス化して、クラス爆発していました。

スターバズコーヒーのクラス階層は、Decoratorパターンで解決しました。

登場人物のふざけたセリフがときどき的を得てるんだよね

人によっては好きになれないみたいね

DepthOfInheritance

メッセージ例

The xx yy has zz parents. Consider to reduce the depth of this class hierarchy to under aa.Code language: plaintext (plaintext)

抑止方法

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

説明

A class with many parents is an indicator for an unbalanced and wrong class hierarchy.
You should consider to refactor this class hierarchy.

（クラス階層が深すぎるとき）クラス階層が悪く、アンバランスかもしれない。
クラス階層をリファクタリングしたほうがいいかもしれないね。
親ー子で1階層、親ー子ー孫で2階層、最大5階層まで。

解説

親クラスのメソッドAに少し機能追加するために、子クラスを作りました。

子クラスのメソッドAに、また少し機能追加するために、孫クラスを作りました。

もし、そのような経緯で、深い階層の子クラスを作っていったのなら、HeadFirstデザインパターンの第3章、スターバズコーヒーのメニューにDecoratorパターンを適用した章が参考になるかもしれません。

オブジェクトタウンへようこそ、だったかしら

CouplingBetweenObjects

メッセージ例

The class xx has a coupling between objects value of yy. Consider to reduce the number of dependencies under zz.Code language: plaintext (plaintext)

抑止方法

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

説明

A class with too many dependencies has negative impacts on several quality aspects of a class.
This includes quality criteria like stability, maintainability and understandability

多くの依存関係があるクラスは、クラスの品質面に悪い影響があります。
品質基準とは、安定性、保守性、理解性などがあります。
（13未満、最大12クラスまで）

コード例

class Foo {
    /**
     * @var \foo\bar\X
     */
    private $x = null;

    /**
     * @var \foo\bar\Y
     */
    private $y = null;

    /**
     * @var \foo\bar\Z
     */
    private $z = null;

    public function setFoo(\Foo $foo) {}
    public function setBar(\Bar $bar) {}
    public function setBaz(\Baz $baz) {}

    /**
     * @return \SplObjectStorage
     * @throws \OutOfRangeException
     * @throws \InvalidArgumentException
     * @throws \ErrorException
     */
    public function process(\Iterator $it) {
        /* @var $x AA3 */
        $x = null;

        new AA1();
        AA2::hoge();
    }
}
Code language: PHP (php)

解説

newやstaticアクセス、メンバー変数の型、引数の型、戻り値、例外、ローカル変数などの型について、phpdocを含めて、他クラスへの参照をカウントします。

composerでサードパーティパッケージをrequireして、各種クラスを使っていると、すぐに13をこえてしまいますね。

リファクタの余地はあると思いますが、放置でもいいと思います。

じゃあ放置しちゃおうっと

常に改善してね！

DevelopmentCodeFragment

メッセージ例

The xx yy() calls the typical debug function zz() which is mostly only used during development.Code language: plaintext (plaintext)

抑止方法

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

説明

Functions like var_dump(), print_r() etc. are normally only used during development and therefore such calls in production code are a good indicator that they were just forgotten.

var_dump、print_rなどの関数は、開発中にのみ使用されます。
本番コードにこれらがあるということは、開発中コードが忘れられて残ったままということです。

コード例

class SuspectCode {

    public function doSomething(array $items)
    {
        foreach ($items as $i => $item) {
            // …

            if ('qafoo' == $item) var_dump($i);

            // …
        }
    }
}
Code language: plaintext (plaintext)

解説

デバグコードには次のものがあります。コミットする前に、デバグコードを削除して、本番コードに戻しましょう。

（１）var_dump、print_r

（２）if文内に強制的に入るため、条件に true を書く

// 本番
if ($x < $y) {

// デバグ中（if文に入りたい）
if (true || $x < $y) {  // TODO デバグ後 true 削除
Code language: PHP (php)

（３）変数にリテラルの固定値を代入する

// 本番
$a = $x + $y;
$this->foo($a, $b);

// デバグ中（$a = 50をfooメソッドに渡したい）
$a = $x + $y;
$a = 50;  // TODO デバグ後削除
$this->foo($a, $b);

Code language: PHP (php)

（４）本番コードをコメントにする。
その行を実行したとき、実行しなかったときの違いを調べるため

// 本番
$a = $x + $y;
$this->foo($a, $b);

// デバグ中（原因調査のため、fooをコメント）
$a = $x + $y;
// $this->foo($a, $b);  // TODO コメントをはずすこと

Code language: PHP (php)

デバグコードには、// TODO をつけておくと、IDEのTODO一覧に残るので、忘れにくくなります。しかし、プログラマの善意に依存しているので、あまりあてにはできません。

xdebugを使えないときは、var_dump書いちゃうね

var_dumpをコミットしないように気をつけてね

EmptyCatchBlock

メッセージ例

Avoid using empty try-catch blocks in xx.Code language: plaintext (plaintext)

抑止方法

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

説明

Usually empty try-catch is a bad idea because you are silently swallowing an error condition and then continuing execution.
Occasionally this may be the right thing to do, but often it's a sign that a developer saw an exception, didn't know what to do about it, and so used an empty catch to silence the problem.

通常、空のtry-catchはよくない考えです。エラーを黙って飲み込み、実行を継続するためです。
場合によっては、これで正しいかもしれません。
しかし、ほとんどは、開発者が例外を認識しているが、どう処理をすればいいかわからなかったので、空のキャッチで問題を黙殺したということです。

コード例

class Foo
{
    public function bar()
    {
        try {
            // ...
        } catch (Exception $e) {} // empty catch block
    }
}Code language: PHP (php)

解説

それは耳タコだよ。具体的にどうすればいいのか教えてよ😥

そうね、ほとんどの記事では「サンプルなのでエラー処理は省略します、実際にはエラー処理してください」ばかりよね。みんな悩んでいるのよ

例外をにぎりつぶしてもアプリは動いているんだから、それでいいんじゃないの？😛

にぎりつぶすのはいいけど、クラッシュレポートに送信しておいたらどうかしら？

ちゃんとやろうとすると、メッセージを組み立てるだけで、結構な行になるよ

catchしたなかで、新しいExceptionを組み立てて、再スローしていると、なんかさ、むだなことをやっている気がしてくるよ

try〜catchでも、戻り値でも、エラー処理は難しいのね🤔

CountInLoopExpression

メッセージ例

Avoid using xx() function in yy loops.Code language: plaintext (plaintext)

抑止方法

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

説明

Using count/sizeof in loops expressions is considered bad practice and is a potential source of many bugs, especially when the loop manipulates an array, as count happens on each iteration.

forループ内で、配列の長さを変化させているのかな？だから、繰り返しの条件のたびにcount()で配列の長さを調べているんだよね。forループ内で、配列の長さを変更するのは、バグの原因だから、やめたほうがいいよ。

コード例

class Foo {

  public function bar()
  {
    $arr = array();

    for ($i = 0; count($arr); $i += 1) {
      // ...
    }
  }
}Code language: PHP (php)

解説

forループ内で、配列の長さが変わらないなら、警告を無視してもかまいません。

対策するのなら、forループに入る前に、配列の長さを求めておくか、

  $cnt = count($arr);
  for ($i = 0; $i < $cnt; $i += 1) {Code language: PHP (php)

foreach文を使います。

  foreach ($arr as $i => $val) {Code language: PHP (php)

もし、forループ内で、配列の長さが変わるような処理をしているのなら、リファクタしましょう。

新しい配列にコピー、追加するようにします。

  $outArr = [];
  foreach ($arr as $i => $val) {
    if ($val == 'x') {
        $outArr[] = '---';
    }
    $outArr[] = $val;
  }
Code language: PHP (php)

forループは、array_mapでリファクタできることが多いです。次の２点を紹介します。

（１）配列$arr[$i]の値を変更したいとき

  for ($i = 0; $i < count($arr); $i += 1) {
    $arr[$i] = $arr[$i] * 2;
  }Code language: PHP (php)

  $arr = array_map(function ($item) {
    return $item * 2;
  }, $arr);Code language: PHP (php)

（２）複数の配列を添字で参照するとき

  $a_arr = 何か;
  $b_arr = 何か;
  $c_arr = 何か;

  for ($i = 0; $i < count($a_arr); $i += 1) {
    $c_arr[$i] += myfunc($a_arr[$i], $b_arr[$i]);
  }Code language: plaintext (plaintext)

  $a_arr = 何か;
  $b_arr = 何か;
  $c_arr = 何か;

  $c_arr = array_map(function ($a, $b, $c) {
    return $c + myfunc($a, $b);
  }, $a_arr, $b_arr, $c_arr);
Code language: plaintext (plaintext)

for文の中身だけもすぐに長くなっていくよね

for文の中身だけをメソッドに分けるといいのよ