「悪〜いコード」を読んだので、ついでにコードメトリクスを計測してみた

はじめに

先日、「悪〜いコード」を読む機会がありました。
どんな風に悪いのか、軽くですが分析してみたので、ポエムとして投稿したいと思います。

古のコード

私は普段Ruby on Railsをメインに開発を行っているのですが、ユーザーからの質問に答えるために、普段の開発や保守しているのとは全く別のシステムのコードを読む機会がありました。

そのシステムはPHPで書かれた古いコードでした。ユーザーの質問はシンプルだったので、コードを見れば一瞬で答えは見つかるだろうと思ったのですが、とても読み難いコードだったので30分ほど頭を悩ませながら読むことになりました。

何が読み難いのか

結果、ユーザーからの質問には答えることができたのですが
「僕の30分を返してくれーーー！」と叫びたい気分です。
と愚痴ってしまいましたが、それだけでは何の進歩もないので、何が読み難かったのかを明らかにしてみたいと思います。

コードを掲載するわけにはいかないのでコードメトリクスを載せてみようと思います。

コードメトリクスとは（ChatGPTによると）
コードメトリクスは、コードの質を評価するために使用される測定基準です。
コードが読みやすく、維持管理可能であるかどうかを判断するために、多くの開発者が使用しています。
コードメトリクスは、コードの複雑さ、可読性、保守性、品質、可用性などの側面を評価するために、
多数の規則を使用します。そのため、コードの品質を向上させるためには、
コードメトリクスを理解して適切に実装することが非常に重要です。

私が読んだコードの概要

PHP

1ファイル

1260行

コメント：102行

2メソッドで構成されている

1メソッド目：1144行
2メソッド目：89行

ループと条件分岐の数

for：4個
foreach：49個

if：124個
else：53個

switch：3個

ループ分の中身が最大451行

最深部がネスト11段
その内訳：ループ、ループ、条件、ループ、条件、条件、ループ、条件、ループ、条件、本体

テストコードなし

「ネスト11段」
これだけで十分な破壊力があることがわかると思います。

「PHP Mess Detector」召喚

PHPにはコードメトリクスを計測するツールが色々あります。
その中の「PHPMD - PHP Mess Detector」を使って計測してみました。

https://phpmd.org/

インストールや利用方法は割愛します。
様々なコードメトリクスがあるのですが、いくつか抜粋します。

ExcessiveClassLength

1249行のコードを持っています。現在の閾値は1000です。
非常に長いクラスを避けてください。

少しオーバーしていますが、これ自体は悪いことではなさそうです。

ExcessiveMethodLength

メソッドは1144行のコードを持っています。現在の閾値は100に設定されています。
非常に長いメソッドを避けてください。

あらら、こちらは10倍以上もオーバーしてしまっています。

CyclomaticComplexity

メソッドのサイクロマティック複雑度は216です。
設定されたサイクロマティック複雑度の閾値は10です。

「２１６」っ！！

サイクロマティック複雑度（循環的複雑度）とは
ソースコードがどれぐらい複雑であるかをメソッド単位で数値にして表す指標になります。 複雑度が高いメソッドほど、コードは複雑でメンテナンスがしづらく、ユニットテストがしづらくなります

サイクロマティック複雑度（循環的複雑度）には次のような目安があります。（諸説あります）

循環的複雑度	複雑さの状態	バグ混入確率
10以下	非常に良い構造	25%
30以上	構造的なリスクあり	40%
50以上	テスト不可能	70%
75以上	いかなる変更も誤修正を生む	98%

参考
https://jp.mathworks.com/discovery/cyclomatic-complexity.html

何ということでしょうか。「２１６」これはラスボス級の強さですね。
テスト不可能なものの動作を理解することも不可能ですね。

NPathComplexity

メソッドのNPath複雑性は731994476777279122598156358356690200320です。
設定されたNPath複雑性の閾値は200です。

こんな長い数値を見たのは円周率以来です。

命名問題

コードメトリクスには現れませんが、コード内に登場する変数、メソッド、クラスなどの命名は可読性に重要な要素です。

今回読んだコードの変数名はこんな感じです。

$tmp[0]

$arr[]

$this->_o[2]

$position[0]

$cv

$n

$this->_o[$n][$no]

$check
$check2
$check3

もちろんわかりやすい名前の変数もあるのですが、
上記のような短くて意味が分かり難い変数名で、かつ変数のスコープが広すぎる場合、理解するのが非常に困難です。
せめてスコープが狭ければ（数行から10行程度とかなら）理解もできるのですが。。。

コード改善に向けて最低限やるべきこと

もしも、今回のコードを改善するとしたら？の空想です。

• コードメトリクスで指摘された値を閾値以下に抑える
• 変数名を理解しやすい名前に変える

しかし、それだけでは読み易くなるとは思えません。

ExcessiveMethodLengthとして指摘されている1メソッドの長さを100行以内に抑えることを実践したとして、今の1260行のメソッドを分割すると13個程度のメソッドになります。

これで元コードと同じようにそのまま11段ネストしてたとしたら、とても理解できたものではありません。（もう少しネストは浅くなるでしょうが、深いネストであることに変わりはないでしょう）

このコードの構造は実現したいものを上から順番に行っている、ある意味素直な構造です。
構造が素直な分、そのしわ寄せとして深いネストと、論理的な区切りを失う結果となってしまってるのです。

例えるなら、章節項のない論文や、句読点がなく延々と接続詞で繋がった長文のようです。

短い文章（コード）であればそれもで構わないのですが、長い文章（コード）を表現するのであれば、論理的な区切り、読み進める上で抽象化できる概念が欲しいところです。

どうすれば「良いコード」になる？

答えは、高凝集・低結合だと考えています。
しかし、残念ながら、まだ上手く説明できないので、さらに理解を深めて改めて記事にできたらと思います。

最後に

冒頭から「悪いコード」と書いてきましたが「動いているコードは良いコード」という言葉もあるので一概に「悪いコード」というわけではありません。しかし、後からコードを読んだり、修正したりする人からみるとやはり「悪いコード」と言わざるを得ない面もあるのかなと思います。

今回分析してみたコードは、これまで見た中で一番長くて複雑なコードというわけでは決してありませんが、こうやって自分が読み難いと思ったコードを分析してみたのは初めてでした。

これまでも読み難いコードを沢山読んできましたが、今回特に読み難いと感じた理由は、最近、読み易さや修正のし易さを考えた、わかりやすくシンプルなコードを書いたり読んだりすることが多くなってきたからだと思います。

「良いコード」を読み書きして心穏やかになりたいですね。