これは ランサーズ Advent Calendar 2021 2 日目の記事です。
DBRE の まみー です。
DB 相談やサポートをチーム横断、事業横断でやっているのですが、その一環として PHPMD や PHPStan のサポートをしています。
中でも最近 PHPMD にて ExcessiveClassComplexity (過剰なクラスの複雑さ) を指摘される事例が増えてきました。
結論としては 指摘は必ず完全対応を前提に考える で、例外はありません。
とはいえ理由もよくわからないまま対応を考えるのはしんどいので、実例をエントリーにしてみます。
起きていたこと
自分の修正がトリガーとなって PHPMD に ExcessiveClassComplexity (過剰なクラスの複雑さ) を指摘されました。
- そもそも複雑さがそこそこ高い既存クラス
- メソッド追加、分岐追加
- 閾値を超える
上記のように、既存クラスに何かしらの複雑さを追加した場合が多いです。
この場合の感情は概ね以下の感じなんじゃないかなと思います。
- メソッド増やしただけなのに
- 分岐を追加しただけなのに
- 既存のコードは触っていないのに
つまり、 修正したら突然怒られた となりがちなんですよね。
そもそも「ExcessiveClassComplexity (過剰なクラスの複雑さ)」とはなんなのか。
知っておく必要があります。
PHPMD の指摘内容
これは実際に社内の管理画面開発時に起こった事例です。
The class SearchComponent has an overall complexity of 50 which is very high.
The configured complexity threshold is 50.
直訳すると以下です。
SearchComponent クラスの全体的な複雑さは 50 で、非常に高くなっています。設定されている複雑さのしきい値は50です。
複雑さの計算方法はこのあとで解説します。
その前に大事なことを書きます。
安易にやってはいけないこと
今回の原因は ExcessiveClassComplexity (過剰なクラスの複雑さ) が閾値を超えたためです。
ここで @SuppressWarnings(PHPMD.ExcessiveClassComplexity) をクラスの PHP Doc に書くと、PHPMD の分析範囲外にできます。
コードは CakePHP 4 前提です。
/**
* Search Component
*
* ...
*
* @SuppressWarnings(PHPMD.ExcessiveClassComplexity)
*/
class SearchComponent extends Component
{
...
が、安易にコメントでスルーせず、クラスの複雑さに向き合ういい機会を得たと受け止め、本質的な修正をしましょう。
サービスはこれからも成長を続けるので、抜本的な設計の見直しを行わない限りは 今現在が最も複雑さが低い状態 です。今後放っていても自然に低くなることはありません。
今やらないと後でもっと大変になるだけ。放置はデメリットでしかないです。
学習の文脈でも「学び始めるのは今が一番若い」と言われます。
今が一番シンプル である、と受け止め、品質と向き合いましょう。
また、閾値の緩和は設定ファイル (xml) で可能ですが、安易に行わないようにしましょう。
妥協から品質低下が始まります。
何を指摘されているのか
ひとことで言うと 1 つのクラス内でのメソッドごとの複雑さの合計が閾値を超えた 状態です。
以下の場合に指摘されます。
- メソッドの循環的複雑度 (CyclomaticComplexity) を計算
- クラス単位の合計が閾値を超えた
- 複雑さの閾値はデフォルト 50
では PHPMD 公式 https://phpmd.org/rules/codesize.html から、過剰なクラスの複雑さ (ExcessiveClassComplexity) を見ていきましょう。
過剰なクラスの複雑さ (ExcessiveClassComplexity)
ExcessiveClassComplexity の章を翻訳します。
過剰なクラスの複雑さ
Since: PHPMD 0.2.5あるクラスの Weighted Method Count (WMC、重み付けされたメソッド数) は、そのクラスの変更や保守にどれだけの時間と労力が必要かを示す良い指標です。WMC の指標は、クラスで宣言されたすべてのメソッドの複雑さの合計として定義されます。メソッドの数が多いということは、そのクラスが派生クラスに大きな影響を与える可能性があるということでもあります。
メソッドに重みを付ける基準が「循環的複雑度 (CyclomaticComplexity) 」で、重みの計算方法が公式にも掲載されています。
この、メソッドごとの循環的複雑度 (CyclomaticComplexity) の合計が、過剰なクラスの複雑さ (ExcessiveClassComplexity) の閾値である 50 を超えた際に指摘されます。
では次に、循環的複雑度 (CyclomaticComplexity) を見てみましょう。
循環的複雑度 (CyclomaticComplexity)
CyclomaticComplexity を翻訳します。
循環的複雑度(CyclomaticComplexity)
Since: PHPMD 0.1複雑さは、メソッド内の決定点の数にメソッドのエントリの数を加えたもので決まります。決定点とは、'if', 'while', 'for', および 'case label' のことです。一般的に、1-4 は低い複雑さ、5-7 は中程度の複雑さ、8-10 は高い複雑さ、11+ は非常に高い複雑さを示します。
公式からコード例を引用します。
// Cyclomatic Complexity = 11
class Foo {
1 public function example() {
2 if ($a == $b) {
3 if ($a1 == $b1) {
fiddle();
4 } elseif ($a2 == $b2) {
fiddle();
} else {
fiddle();
}
5 } elseif ($c == $d) {
6 while ($c == $d) {
fiddle();
}
7 } elseif ($e == $f) {
8 for ($n = 0; $n < $h; $n++) {
fiddle();
}
} else {
switch ($z) {
9 case 1:
fiddle();
break;
10 case 2:
fiddle();
break;
11 case 3:
fiddle();
break;
default:
fiddle();
break;
}
}
}
}
判断基準は以下のようです。
分岐 2 に関しては、実際に書いて試した結果としてカウントされていることを確認したものです。
- メソッドそのものが 1 分岐
- 分岐 1
ifelseifswitch-casecatch
- 分岐 2
&&||?
- ループ
メソッド単位で循環的複雑度に問題がある場合は、CyclomaticComplexity も指摘されます。
今回の事例では、メソッド単位では問題なかったものの、合計してみるとクラス単位で問題になっていました。
実際に循環的複雑度を数えてみる
閾値は以下の合計数 50 です。
(カウント対象に抜け漏れあればご指摘ください)
- メソッド数
- 使う場合、使わない場合、で分岐と換算する
- 分岐数
- 三項演算子も分岐
- if の中に複数条件あればそれぞれカウント
- if をカウントしているので else はカウントしない
- case をカウントしているので default はカウントしない
コード
実際に、過剰なクラスの複雑さ (ExcessiveClassComplexity) の指摘を受けたコードを見ていきます。
一部省略したり改変していますが、カウント対象はそのままです。
<?php
declare(strict_types=1);
namespace App\Controller\Component;
use Cake\Core\Configure;
...
class SearchComponent extends Component
{
use ModelAwareTrait;
1 public function initialize(array $_config): void
{
parent::initialize();
}
2 public function existsIndex(): bool
{
return ...;
}
3 public function deleteIndex(): bool
{
$response = ...;
return !empty($response['...']);
}
4 private function getCharFilter(): array
{
return [
...,
];
}
5 private function getTokenizer(): array
{
return [
...,
];
}
6 private function getAnalyzer(): array
{
return [
...,
];
}
7 private function getTypeText(): array
{
return [
...,
];
}
8 public function createIndex(): bool
{
$params = [
...,
];
$response = ...;
return !empty($response['...']);
}
9 public function getProjectsQuery(): \Cake\ORM\Query
{
return $this->projects->find()->contain([
...,
]);
}
10 public function getProjectsQueryByStartIdAndEndId(int $startId = 1, int $endId = -1): \Cake\ORM\Query
{
$query = $this->getProjectsQuery();
11 if ($endId < 1) {
$query->where(['Projects.id >=' => $startId]);
} else {
$query->where(function (QueryExpression $exp) use ($startId, $endId) {
return $exp->between('Projects.id', $startId, $endId);
});
}
return $query;
}
12 public function getProjectsQueryByIds(array $ids): \Cake\ORM\Query
{
$query = $this->getProjectsQuery();
$query->where(function (QueryExpression $exp) use ($ids) {
return $exp->in('Projects.id', $ids);
});
return $query;
}
13 public function getProjectsOfLimit(\Cake\ORM\Query $query, int $i = 0): array
{
return $query
->limit(100)
->offset(100 * $i)
->toList();
}
14 public function executeBulk(\Cake\ORM\Query $query): void
{
$total = 0;
15 for ($i = 0;; $i++) {
$projects = $this->getProjectsOfLimit($query, $i);
16 if (count($projects) == 0) {
break;
}
$loopIndexCount = $i + 1;
$indexProjects = [];
$deleteProjects = [];
17 foreach ($projects as $project) {
18 $isIndex = !$project->is_disabled && !$project->project_status->is_archived;
19 if ($isIndex) {
$indexProjects[] = $project;
} else {
$deleteProjects[] = $project;
}
}
20 if (count($indexProjects)) {
$this->elasticSearchComponent->bulkDocument(
$this->makeIndexBulkParams($indexProjects)
);
}
21 if (count($deleteProjects)) {
$this->elasticSearchComponent->bulkDocument(
$this->makeDeleteBulkParams($deleteProjects)
);
}
$total += count($projects);
}
}
22 public function message(string $message, array $params = [], ?string $className = null, ?string $functionName = null): string
{
23 if (!$className) {
$className = self::class;
}
24 if (!$functionName) {
$functionName = ...;
}
$outParams = ...;
return vsprintf('[%s] %s ' . $message, $outParams);
}
25 private function makeIndexBulkParams(array $projects): array
{
$params = ['body' => []];
26 foreach ($projects as $project) {
$params['body'][] = [
...,
];
$params['body'][] = $this->getBody($Project);
}
return $params;
}
27 public function makeDeleteBulkParams(array $projects): array
{
$params = ['body' => []];
28 foreach ($projects as $project) {
$params['body'][] = [
...,
];
}
return $params;
}
29 private function getBody(Project $project): array
{
return [
...,
];
}
30 private function makeRootAndPrimary(Project $project): array
{
$list = array_map(function ($p) {
...;
}, ...);
return [$list[0], $list[1]];
}
31 private function makeSecondary(Project $project): array
{
return array_map(function ($c) {
return [
...,
];
}, $Project->project_categories);
}
32 private function makeUser(Project $project): array
{
return [
...,
];
}
33 private function makeService(Project $project): array
{
return array_map(function ($i) {
...;
}, ...);
}
34 private function makePrice(Project $project): array
{
return array_map(function ($m) {
...;
}, ...);
}
35 private function makeDeliveryTime(Project $project): array
{
return array_map(function ($m) {
...;
}, ...);
}
36 private function makeTag(Project $project): array
{
return array_map(function ($t) {
...;
}, ...);
}
37 private function makeCountAll(Project $project): int
{
return $project->project_summary->count_all;
}
38 private function makeAvgAll(Project $project): float
{
return $project->project_summary->avg_all;
}
39 private function makeSumAll(Project $project): int
{
return $project->project_summary->sum_all;
}
40 private function makeImpressionCount7d(Project $project): int
{
$fromDate = Carbon::now()->subDays(8)->format('Y-m-d');
$toDate = Carbon::now()->subDays(1)->format('Y-m-d');
return $this->getPointSummary($project, 'ImpressionProject', $fromDate, $toDate);
}
41 private function makeFootprintCount7d(Project $project): int
{
$fromDate = Carbon::now()->subDays(8)->format('Y-m-d');
$toDate = Carbon::now()->subDays(1)->format('Y-m-d');
return $this->getPointSummary($project, 'FootprintProject', $fromDate, $toDate);
}
42 private function makeQuoteCount7d(Project $project): int
{
$fromDate = Carbon::now()->subDays(8)->format('Y-m-d');
$toDate = Carbon::now()->subDays(1)->format('Y-m-d');
return $this->getPointSummary($project, 'QuoteProject', $fromDate, $toDate);
}
43 private function makeLikeCount7d(Project $project): int
{
$fromDate = Carbon::now()->subDays(8)->format('Y-m-d');
$toDate = Carbon::now()->subDays(1)->format('Y-m-d');
return $this->getPointSummary($project, 'LikeProject', $fromDate, $toDate);
}
44 private function makeOrderCount7d(Project $project): int
{
$fromDate = Carbon::now()->subDays(8)->format('Y-m-d');
$toDate = Carbon::now()->subDays(1)->format('Y-m-d');
return $this->getPointSummary($project, 'OrderProject', $fromDate, $toDate);
}
45 private function makeCreated(Project $project): string
{
return Carbon::parse($project->created)
->setTimezone('UTC')
->format('Y-m-d\TH:i:s\Z');
}
46 private function makeModified(Project $project): string
{
return Carbon::parse($project->modified)
->setTimezone('UTC')
->format('Y-m-d\TH:i:s\Z');
}
47 private function getPointSummary(Project $project, string $metricName, ?string $fromDate = null, ?string $toDate = null): int
{
$metric = $this->Metrics
->findByName($metricName)
->cache($metricName)
->first();
48 if (!$metric) {
return 0;
}
$query = $this->...
->find('all')
->where([
'metric_id' => $metric->id,
'project_id' => $Project->id,
]);
if (
49 !empty($fromDate)
50 && !empty($toDate)
) {
$query = $query->where(function ($exp) use ($fromDate, $toDate) {
return $exp->between('date', $fromDate, $toDate);
});
}
$pointSummary = $query
->select(['value' => $query->func()->sum('...')])
->first();
return intval($pointSummary->value);
}
}
解説・改善
ちょうど 50 に達していました。
では、主な部分を上から順に簡単に解説します。
No. 4 ~ 7
ElasticSearch へインデックスを更新する際に必要な array を返すメソッドです。
似たようなメソッドが並びますが、単なる定数ではなくどれも違う処理で、具体的には以下の内容で個別メソッド化が適切でしたので、問題なしと判断しました。
- 返す array の構造がそれぞれで違う
- 用途に合わせた別のメソッド実行結果などから array を作る
No. 9 ~ 12
クエリビルダの各条件をメソッド化しました。
今回はメソッドチェーンで書くには長く可読性に欠けると判断し、問題なしとしました。
(個人的には SQL は 1 箇所で見通しが良いのが好みではあります。)
No. 14 ~ 21 : 改善の余地あり
ElasticSearch へ実際にデータを投入するメソッドです。
- limit をかけて DB から一定のレコードを取得
- 取得レコード全件ループ
- ElasticSearch のインデックスに対する振り分け
- 挿入対象を振り分け
- 削除対象を振り分け
- 挿入対象があればインデックスへ挿入
- 削除対象があればインデックスから削除
分岐数とループのネストに改善の余地があります。
が、以下の理由で改善対象外としました。
- 読めばわかる
- 十分にテストが書かれている
- 今後の拡張性が低い（修正される可能性が低い）
処理対象レコードがなくなるまで実行、の場合のループ処理を for で書くか while で書くかなどは、意見の分かれるところだとは思います。
No. 22 ~ 24
このコード実はバッチ処理なのですが、ログメッセージの編集を実行しています。
のちの実行経過観察、不具合時の追跡などに必要な処理でした。
No. 25 ~ 36 : 改善の余地あり
No. 4 ~ 7 で ElasticSearch へインデックスを更新する際に必要な array を作っていましたが、その中で call されているメソッド群です。
これらは必要と判断できました。
ただしここでは array_map() を 6 箇所で使っています。
array_map() は foreach と等価なループを別の構文に書き換えたものと解釈できますので、循環的複雑度 (CyclomaticComplexity) が実質 6 箇所不当に隠蔽されているとも言えます。
引き続き複雑度の課題として、今後も改善を続けていく必要があります。
No. 37 ~ 39 : 要改善
getter メソッドです。
今回の用途は No. 4 ~ 7 の array に値をセットするための private メソッドで、わざわざ切り出す必要がなかったので廃止し、array に直接値をセットするよう修正しました。
メソッドが消えたことにより、 循環的複雑度 (CyclomaticComplexity) が 3 つ解消 されました。
が、 $project->project_summary->count_all のように、 デメテルの法則 に反しているコードは依然残ったままです。
詳細は後続の「今後の課題」で触れますが、循環的複雑度 (CyclomaticComplexity) が解消したとはいえ、引き続き課題と認識し改善を続けていく必要があります。
No. 40 ~ 44 : 要改善
さまざまな count を取得するメソッドです。
複数の問題点がありました。
- 期間が 7 days から 14 days などに変更された場合、5 つ全て修正する必要がある
- どの count を取りたいかが各メソッド内の固定文字列で書かれており、メソッド名と意味が重複する
- 取りたい count の種類が増えた場合、メソッドも増える
5 つのメソッドは統合できると判断し、以下の修正を行いました。
- 以下を引数で渡す
- 期間
- 取りたい count の文字列
結果 1 つのメソッドにまとまり、 循環的複雑度 (CyclomaticComplexity) が 4 つ解消 されました。
No. 45 ~ 46 : 要改善
DB の Datetime 型から、ElasticSearch の Timestamp フォーマットへ変換する処理です。
処理としては必要なのですが、2 つのメソッドの処理自体は同じで以下の問題がありました。
- 今後フォーマットが変わると 2 箇所修正が必要
- ElasticSearch にインデックスしたい Datetime が増えると似たようなメソッドが増える
よって、No. 40 ~ 44 と同じようにメソッドを統合することで、 循環的複雑度 (CyclomaticComplexity) が 1 つ解消 されました。
注意点
改修ポイントとは別に、カウントする際につい見落としがちな部分を解説します。
No. 18
論理積演算子 && です。
分岐は if や case が認識しやすいですが、以下も分岐です。
&&||?catch
17 foreach ($projects as $project) {
18 $isIndex = !$project->is_disabled && !$project->project_status->is_archived;
19 if ($isIndex) {
$indexProjects[] = $project;
} else {
$deleteProjects[] = $project;
}
}
今回、これを見落としたために「あれ、1 つ足りない？」となって何度かコードを往復しました。
No. 49 ~ 50
&& も分岐ですと書きましたが、 if 文の中でも同じです。
if と、その中の && は別物なのでそれぞれカウントする必要があります。
PSR-12 https://www.php-fig.org/psr/psr-12/ (5.1 if, elseif, else を参照) に従って改行していると気づきやすいですね。
if (
49 !empty($fromDate)
50 && !empty($toDate)
) {
if をカウントしていても、その中の分岐は忘れがちでした。
まとめ
重複するメソッドを統合することで、循環的複雑度 (CyclomaticComplexity) が 8 つ改善され、結果として ExcessiveClassComplexity (過剰なクラスの複雑さ) の閾値を下回りました。
確実に問題があったことがわかりました。
今回の PHPMD の指摘を追いかけてみましたが、以下のことが明白になるなと実感しました。
- 複雑さには必ず理由がある
- 理由を知る
- メソッド数、分岐数と種類
- 改善する
- クラスを分割
- 似たようなメソッドを 1 つにまとめる
- 無駄な分岐を減らす
改善の余地が残っていますし、今後は引き続き品質に向き合い、妥協せず改善していきます。
今後の課題
PHPMD が指摘してくれている循環的複雑度 (CyclomaticComplexity) は解消できましたが、以下の課題が残っています。
array_map() 使用による、実質的な foreach ループの隠蔽
array_map() を 6 箇所で使用していますが、
6 箇所ある= 循環的複雑度 (CyclomaticComplexity) が 6 箇所残っています。
8 箇所対応できて、閾値の 50 から 42 に改善できたと思っていても、実質的には 48 にしかなっていません。
メソッド、分岐、ループの追加で簡単に閾値を超えてしまう可能性が高いです。
その際は責務に応じてクラスを分割するなど、今後の課題として向き合い改善を続けていきます。
デメテルの法則違反
$project->project_summary->count_all のように、 デメテルの法則 に反しているコードがあります。
デメテルの法則から引用すると以下の部分が該当します。
基本的な考え方は、任意のオブジェクトが自分以外（サブコンポーネント含む）の構造やプロパティに対して持っている仮定を最小限にすべきであるという点にある。
あるオブジェクトAは別のオブジェクトBのサービスを要求してもよい（メソッドを呼び出してもよい）が、オブジェクトAがオブジェクトBを「経由して」さらに別のオブジェクトCのサービスを要求してはならない。これが望ましくないのは、オブジェクトAがオブジェクトBに対して、オブジェクトB自身の内部構造以上の知識を要求してしまうためである。 このような場合には、クラスBを変更し、クラスAがクラスBに対して行った要求を適切なBのサブコンポーネントに伝播させるようにすればよい。または、AがCへのリファレンスを持つようにして、AがCを直接呼ぶようにしてもよい。この法則に従えば、オブジェクトBが知っているのは自分自身の内部構造だけになる。
$project->project_summary->count_all は、アロー演算子で「自分 (ここでは $project) 以外の構造を参照」していることが問題なのですが、実は以下の処理結果でもあります。
-
projectsテーブルとproject_summariesテーブルは 1 対 1 の関係- CakePHP 4 の Model でアソシエーションされている
- ORM の contain で関連するテーブルを指定
- 具体的には
$this->Projects->find()->contain([ 'project_summaries']);
- 具体的には
- 結果、
$projectオブジェクトの子として$project_summaryを参照-
project_summaries.count_allカラムを参照が目的
-
これが本当にデメテルの法則に反しているのか考える必要がありますし、CakePHP 4 と ORM の使い方としての正しさと合わせて、より深く調べる必要があるかと考えています。
おわりに
ExcessiveClassComplexity (過剰なクラスの複雑さ) がちょうど 50 の実例をもとに書いてみました。
ちょうど 50 というと「それ作ったでしょ？」と思うかもですが、 実際に現場で起こっている事実 です。
同じ問題で悩んでいる人の記事で、 SuppressWarnings で対象外にしているのもありました。
現場によって事情が異なるのでそれはそれなのですが、 妥協は品質低下の始まりであることは常に意識したい なと考えています。
PHPMD を導入してみて、少しずつですが構造的な問題点が可視化されてきました。
特に今回は「今後の課題」でも触れたように、 PHPMD の指摘があったからこそより本質的な問題が表面化 しました。
問題の表層を取り除き本質を可視化することは PHPMD 導入の醍醐味の 1 つだと思いますし、非常にポジティブです。
自動化できるところは任せ、人間はより本質的なことへ注力するためにも、便利なツールを吟味の上で導入していきたいです。
そして、 妥協せず改善を続けていくことこそ成長 だと信じ、今後も続けていきます。
余談ですが、ランサーズの CakePHP バージョンアッププロジェクトでは PHPStan も導入しました。
これがまたすごく良くて最高です (語彙力)。
品質は上がるし勉強になるしで良いことしかありません。
みなさんもぜひ、品質に向き合ってみてはいかがでしょうか。
アドベントカレンダー、明日は QA チームのいさな(@isanasan_) さんの「CI を待たずに phpcs の検証を自動実行するためのエディタ中立な手法を模索する」です。コードの品質を向上するツールの話、楽しみです！