低品質コードの成長過程
インデントの深さや変数・関数の命名規則に関する信条の違いで殴り合いの喧嘩ができる我々プログラマにとって、他人の書いたコードを読んだり、それに手を入れたりすることは大変にストレスフルな作業。 しかしながら今日では、ビジネスとして作成されるプログラムがたった一人の人間によって書かれることは極稀なことであり、職業プログラマは誰もが多かれ少なかれ、そのストレスに耐えながら仕事をしています。
それでも、よく管理されているプロジェクト、よくメンテナンスされているシステムではプログラムの質も比較的優良であり、その改修・拡張といった作業は開発元企業に属する人員によって行われている場合が多いようです。 一方、私のような「外注」に改修依頼が出されるようなプログラムというのは、開発元の「正規部隊」が手を付けられない、あるいは手を付けたくないほどに低品質なものであるというのが相場。 さすがに本記事のタイトルに入れることは思いとどまりましたが、そのようなコードは「糞コード (くそこーど)」と呼ばれ、ある種この業界の名物となっているとかいないとか。
「なんだこの糞コードは! (怒)」「書いた奴出てこい!(怒)」
こんな声を聞いたり、叫んだりしたことはありませんか?
ウンコードについて学ぶことによってウンコードを撲滅しましょう!
私自身、既に本が書けそうなほどに糞コードを相手にしてきましたが、その中のひとつに、それ自体は非常に小さくシンプルなものでありながら、糞コードが生産されるプロセスを理解するのに大変有用な事例がありました。
if ($record->type != TYPE_A && $record->type != TYPE_B && $record->type != TYPE_C){
//一般的な処理
}
else if ($record->type != TYPE_B && $record->type != TYPE_C){
//タイプAに対する処理
}
else if ($record->type != TYPE_C){
//タイプBに対する処理
}
else {
//タイプCに対する処理
}
今回はこのコードを教本として、糞コードが出来上がるまでの過程を、読者の皆様と共に考察していきたいと思います。
どこが駄目なのか
先に挙げたコードが酷い出来のものであることは一目瞭然なのですが、プログラマでない方にとってはそうではないかもしれません。 そこでまず、これと同じ動作をするコードを「まともに」書いたらどうなるのかを見てみましょう。
if ($record->type == TYPE_A){
//タイプAに対する処理
}
else if ($record->type == TYPE_B){
//タイプBに対する処理
}
else if ($record->type == TYPE_C){
//タイプCに対する処理
}
else {
//一般的な処理
}
かなりマシになコードになりましたが、私自身が実際にリファクタリングをするのであれば間違いなく
ということで、制御構造 switch を導入するでしょう。
switch ($record->type){
case TYPE_A:
//タイプAに対する処理
break;
case TYPE_B:
//タイプBに対する処理
break;
case TYPE_C:
//タイプCに対する処理
break;
default:
//一般的な処理
break;
}
可読性の絶対的な基準とは言い難いものの、コードの行数 (≒ステップ数) や行当たりの文字数は、プログラムの局所的な可読性の指標となるもの。 元のコードと比較すると、修正後の2例は共に顕著に「横に短く」なっており、これは「その動作を読み解くのに時間が掛からない」コードになっていることを意味します。 とは言え、これらは別段「凄腕のプログラマだけが書ける」という類の芸術的コードではなく、平均的どころかそれをやや下回る程度の腕前であっても、
と期待されるもの。 それにも関わらず
という問いが、本エントリの主題となります。
コードに歴史あり
疑いようもなく糞であるこのコードも、書かれたときから酷いものだったわけではありません。 最初のマズい変化が生じたのは、後から追加された特殊な処理を必要とするレコードタイプAへの対応の際であったと推測されます。
| 変更前 (Rev. 1) | 変更後 (Rev. 2) |
|---|---|
if ($record->type != TYPE_A){ |
|
//一般的な処理 |
//一般的な処理 |
} |
|
else { |
|
//タイプAに対する処理 |
|
} |
一見すると、この変更は "原則"、すなわち「一般的な処理」を先に、「タイプAに対する処理」という "例外" を後に記述するという、日常生活で目にすることの多い様式に則っているため、違和感を感じる人は少ないかもしれません。 しかし、この if 文の else 節に制御が移る条件を自然言語 で表現してみると、
という二重否定の形になっており、プログラマの間で広汎に共有されている
という指針に反したものとなっています。 下図左側に示すのは Windows のとある悪名高いオプション設定のチェックボックスですが、このようにチェックが外れているとき、結局拡張子は表示されるのでしょうか? されないのでしょうか? 程度の差こそあれ、「"表示しない" をしない、ということは......表示する、でいいんだよな?」といった具合に判断に一瞬の遅れが生じるはず。 そうした混乱の度合いは、右側のように、動作の記述を "隠す" という否定でない形に変更することで抑制することができます。
加えて、このコードにおいては、if 文の条件式に否定形を使うこの記述の仕方は、単に「読みやすさ」「分かりやすさ」を低下させるにとどまらず、その後に行われる改修作業にも悪い影響を与えます。
上記の改修が行われた後さらに特別な処理を必要とするタイプBが追加された場合、対応するプログラマはどのような変更をコードに加えるでしょうか。 標準的な開発現場においてはコードを修正する場合、変更した箇所すべてに対してテストを行うことが義務付けられます。 改修にあたるプログラマはテストに割く作業工数 (=手間) をできるだけ省くため、必要最小限の変更量でこれに対応しようとするでしょう。 従って先のコードへの修正は以下のようになるはずです。
| 変更前 (Rev. 2) | 変更後 (Rev. 3) |
|---|---|
if ($record->type != TYPE_A){ |
if ($record->type != TYPE_A && $reocrd->type != TYPE_B){ |
//一般的な処理 |
//一般的な処理 |
else { |
else if ($record->type != TYPE_B){ |
//タイプAに対する処理 |
//タイプAに対する処理 |
} |
} |
else { |
|
//タイプBに対する処理 |
|
} |
ここに来て、悪い兆候が顕著に見えてきました。 if 文の条件に && による結合が入ることで、プログラムが「横に長く」なっています。 できることなら、前節で挙げた「まともなコード」のような修正を施したいところですが、残念ながらそれは行われません。 何故ならば、それには以下に示すような「大量の」変更が必要となるからです。
| 変更前 (Rev. 2) | 行われない変更 (Rev. 3') |
|---|---|
if ($record->type != TYPE_A){ |
if ($record->type == TYPE_A){ |
//一般的な処理 |
//タイプAに対する処理 |
else { |
else if ($record->type == TYPE_B){ |
//タイプAに対する処理 |
//タイプBに対する処理 |
} |
} |
else { |
|
//一般的な処理 |
|
} |
この例では単一行のコメントとして省略表現されている「各タイプに対する処理」の実体は複数のステートメントから構成されるサブプログラムであり、"修正差分" としてハイライトされるコードの量はここで示されているよりもずっと多いことに留意してください。 また、条件式を否定形から肯定系に変更したことに伴い、「一般的な処理」と「タイプAに対する処理」の位置が入れ替わっていますが、一般的なバージョン管理システム ではこうした順序の入れ替えはすべて更新と見なされるため、それらの箇所はすべて新規に書き起こしたのと同様の扱いを受けることに。 そのため、このような変更をしてしまうと、既存機能であるはずの「一般的な処理」と「タイプAに対する処理」を通過するフローもすべてがテストの対象となってしまいます。
二葉にして絶たざれば斧を用うるに至る。 後々のことを考えれば、多少面倒なそのテストを実施してでも、コードを「まともな」形にしておくべきだと思うのですが、常に納期に追われ、コスト (=工数) 削減の圧力に晒されている開発現場ではそうした意見は通らない、というのが世の常。 無論、ここでリファクタリングの工数を割けないプロジェクトにおいて、後に追加される「タイプCに対する処理」への対応時にそれが行われるなどと期待できる道理もなし。
| 変更前 (Rev. 3) | 変更後 (Rev. 4) |
|---|---|
if ($record->type != TYPE_A && $reocrd->type != TYPE_B){ |
if ($record->type != TYPE_A && $reocrd->type != TYPE_B && $record-> != TYPE_C){ |
//一般的な処理 |
//一般的な処理 |
else if ($record->type != TYPE_B){ |
else if ($record->type != TYPE_B && $record->type != TYPE_C){ |
//タイプAに対する処理 |
//タイプAに対する処理 |
} |
} |
else { |
else if ($record->type != TYPE_C){ |
//タイプBに対する処理 |
//タイプBに対する処理 |
} |
} |
else { |
|
//タイプCに対する処理 |
|
} |
斯くして目出度く、本エントリの冒頭に挙げた「糞コード」が製品プログラムおよびリポジトリの中に安定した居場所を確保、と相成りました。 結晶成長において、最初の一片 (タネ) が後に形成される結晶の方向や形を決定付けるように、糞コードは最初に書かれた「わずかに問題のあるコード」から成長していくことで出来上がるものなのです。
プログラムの中に溜まっていくゴミ
改修に伴って劣化するコードのもうひとつの例として、別のプロジェクトで行われたコード変更をご覧ください。
かなり単純化してありますが、以下のプログラムは、セッションに対して終了 (finish) 操作が行われた際の処理として、現在時刻 ($now) が期限 (time_expire) を超過していたら、自身 ($this) のステータス (status) として「期限切れ (STATUS_EXPIRED)」を、そうでなければ「閉鎖 (STATUS_CLOSED)」を設定する、というものです。
function finish(){
$now = new DateTime();
if ($this->time_expire >= $now){
$this->status = self::STATUS_EXPIRED;
}
else {
$this->status = self::STATUS_CLOSED;
}
}
後に仕様変更があり、セッション終了時は期限と関係なくステータスを「期限切れ」に設定することとなりました。 普通に考えれば、そのための修正は次のようになるでしょう:
| 変更前 | 期待される変更 |
|---|---|
function finish(){ |
function finish(){ |
$now = new DateTime(); |
|
if ($this->time_expire >= $now){ |
|
$this->status = self::STATUS_EXPIRED; |
$this->status = self::STATUS_EXPIRED; |
} |
|
else { |
|
$this->status = self::STATUS_CLOSED; |
|
} |
|
} |
} |
つまり、修正後のコードは以下のようになるはずだったのです:
function finish(){
$this->status = self::STATUS_EXPIRED;
}
ところが、実際に行われた修正は次のようなものでした:
| 変更前 | 実際の変更 |
|---|---|
function finish(){ |
function finish(){ |
$now = new DateTime(); |
$now = new DateTime(); |
if ($this->time_expire >= $now){ |
if ($this->time_expire >= $now){ |
$this->status = self::STATUS_EXPIRED; |
$this->status = self::STATUS_EXPIRED; |
} |
} |
else { |
else { |
$this->status = self::STATUS_CLOSED; |
$this->status = self::STATUS_EXPIRED; |
} |
} |
} |
} |
修正後のコードは以下の通り:
function finish(){
$now = new DateTime();
if ($this->time_expire >= $now){
$this->status = self::STATUS_EXPIRED;
}
else {
$this->status = self::STATUS_EXPIRED;
}
}
条件式が真の場合にも偽の場合にも同じ処理をする if 文を目にする機会などそうそうあるものではないため、狼狽してしまい落ち着きを取り戻すまでに10分ほどを要したのはここだけの話。 冷静になって考えてみれば、これも先ほどの例と同じく、改修に当たったプログラマが「変更量最小の法則」に従っただけのことだということに思い至るでしょう。 とは言え、
- 現在日時を取得する
- 日時の比較 (現在 ⇔ 期限) を行う
というシステムの動作に無関係となった処理が残り、それ故にこれを読み解く人間に余計な集中力を要求するこのコードの品質が最低ランクであることに疑いはありません。
我々は知的設計者たりうるか
今回取り上げたような、盲目的に変更量を最小するスタイルの改修が繰り返された結果として生成された「糞コード」を目にするときに、私の頭をよぎるのは、微小な変化の積み重ねとして進化してきた生物の身体が抱える欠陥にまつわる話です。
大きな設計上の欠陥が、その後の取り繕いによって埋め合わせられるというこのパターンは、もし本当に現実の設計者がいたとしたら、まさに起こるはずのない事態である。
(中略)私が大学院生のときの先生だった J. D. カリー教授が指摘してくれて以来、私のお気に入りとなっている例は、回帰性喉頭神経、すなわち医学で反回神経と呼ばれるものである。 これは脳神経の一つから枝分かれしたもので、脊髄からではなく、脳から直接導かれている神経である。 脳神経の一つである迷走神経はいくつもの分枝をもっていて、そのうち二本が心臓へ、二本が喉頭 (哺乳類の発声器官) に行っている。 頸の両側には、喉頭神経の分枝の一本がまっすぐ喉頭に向かい、設計者なら選んだかもしれない直接的なルートをたどっている。 しかしもう一本の分枝は、驚くほどの遠回りをして喉頭に至る。 それはまっすぐ胸まで下降して、心臓から出る主な動脈の周囲をぐるりと回ってから、頸を上に向かって逆戻りして、目的地にたどりつくのである。
もしあなたが、それを設計の産物であると考えるのなら、反回神経は設計者の面汚しである。 ヘルムホルツなら、眼のときよりももっと強硬な態度でそれを返品したことだろう。 しかし、眼と同じように、設計のことを忘れ、代わりに歴史のことを考えた瞬間に、それは完璧に理に適ったものになる。
『進化の存在証明』/ 著: リチャード・ドーキンス , 訳: 垂水雄二
リチャード・ドーキンスはこうした欠陥の存在は、生物が「知性を持つ設計者 (≒神)」によって創造されたのではないことを示唆するものであると述べています。 翻って考えるに、自然選択よろしく、全体に関しての見通しを持たず、計画を立てず、場当たり的な漸近的変化の手法に依存してプログラムの開発・保守を行い、糞コードを量産してはこれを放置するソフトウェア・エンジニアは、そして、それを漫然と許容しているIT業界は、設計者 (designer) あるいは計画者 (planner) として「知的 (intelligent)」な存在であるとは到底言いえないのではないだろうか、という疑念が持ち上がってきます。
須磨が今若い外科医を見て気になるのは、彼らの保守性だという。 彼らは失敗したときのいいわけを考えながら、失敗しないように常に何かを加えていく。だから作業量が減らない。 あれもやり、プラスしてこれもやる。そうした姿勢こそが失敗を防ぎ、二度と過ちを犯さない方法だといわんばかりにどんどん手技を加えていく。
須磨は、何も足さない。 代わりに何かを引いていく。 手術は手数と時間は少なければ少ないほどいい。 患者の負担が少なくなるから。 これが須磨の手術哲学の基本だ。 手数が増えれば、その分失敗の過程が増える。
一度手術がうまくいった時にもできるだけ同じ術式を繰り返さないよう心がける。 手術が終了したその瞬間から、この手術ではここからまだ何かを引けるのではないかと考える。 こうした哲学を、須磨はひとことで切り取ってみせる。 「メイク・イット・シンプル (Make it simple)」
『外科医 須磨久善』/ 海堂 尊
リファクタリング、あるいは要件・仕様の再整理を伴うシステムの再構築といった作業は、多かれ少なかれ、ほぼすべてのソフトウェア開発事業者が必要としながら、見ないフリをしているもののひとつでしょう。 それを限度を超えて先送りした悪影響が顕在化した例のひとつが、某銀行のシステム移行・改修にまつわる底なし沼。 そうした業務への取り組みを怠るというのは、経営におけるリスク管理の観点からも相当に重要なファクタとなるはずです。
コメント