リーダブルコードを読んだ
苦い思い出が蘇る書籍
より良いコードを書くには、ということで手に取ってみた書籍ですが。例題に書かれているコードを見て「あー。業務でもあるあるネタだなぁ（業務はもっとひどいけど）」という苦い思い出が蘇ることとなりました。
せっかく読み終わったので、自分がどんなコードと出会って、何に問題を感じて、どう対処したのかを書籍の内容を交えつつ書いてみます。
コメントによるバージョン管理
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 |
public static String doSomething() throws Exception { doSomething1(); // 2000/01/15 version2.0.1 修正スタート doSomething2(); // 2000/01/15 version2.0.1 修正エンド // 2000/01/15 version2.0.1 修正スタート //doSomething3(); // 2000/01/15 version2.0.1 修正エンド return doSomething4(); } |
出会ったプロジェクトで、Subversionを使用しているにもかかわらず（！）修正日時とバージョンをコメント文で管理している文化がありました。修正範囲を示すコメントはコードのネストを無視して先頭列から記述が始まり、削除の場合でもコメントアウトで対処している箇所がありました。
このルールにより、私がプロジェクトに参加した時点でどうなっていたかと言うと、見事なまでに修正コメントがミルフィーユのように重なっているコードが出来上がっていました。浄化するルールがないためコードは増え続ける一方でありながら何も対処されません。（コメントがミルフィーユのように重なっているという言い回しは結構気に入ってます。どういう状況かが想像つくので。）
コーディング規約を参照するも、コメントによるバージョン管理の規約について書かれておらず、当時プロジェクトに関わっていたメンバー何人かに尋ねてみるとおおよそ「何か問題が発生して原因となるコード行を特定したときに、すぐにいつの開発の修正か、開発担当者がだれかを特定できるようにするため」と言った回答が返ってきました。
なぜこのようなルールができてしまったのかを推測してみます。おそらくですが、リーダブルコードで言う「能力が低い人に合わせる」文化が形成されてしまっており、「とにかく時間がない」状態だったためその時コメントによるバージョン管理をレビューする暇もなく、さらにその状態が踏襲され現在に至った。という流れでしょうか。
今は修正範囲コメント含む不要なコードは削除するようにしています。また、Gitに移行してPRを送るタイミングでコードレビューする文化を作り始めています。不要なコメントが生み出されても直ぐにレビューの指摘で削除できます。
不要なコメント
|
1 2 |
// インスタンスの生成 Hoge hoge = new Hoge(); |
インスタンスを生成しているのは見ればわかります。
|
1 2 3 4 |
public class Fuga{ // コンストラクタ Fuga(){} } |
知ってますって！
これ、製造者が単語を覚えたての時にこのようなコメントを書くケースが多い気がします。「この記述はどういう意味だったか忘れないようにメモしよう」的な。後の人が単語を検索して記述の意味まで理解できるようにという思いも含まれているかもしれません。（まずはOOPの勉強しよう！と言いたいところですが）。これはレビューで有識者が指摘して削除しておきたいところ。
上手なコメントの書き方って習う機会が無いんですよね。情報系の学部でしたが、授業では「後から見た時に何をやっているのかがわかるように書きましょう」というご指導だけでした。授業で書くようなプログラムはアルゴリズムがほとんどなので、対象行が何をやっているのかを事細かに日本語でコメントを書いていました。
ネストが深い（Doom of Pyramid）
コメントネタは尽きないのですが、ちょっと別のネタもご紹介。
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 |
if(isHoge()){ if(isFuga()){ // 特になし }else{ piyo(); } }else{ if(isFuga()){ // 特になし }else{ foo(); piyo(); } } |
はじめは以下のようなシンプルなコードだったはずです。（ifスコープに何の処理も記載無いのはまた別の問題として）
|
1 2 3 4 5 6 |
if(isFuga()){ // 特になし }else{ foo(); piyo(); } |
しかし、後の改修で処理（isHoge()がtrueの場合、foo()を実行してほしくないという要件）が追加されたのでしょう。ちょっとした追加だし時間もないのでまあいいか、と当時の担当者は全体最適をせずにif文を追加して死のピラミッドを1段重ねることとなりました。
「まあいいか」が積み重なり、今では10段近くのピラミッド。循環複雑度は90を超えており、いかなる仕様変更もバグを生み出すといわれる値まで成長していました。設計書や試験についてのレビューは入念に実施するプロジェクトでしたが、コード自体のチェックは緩かったためこのような自体となったのでしょう。
ライブラリを知る
Mapを知っていれば。。。
|
1 2 3 4 5 6 7 8 |
private List<KeyValue> list; private String getValue(String key){ for(KeyValue keyValue: list){ if(keyValue.getKey().equals(key)) return (String) keyValue.getValue(); } return null; } |
わざわざ専用のKeyValueクラスを作成してforループでkeyを探索しています。この方法だと処理はO(n)と遅くなりますし、これだけの処理のために理解に時間をかけてしまいます。担当者がMapを知っていれば、コード自体のレビューを行っていれば、という気持ちになりますね。
例として共通関数化したコードを書きましたが、実際は上記のコードが約100箇所に渡ってコピーされ直接使用されていました。とりあえず前回の作りを踏襲することで指摘を逃れられるというのも問題があるような。
レビューしよう
というわけで自分が最近取り組みたいこととしては
- コードのレビュー文化を浸透させよう。
- コーダーに循環複雑度を意識してもらおう。
あたりかな。と思いました。
リーダブルコードおすすめなので是非読んでみてください。