モンスターメソッド（phpmdのExcessiveMethodLengthをリファクタするには）

いつメソッドを短くするの？

今でしょ

ExcessiveMethodLengthとは
モンスターメソッド
メソッドの成長速度を遅くする
メソッドに分ける範囲
メソッドを新しいメソッドに分ける
メソッドを新しいクラスにする

ExcessiveMethodLengthとは

メソッドの行数が多すぎる（縦に長い）という警告です。

最初にリファクタしてほしい警告であり、常に気をつけてほしい警告です。

phpmdの既定値は100行ですが、まだまだ長すぎです。

「ThoughtWorksアンソロジー」の第5章で、Jeff Bay氏が提唱した「オブジェクト指向エクササイズ」のルール1は、もっと厳しくて、（メソッドではなく）クラスが50行までです。

例えばね、ケント・ベックが長いメソッドを書いているの、想像できる？

たまには書くかもしれないよ

じゃあね、ケント・ベックが長いメソッドのまま、デザインパターンを使おうとしているの、想像できる？

モンスターメソッドだらけのまま、ちょちょいとデザインパターンを使ってしまうかもね

まじめに答えてよー

メッセージ例

The xx yy() has zz lines of code. Current threshold is set to aa. Avoid really long methods.

抑止方法

/**
 *
 * @SuppressWarnings(PHPMD.ExcessiveMethodLength)
 */

説明

Violations of this rule usually indicate that the method is doing too much. Try to reduce the method size by creating helper methods and removing any copy/pasted code.

メソッド長すぎ！
メソッドの処理内容が多すぎるね。
ヘルパーメソッドに分割して、行数を小さくしよう。
（空行もカウントします）

モンスターメソッド

この世界には、メソッド人とエンジニア人が暮らしていました。どちらの民族も友好的でした。

メソッド人のなかには巨人化してモンスターメソッドに変化する者がいます。モンスターメソッドは、エンジニア人が対抗することができない恐ろしい怪物なのです。

メソッド人の大きさは「行」で数えます。何行以上をモンスターメソッドとするという明確な定義はありませんが、一般的には、200〜300行級を小型、500行級を中型、1000行級を大型と分類することが多いようです。

200〜300行級の小型はあちこちに大勢います。それどころか、エンジニア人の多くは、200〜300行級をモンスターメソッドと認識していないかもしれません。1000行級の大型も珍しくなく、うわさでは3000行級の超大型もいると聞きます。

メソッド人は数値の計算が得意なおだやかな民族なので、なぜ凶暴なモンスターメソッドに変化してしまうのか、謎につつまれていました。

エンジニア人の調査によると、モンスターメソッドは病気であり、メソッド人は誰でもモンスターメソッドになってしまうかもしれないことがわかりました。

メソッド人の主食はコード芋です。メソッド人の身体には排泄器官がないため、コード芋を食べると食べただけ、内蔵脂肪や皮下脂肪の一種であるコード脂肪がたまり、体が大きくなります。

そのため、メソッド人は健康オタクが多く、テストケースサプリを飲んだり、フィットネスジムでリファクタ運動することがブームとなっているほどです。

ところが、テストケースサプリを飲み忘れると、メソッド人は体を動かすのが面倒になります。リファクタ運動をさぼるようになり、少しづつ巨人化がはじまります。そして、満腹中枢が麻痺し、コード芋をむさぼるように食べ、巨人化へのループが止まらなくなるのです。

いくつかの治療報告では、巨人化の初期に、テストケースサプリを飲んだり、リファクタ運動することで、元のメソッド人に戻すことができたそうです。

別の治療報告では、巨人化がはじまったメソッド人は、性格が凶暴化し、テストケースサプリを飲むこともリファクタ運動をすることも拒否するため、治療は難しいともあります。

病気は進行し、やがて完全なモンスターメソッドとなり、エンジニア人に対して大暴れするのです。

この惨状を見かねたエンジニア人政府は、巨人化がはじまったメソッド人にコード芋を与えてはいけいないという法律（コード芋禁止法）をつくりました。

