モンスターメソッド(phpmdの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)

メソッドに分ける範囲

我々は1行2行でもヘルパーメソッドに分ける、そのドメインの言葉でメソッド名をつけることで、一種の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...です。

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

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

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)

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

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)

(3)ガード節と早期リターンが何個もある場合です。

public function foo($x, $y) { if ($x === '') { return; } if ($y === '') { return; } // メインロジック }
Code language: PHP (php)

(3−1)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)

(3−2)手作業で、ガード節をメソッドに分けます。

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)

(3−3)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)

(4)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文ではなくて、意味のある範囲を選んで、メソッドに分けたほうがいいと思うんだけど?

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

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

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

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

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

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

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)

こちらのほうが元のbarメソッドのままで、リファクタしやすいとも思いましたが、こちらの方法は手動です。

barメソッドで使っているprivateフィールドの個数が多いと、コンストラクタを呼ぶ側、コンストラクタ側のコードを正しく書くのが面倒そうです。

ましてやユニットテストなしでは、手動で正しくリファクタできたかどうか不安です。

結局、
(1)元のクラスでリファクタを続ける
(2)PhpStormのリファクタ機能を使って新クラスに分ける
(3)手動で新クラスに分ける
のどれを使うかはケースバイケースですね。

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