コード芋禁止法は、いつから巨人化がはじまったとするのか明確ではない、守らなかったときの罰則がないなど、その効果が疑問視されています。

おおかたの予想どおり、エンジニア人はコード芋禁止法を守ることはなく、今このときもモンスターメソッドは増え続けています。

メソッドの成長速度を遅くする

fooメソッドに機能追加するとき、

2行以上を追加する場合は、fooメソッドにその2行を実装するのではなく、

class MyFoo
{
    public function foo()
    {
        ...
        $x += ($y + $z);  // 新規コード
        $a += ($b + $c);  // 新規コード
        ...
    }Code language: PHP (php)

新しくbarメソッドを作り、barメソッドに実装します。fooメソッドには、barメソッドを呼ぶ1行だけを追加するようにします。

class MyFoo
{
    public function foo()
    {
        ...
        list($x, $a) = $this->bar($a, $b, $c, $x, $y, $z);
        ...
    }

    public function bar($a, $b, $c, $x, $y, $z)
    {
        $x += ($y + $z);  // 新規コード
        $a += ($b + $c);  // 新規コード
        return [$x, $a];
    }Code language: PHP (php)

MyFooクラスは新しいbarメソッドの分だけ大きくなってしまいました。MyFooクラスを大きくしたくない場合は、新しくMyBarクラスを作って、barメソッドを移動します。

class MyFoo
{
    public function foo()
    {
        ...
        list($x, $a) = $this->bar($a, $b, $c, $x, $y, $z);
        ...
    }

    private function bar($a, $b, $c, $x, $y, $z)
    {
        $myBar = new MyBar();
        return $myBar->bar($a, $b, $c, $x, $y, $z);
    }Code language: PHP (php)

class MyBar
{
    public function bar($a, $b, $c, $x, $y, $z)
    {
        $x += ($y + $z);  // 新規コード
        $a += ($b + $c);  // 新規コード
        return [$x, $a];
    }
}Code language: PHP (php)

メソッドに分ける範囲

我々は１行２行でもヘルパーメソッドに分ける、そのドメインの言葉でメソッド名をつけることで、一種のDSL（ドメイン固有言語）を作っているのだ
実践テスト駆動開発

100行に及ぶメソッドを再利用できる機会はない
ThoughtWorksアンソロジー第5章 Jeff Bay氏

さまざまな文脈で利用できるのは、3行のメソッド
ThoughtWorksアンソロジー第5章 Jeff Bay氏

ふだんから、どんどんメソッドに分けましょう。

●メソッド前半にガード節と早期リターンが何個もある場合、ガード節群とメインメソッドをそれぞれメソッドに分けます。

●コメントをつけたくなったら、メソッドに分けます。
つけたいコメントを参考にしてメソッド名にします。

●連続行を空白行で区切りたくなったらメソッドに分けます。
そのかたまりには意味があり、他のかたまりとは意味が違うかもしれません。

●メソッド内の配列リテラルは、それを返すメソッドに分けます。
例えば、getFooLiteralというメソッドにします。

●for文まるごとメソッドに分けます。
例えば、最後に Allをつけて、addFooAllというメソッドにします。

●for文内の処理をメソッドに分けます。
例えば、最後に SingleやOneをつけて、addFooOneというメソッドにします。

●if文まるごとメソッドに分けます。
例えば、最後に Ifxxxをつけて、addFooIfAgeIsLessThanEquals にします。

●if文内の処理をメソッドに分けます。

●switch文まるごとメソッドに分けます。

●case文の処理をメソッドに分けます。

●preg_matchやpreg_replaceをメソッドに分けて、ユニットテストを書きます。
正規表現の間違いが見つかるかもれしません。

これまで動いていたのは、たまたまってことね

どんどんメソッドに分けようー！

メソッドを短くしようー！

デモ行進みたい

メソッドを新しいメソッドに分ける

メソッドに分けるときは、IDEのリファクタ機能を使うと便利です。PhpStormでは、分けたい行や範囲を選択して、メニューのRefactor＞Refactor this＞7. Extract Method...です。

（１）〜（３）は、メインロジックに手を付けないので、テストケースがない場合でも、比較的安心してリファクタできます。

（１）まず、巨大な文字列は、それだけを返すメソッドに分けます。

    public function foo()
    {
        $str = "長い文字列、複数行かもね:" . $x;Code language: PHP (php)

PhpStormでリファクタする場合は、$str = の右辺、セミコロン含まない、
"長い文字列、複数行かもね:" . $x
の範囲を選択してから、リファクタします。

    public function foo()
    {
        $str = $this->getStr($a);
    }

    /**
     * @param $a
     * @return string
     */
    public function getStr($a): string
    {
        return "長い文字列、複数行かもね:" . $a;
    }
Code language: PHP (php)

（２）配列リテラルを定義している場合も、それだけを返すメソッドに分けます。

    public function foo()
    {
        $arr = [
            ['xxx' => 0, 'yyy' => 1],
            ['xxx' => 1, 'yyy' => 2],
        ];
Code language: PHP (php)

PhpStormでリファクタする場合は、
$arr =の右辺、セミコロン含まない
の範囲を選択してから、リファクタします。

    public function foo()
    {
        $arr = $this->getArr();
    }

    /**
     * @param $a
     * @return array
     */
    public function getArr(): array
    {
        return [
            ['xxx' => 0, 'yyy' => 1],
            ['xxx' => 1, 'yyy' => 2],
        ];
    }
Code language: PHP (php)

（３）ガード節と早期リターンが何個もある場合です。

    public function foo($x, $y)
    {
        if ($x === '') {
            return;
        }
        if ($y === '') {
            return;
        }

        // メインロジック
    }Code language: PHP (php)

（３−１）phpstormのリファクタ機能を使って、メインロジックをメソッドに分けます。

    public function foo($x, $y)
    {
        if ($x === '') {
            return;
        }
        if ($y === '') {
            return;
        }

        $this->fooMainLogick($x, $y);
    }

    private function fooMainLogic($x, $y)
    {
        // メインロジック
    }Code language: PHP (php)

（３−２）手作業で、ガード節をメソッドに分けます。

    public function foo($x, $y)
    {
        if (!$this->fooIsValidate($x, $y)) {
            return;
        }
        $this->fooMainLogic($x, $y);
    }

    private function fooIsValidate($x, $y)
    {
        if ($x === '') {
            return false;
        }
        if ($y === '') {
            return false;
        }

        return true;
    }

    private function fooMainLogic($x, $y)
    {
        // メインロジック
    }Code language: PHP (php)

（３−３）fooメソッドごと、別クラスにしてもいいと思います。

    public function foo($x, $y)
    {
        $fooLogic = new FooLogic();
        $fooLogic->foo($x, $y);
    }
Code language: PHP (php)

class FooLogic
{
    public function foo($x, $y)
    {
        if (!$this->isValidate($x, $y)) {
            return;
        }
        $this->mainLogic($x, $y);
    }

    private function isValidate($x, $y)
    {
        if ($x === '') {
            return false;
        }
        if ($y === '') {
            return false;
        }

        return true;
    }

    private function mainLogic($x, $y)
    {
        // メインロジック
    }
}Code language: PHP (php)

（４）for文まるごとメソッドに分けます。

    public function foo($x, $y)
    {
         // 何か処理

        foreach ($arr as $i => $rcd) {
            $rcd['xxx'] = $rcd['yyy'];
            $arr[$i] = $rcd;
        }

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

foreach内で$arrを変更しているので、return で返しました。

    public function foo($x, $y)
    {
         // 何か処理

　　　　$arr = $this->doDomethingAll($arr);

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

    public function doDomethingAll($arr)
    {
        foreach ($arr as $i => $rcd) {
            $rcd['xxx'] = $rcd['yyy'];
            $arr[$i] = $rcd;
        }
    }
Code language: PHP (php)

長いメソッドをリファクタするときは、同じように、if文、switch文をメソッドに分けていきます。

for文やif文ではなくて、意味のある範囲を選んで、メソッドに分けたほうがいいと思うんだけど？

それができないから、モンスターメソッドになったんでしょ

短くしているだけの気がするけど、これってリファクタと言えるのかな？

分けたメソッドなら、ユニットテストを書こうという気になるでしょ。一部分だけでもユニットテストができれば、すごい進歩よ！

まじで面倒だよ、モンスターメソッドが放置されてしまうのもわかるよ。最初からユニットテストを書いたり、短いメソッドを書いたほうが楽だよ

１日２日でモンスターメソッドになったわけではないから、リファクタするのにも時間も手間もかかるのよ。ダイエットと同じね。

メソッドを新しいクラスにする

PhpStormのリファクタ機能で、barメソッドから新しいクラスFooBarを作りました。

class Foo
{
    private $x;
    private $arr;

    public function bar($a)
    {
        $this->arr = [
            ['xxx' => 0, 'yyy' => 1],
            ['xxx' => 1, 'yyy' => 2],
        ];

        $this->x = $this->getStr($a);
        $this->arr = $this->arrUpdate($this->arr);
　 }Code language: PHP (php)

元のクラスには、privateプロパティのsetterが自動生成されました。元のクラスの$thisを、新しいクラスのコンストラクタで渡しています。

class Foo
{
    private $x;
    private $arr;
    /** @var FooBar */
    private $fooBar;

    public function __construct()
    {
        $this->fooBar = new FooBar($this);
    }

    public function bar($a)
    {
        $this->fooBar->bar($a);
    }

    /**
     * @param mixed $arr
     */
    public function setArr($arr): void
    {
        $this->arr = $arr;
    }

    /**
     * @param mixed $x
     */
    public function setX($x): void
    {
        $this->x = $x;
    }

Code language: PHP (php)

class FooBar
{
    private $foo;

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

    public function bar($a)
    {
        $this->foo->setArr([
            ['xxx' => 0, 'yyy' => 1],
            ['xxx' => 1, 'yyy' => 2],
        ]);

        $this->foo->setX($this->foo->getStr($a));
        $this->foo->setArr($this->foo->arrUpdate($this->foo->getArr()));

        var_dump($this->foo->getArr());
    }
}Code language: PHP (php)

元のクラスはすっきりしました。

しかし、新しいクラスのbarメソッドは、元のbarメソッドより、読みにくくなったようです。$this->foo経由でset/getしているためですね。

PhpStormが自動生成しているので、リファクタ後も同じように動作する可能性が高く、この安心感は捨てがたいです。

これをベースに読みやすいようにリファクタします。

メソッドの最初で、$this->foo->getXX() をローカル変数やprivateプロパティに代入し、
メソッド内では、ローカル変数やprivateプロパティで処理し、
メソッド最後に、$this->foo->setXX()で、結果を設定します。

筆者が考えていたリファクタは、次のようなコードです。

元のBarクラスのbarメソッドを、新FooBarクラスへコピペ。
barメソッドで使っているFooクラスのプロパティを、新FooBarクラスのコンストラクタで参照渡し。

class Foo
{
    private $x;
    private $arr;

    public function bar($a)
    {
        $fooBar = new FooBar($x, $arr);
        $fooBar->$bar($a);
　 }
Code language: PHP (php)

class FooBar
{
    private $x;
    private $arr;

    // barメソッドで使っているプロパティを参照渡し
    public function __construct(&$x, &$arr)
    {
        $this->x   = &$x;
        $this->arr = &$arr;
    }

    // 元のFooのbarメソッドからコピペ
    public function bar($a)
    {
        $this->arr = [
            ['xxx' => 0, 'yyy' => 1],
            ['xxx' => 1, 'yyy' => 2],
        ];

        $this->x = $this->getStr($a);
        $this->arr = $this->arrUpdate($this->arr);
　 }
Code language: PHP (php)