プログラミング中級者に読んでほしい良いコードを書くための20箇条

2016年5月12日追記

よろしければこちらもお読みください→「プログラミング中級者に読んでほしい20個の心得の補足

私がコーディングする際、なるべくバグを混入させず、かつシンプルで保守性が高くなるように気を付けていることをまとめてみる。「良いコードとはなんだ」の定義については考えない。100人のプログラマーが居たら80人くらいが同意するような基準を想定している。

基礎

基本は大きく分けて二つ。コードをなるべく書かないということと、オブジェクト指向設計をするということである。

コードは書けば書くほどバグが混入する。保守性が悪化する。コードはなるべく書かないほうがいい。また、設計はオブジェクト指向を基礎とするべきだ。オブジェクト指向設計をすると再利用性と保守性が向上する。再利用性が向上すれば、コードをさらに書かなくてもよくなる。

じゃあ、そのためにどういうことを私が普段気を付けているかを次から記してみる。

1. クラス中のメソッド数を少なくする

正しくオブジェクト指向設計を行えば、1クラス中のメソッド数はそれほど多くならないはずだ。良いコードのお手本としてApache Commonsのコードを参照してみよう。すると、1クラス中の平均メソッド数は6.5くらい、全体の8割にあたるクラスのメソッド数が10以下であった。実経験的にも、おおよそこれくらいという印象である。

だから、クラス中にメソッドが増えてきたら、一つのクラスに複数の役割を持たせてしまっているのではないか?と疑うことが必要だろう。具体的には、複数のクラスをまとめてスーパークラスを抽出できないか、もしくは、別のクラスとして抽出できないか、ということを考えるべきだ。

2. メソッド中のステップ数を少なくする

それぞれのメソッドは小さい粒度の処理(つまり少ないステップ数)にしておくべきだ。例によってApache Commonsのコードを落としてきて計測したところ、メソッド中の平均行数は6.5行程度であった。また、昔読んだ本に「Linuxのカーネルソースにおける各メソッドの行数を計測すると、90%くらいが20行未満」という記述があった気がする。かなりうろ覚えだが。

経験的には、大体のメソッドが10行未満、どんなに多くても30行には満たないと思う。IDEで1画面中に収まるくらいだろうか。長いswitch文を書かなければならない例外もあるが、最近の言語であればアノテーションやアトリビュートなどといったメタデータを活用するなどして回避できると思われる。

3. クラス中の行数を少なくする

重複するかもしれないが、これも私は気を付けることが多い。これもApache Commonsを参考にすると平均204行くらいであった。私の経験上は大体500行以内に収まることが多い気がする。1000行を超えるクラスは多分作ったことがないと思う。

4. ネストを小さくする

ネスト数は小さければ小さいほどいい。ネストが深く、かつ、長いステートメントが連続しているようなコードは実質的にGOTO文を多用しているのと同じだ。私は大体、ネストが3以下になるように気を付けている。またApache Commonsの平均を計測すると、各クラスの最大ネスト数を抽出してクラス毎に平均をとると2.6であった。ネストが深くなると、人間が読んでも理解できなくなってくる。するとメンテが大変になる。

ちなみに私は最大15段ネストしているコードを見たことがある。こうなるとコードのインデントが役目を果たさないし、非常にわかりにくい。

5. 変数をむやみやたらに作らない

ちょっと複雑な処理をするとき、むやみやたらに変数を作る人がいる。「prevCnt」「nextBit」「nowGrouping」「checkFlg」「selectFlg」などという多種多様な変数を定義して、あっちに入れてこっちに入れてあっちから読みだしてこっちから読みだして、などということをする。

変数を定義した分だけ、それを管理するコストも余分にかかる。これは一つのループ処理などといったごく小さなスコープにおいても当てはまる。ループを回し、条件分岐を重ね、ある場合に更新されるべき変数が更新されていなかったためにバグが発生する、などというケースを私は何度も見てきた。

そもそも「xxxFlag」などという命名をしているような人は、経験上、ひどいコードを書く場合が多い(後述する)。

6. 便利な言語機能を使う

最近の言語は、便利で新しい仕組みが備わっていることが多い。C#のLINQ、Pythonのリスト内包表記などだ。ラムダ式やマルチパラダイムといったものもこれに含まれるだろう。こういうものをうまく用いれば、制御構文を書かなければならないケースがぐっと減る。単純に書ければその分バグも減るし、可読性も向上するし、保守も容易になるし、実装時間も削減できる。

しかし、勉強嫌いな一部のダメプログラマはこういう機能が備わっている高級言語を使用していてもいまだにforループを回してmax値を取ってきたりする。

7. ライブラリ、コンポーネントを使う

もし自分が何かやりたいことがあり、すでにそれを実現しているオープンソースソフトウェアライブラリがあれば、まずはそれを使用することを検討すべきだ。「自分で書いたほうが楽じゃーん」などと自分で実装しても余計なバグを混入させるだけである。多くのオープンソースソフトウェアはあなたが書いが少数の組織/団体/ユーザーで使われているプログラムよりも、ずっと多くの人による監視の目に晒され、丁寧にテストされ、何度もバグとセキュリティホールを修正されている。

すべてのプログラマは、自分が実装するよりも、優れたフレームワークやライブラリがすでに利用できるのではないか、と、ググってみるくらいの謙虚さを身に着けるべきである。車輪の再発明をしたうえに劣化コピーを作るようなことはあってはならない。

8. メモリ使用量とループ回数を考える

プログラマであればオーダ記法とその見積もりは当然身に着けているべき基礎である。基本情報処理技術者試験でも出てくるレベルだし、プログラムを書いて飯を食っているならば知っていて当然の概念である。

ループ処理を書くときは、自動的にオーダーいくつのアルゴリズムになるのかを考えることが望ましい。こういうことができないと、何度もDOMツリーを走査したり、同じようなループを何度も回すプログラムを書いたり、大量のタプルが途中で生成されるSQL文を書いたり、O(N^m)になるようなアルゴリズムを書いて、いざ大量の運用データを入れたら画面表示に5分もかかるシステムになったりする。

9. IOアクセスは最小にとどめる

IOアクセスは最小の時間、スコープ、回数にとどめるようにすべきだ。ほとんどのプログラムのボトルネックはIOにある。これを忘れてはならない。

たとえば、私は数GBのメモリを乗せたマシンで、高々数MBのバイナリファイルを走査して処理するのに、アクセスポインタを何度もシークして10回くらいファイル全体にアクセスするような処理を見たことがある。こういう場合は1度すべての内容をメモリに乗せてしまうべきだ。メモリに乗らないくらい大きなファイルサイズであれば、1度のシークですべて済むようなアルゴリズムにできないか検討すべきである。

10. 同じことを書かない

同じことを2回以上書かない。同じことを2回書いたらメソッド化/クラスとして抽出できないか検討する。同じことを二か所以上に書いてしまうと、そこで変更やバグが発生した場合にコードの修正がとても面倒になる。

メソッドコールが頻繁に発生することでパフォーマンスに影響する心配をする必要は、まず無いと言っていいだろう。その様なシビアなチューニングが必要になること自体が非常にまれであるし、コンパイラの最適化によってインライン展開される場合もある。

そもそも「メソッドをいっぱい呼んだらパフォーマンスが悪くなる」とすれば、それはコーディングで解決すべき問題ではなくコンパイラが解決すべき問題である。自分のやるべきでないことに手を出す必要はない。例外的に必要性があるケースは前述したように非常にまれである。

11. なるべくテスト可能なコードを書く

クラスとメソッドは、ユニットテストツールによってテスト可能になっていることが理想である。テスト可能であるとは、ユニットテストフレームワークからすべての操作を触れるということである。テスト可能でないとは、実際にプログラムを動かし、操作しないと試験できないような状態を指す。より具体的に言えば、クラスAの試験するときに、アプリケーションを起動して、ボタンAを押し、テキストボックスBに文字列Cを入力してOKを押す・・・という方法以外に試験を行うすべがないような状態である。デスクトップアプリであれ、Webアプリであれ、こういう実装になっているケースはとても多い。

ポイントは、クラス間、モジュール間の依存性をいかにして小さくするか、である。片方のクラスを動作させるためにもう一方のクラスが必ず必要になるような設計はあまりよくない。インタフェースや抽象クラスなどを利用してオブジェクトの抽象度を向上させてやるのが一つの解決策になる。

・・・と簡単に書いたがこれは本質的に高度で難しい作業である。なので、「なるべく」と書いた。

(補足)
最初、この節は「単体テストを書く」にしようとおもった。単体テストを書き、テストケースをメンテし続け、Jenkinsなどのツールを使って継続的に健全性を保ち続ける・・・というのが理想ではあるが、実際にはそこまでやっている費用がなかったり、単体テストを書いてもテストケースがその後メンテされていなくてテスト失敗したりというケースが多い気がするし、前述したようにそもそも相互依存性の低いクラス/モジュールを設計することがそれなりに難しいため、より敷居の低い「なるべくテスト可能なコードを書く」というかなり控えめな表現にした。

12. 送り出す結果は厳密に、受け取るデータは寛容に

ネットワーク通信や他アプリのためにデータ出力するようなプログラムを書く場合で、かつ、相手のプログラムを書いているのが違う人/異なる組織である場合はこれがとても重要だ。送り出すデータは定められたインタフェースに厳密に従った形式にすべきである。データを受け取る場合は、インタフェースから多少ずれたデータが来ても寛容に処理してあげるという気持ちが大事であろう。

しかし、これはあくまでも基本的な原則だ。あまりにも寛容に処理してしまうと仕様に従った処理をしているとは言えなくなってしまうし、そもそもインタフェース仕様が厳密に決まっていないケースも多いだろう。そういう時は、あいまいな言い方になってしまうが、その組織の文化や相手の知識レベルに応じてケースバイケースの対応が必要になる。

13. シンプルなインタフェースを心がける

メソッドやコンストラクタの引数、送受信するデータは必要最小限の情報をもっとも簡潔な表現で表した形であるべきだ。その理由は言うまでもないだろう。

たまに、無理やりIDEのリファクタリング機能をつかってメソッドを抽出したかのようなコードに出くわす。たとえば、JavaでStringBuilderを引数にとるメソッドがあり、中身を追っていくとただ単にメソッド中でバッファとして利用しているだけで外部には全く影響していなかった。こういう変数のスコープはそのメソッド内部のみにとどめるべきである。それを面倒臭がって放置しておくと、どんどん保守できないコードへと変貌していく。

14. 複雑な内部状態を定義しない

カプセル化は「臭い物に蓋をする」ための手法ではない。変数をprivateで宣言すれば無制限に増やしていいわけではない。変数を増やせば増やすほど、そのクラスの取りうる内部状態は増えていく。管理の手間もテストの手間も指数関数的に増加していく。

わかりやすく例を示すと、初心者が書きがちなコードとして、クラス内部に「xxxFlag」などというブール型の変数を多量に宣言してしまうものがある。たかが二値であっても、変数の数Nに対して内部状態は2^nに爆発してしまう。現代的な高級言語を使う状況でそもそもいわゆる「フラグ」を利用するケースなど殆ど無いと言える。

クラスの内部変数が多くなったときは、むやみにスコープを広げてしまっていないか(本来のスコープはメソッド内ではないか)、複数のオブジェクトを一つのクラスとして表現していないか、スーパークラスを抽出できるのではないか、を検討してみるべきだ。

15. コメントをなるべく書かない

これは賛否両論あるだろうと思う。私の周りでも「コメントは積極的に書くべき」という人が何人もいる。その多くの理由は、「書いたほうがわかり易いから」というものだ。

しかし、私はなるべくコメントを書かないようにしている。というかより正確に言えば、良いコードは自然とコメントの量が少なくなると思っているので、コメントがたくさん必要になったときはわざわざ遠回りして複雑なことをしていないか?と疑ってみるべきだ、という意味になる。また、「コメントを書かないとわからないようなコードを書いているほうがそもそも問題」とも思う。

前述したように、「良いコード」の一つのメソッド中の行数は大体10行以下、多くてもほとんどが20行以下くらい、という点を示した。一つのメソッド中のステップ数が数ステップであれば、ふつう、見ただけで処理の内容が把握できるのでコメントはいらない。これが理想の状態であると思う。

ただし、コメントが完全に不要なわけではない。コメントを書いたほうが良い場面もある。たとえば、使用しているライブラリのバグなどが原因で特殊なコードを書かなければならない場合とか、意図的にコンパイラ警告や例外の送出を黙殺したりする理由を示す場合などだ。

以下に、Apache CommonsのMapUtilsクラスの実装の一部を示す。

@SuppressWarnings("unchecked") // As per Javadoc throws CCE for invalid array contents
public static <K, V> Map<K, V> putAll(final Map<K, V> map, final Object[] array) {
    map.size();  // force NPE
    if (array == null || array.length == 0) {
        return map;
    }
    final Object obj = array[0];
    if (obj instanceof Map.Entry) {
        for (final Object element : array) {
            // cast ok here, type is checked above
            final Map.Entry<K, V> entry = (Map.Entry<K, V>) element;
            map.put(entry.getKey(), entry.getValue());
        }
    } else if (obj instanceof KeyValue) {
        for (final Object element : array) {
            // cast ok here, type is checked above
            final KeyValue<K, V> keyval = (KeyValue<K, V>) element;
            map.put(keyval.getKey(), keyval.getValue());
        }
    } else if (obj instanceof Object[]) {
        for (int i = 0; i < array.length; i++) {
            final Object[] sub = (Object[]) array[i];
            if (sub == null || sub.length < 2) {
                throw new IllegalArgumentException("Invalid array element: " + i);
            }
            // these casts can fail if array has incorrect types
            map.put((K) sub[0], (V) sub[1]);
        }
    } else {
        for (int i = 0; i < array.length - 1;) {
            // these casts can fail if array has incorrect types
            map.put((K) array[i++], (V) array[i++]);
        }
    }
    return map;
}

これはMapとObjectの配列を渡して、うまい具合に型を判別してMapに配列要素を入れてもらう、というユーティリティメソッドである。このコードにはいくつかコメントがあるので、それを読んでみよう。

まずはじめに、SuppressWarningsアノテーションにつけられたコメント、As Per javadoc ~というのは、訳すと「Javadocに書いてあるように、不正な型の配列が与えられたときはCCE(ClassCastException)を投げますよ」という意味である。つまり、ClassCastExceptionを送出するのは仕様ですよ、だからコンパイラが「Objectをキャストする時に型チェックしてない」という警告を無視しますよ、という意味である。上記では長いので乗せなかったが、ちゃんとjavadocには「不正な型が来たときはClassCastExceptionを送出する」と書いている。こういうのはコメントに書いておかないとわからない。コメントに書くことが望ましい一例だ。

その下のmap.size()に付属している「force NPE」というコメントは直訳すると「強制的にNullPointerExceptionを出しますよ」ということになる。どういうことかというと、mapがnullであるか否かをチェックし、nullならばNullPointerExceptionを送出するということをやっていますよ、という意味だ。

普通にnullチェックをするならば、

if(map == null)
    throw new NullPointerException("nullはダメ");

などと記述するところであるが、そもそも引数のnullチェック程度で新たにエラーメッセージを定義してExceptionを作るのは手間の割に意味がないし、記述も条件分岐が一つ入って長ったらしくなる。

その点、map.size()を一行書けば、mapがnullであったならば自動的にNullPointerExceptionが送出されるので簡単だ。size()メソッドは内部のプライベート変数から値を読みだして返却するだけなので、パフォーマンス上の影響も無い(もしかしたら上記コードより早いかもしれない)。

しかしながらこれはテクニックであるので、上記のようなことを説明されないと意味が分からないだろう。だからコメントは書いてしかるべきである。

それに続いていくつか、「ここは上で型チェックしているのでキャストしてもOKだよ」「ここは型が間違ってたら失敗するよ」と書いている。このコメントは書かなくてもよくコードを読めばそれがわかるが、「よくコードを読む」ためにかかる数秒~十数秒が節約できるので、「必須ではないが、書いといたほうが親切」というレベルであると思う。

コメントというのは、本来上記のような限られたケースでのみ必要になるものである。「データを入れる」「加工する」「表示する」なんてコメントは無駄であるか、そもそも必要になること自体がおかしい(つまり複数の処理をだらだら一つのメソッドに書いている可能性がある)。

16. XMLドキュメント(javadocなど)を書く

上記でコメントはなるべく書かないように、と述べたが、Javadocはできるだけ詳しく書くべきだ。これはもちろん後々の保守のためとか、あとで仕様書に仕立てたいとかいう理由もあるが、一番は「このメソッド/クラスに何をさせたいのか」という点を自分で明確にしておきたいためである。

17. コメントアウトしない

とりあえずコードを修正したけど、もしかしたら元に戻すかもしれない・・・。もしくは、修正したけど、他人の書いたコードを消すのは忍びない・・・。はたまた、とりあえずなんとなく・・・程度の理由で、変更前のコードをコメントアウトして残しておく人をよく見かける。

これははっきりと無意味である。コードの変更管理はすべてGitやSubversionといった構成管理ツールに任せるべきだ。コメントアウトするとコードのインデントが合わないように見えてしまうし、エディタ上で邪魔だし、変更を繰り返した後だとコメントを外してもそもそも動作しないコードになっている。何の利点もないし、害悪だらけである。

18. コミット時のコメントをちゃんと書く

リポジトリにコミットするときのコメントはちゃんと書くべきだ。プロジェクト管理上の重要なパラメータ、つまり、どの機能がいつ実装されたのか、どのバグがいつ、誰によって修正されたのか。こういう情報を入力するのにとても適している機会がコミット時のコメント入力である。

たとえば、Redmineでは、コミット時のコメントから修正に要した工数などを拾ってきてシステムに自動的に入力させる機能なんかがある。GoogleのBugspotsというバグ予測ツールを利用するには、コミット時のコメント情報を入力している必要がある。

19. 良い名前を付ける

たとえば「xxxFlg」なんて変数は最悪だ。たとえば、bool型のwriteFlgという変数があり、これがどういう状態を示すものであるか瞬時に理解できるだろうか。書き込み中のフラグなのか、書き込み完了を示すフラグなのか、書き込みが必要なことを示すフラグなのか・・・。どうとでも解釈できる。で、もしこれが読み込みが必要なことを示すフラグだったとすると、今度はtrueの時に読み込みが必要なのかそれともfalseの時なのか、という判断でまた悩む羽目になる。

変数名、クラス名、メソッド名は誰が読んでもよくわかる名前にすべきだ。

たとえば、書き込み中を示すフラグならばisWriting。書き込みが必要なことを示すフラグならばwritingNeeded(それほど重要でないケースではshouldWrite)。書き込み完了を示すフラグならばhasWritten、のように書くべきだろう。そのとき定義する名前に対して何がふさわしいかというのを判断するのは難しいが、たとえば言語の標準APIや、有名なライブラリなどを命名を参考にするとわかり易い。また、codicという変数名命名のために使える辞書サービスなんかもあるので、これを参考にしてもよい。

20. オブジェクト指向設計する

やっとここまで来た。オブジェクト指向言語を使ってオブジェクト指向設計したプログラムを実装するというのは、実はそれなりに難しい作業である。オブジェクト指向言語を使ったらオブジェクト指向設計に自動的になるわけではない。

私が今まで見たプロジェクトだと、クラスをせいぜい「画面/機能/処理層ごとに処理をカテゴライズしたもの」として認識・使用していることが多いし、staticメソッドを30も40も集めたクラスがわんさかある、というパターンがほぼ100%であった。クラスの継承も「共通部分を抜き出す」という手法を実現するためのみのテクニックとして使われている場合が余りにも多い。

こういう設計方法は少なくともオブジェクト指向でないため、オブジェクト指向の恩恵、すなわち、高い保守性、再利用性といったものが得られない。だからバグ改修も大変になるし、同じコードを何度も何度も書く羽目になる。

クラスとインスタンス、継承、インタフェースと抽象クラス、こういった基礎を本を読んで理解したのち、有名なオープンソースプロジェクトのコードを落としてきて、見て、使って、改造してということに一通り慣れておくとよいだろう。すると、主要なデザインパターンのいくつか自分で発明・発見できるくらいにはなる。逆に言えば、このようなプロセスを踏まない限り、真にオブジェクト思考を理解したとは言えないと思う。

まとめ

上記に挙げたことは、すべてが必須で、経験を積んだプログラマであれば全員が同意する、という類のものではない。また、まだまだ私が忘れているために抜け落ちている重要な点が多いだろう。

ただ、ここで挙げたことは、ちゃんとした本を読んで勉強し、他人が打った有名なソフトウェアプロジェクトのコードを見て、読んで、改造してみて、自分でも打ってみて、ということを経験すれば自然に達する結論であると思う。なので、これらすべてを覚えてほしいという意図はない。

2件のコメント

  1. グラマ より:

    フラグが全く必要ない?は?え?
    オープンソース読んだことある?めちゃ使ってるよね?w
    あんた歳いくつ?

    1. withpop より:

      > オープンソース読んだことある?めちゃ使ってるよね?w
      面倒でなければ例えばどういうプロジェクトか教えていただけないでしょうか。
      ソース出せ、ということではなくて純粋に興味があるので読んでみたいのですが。

ただいまコメントは受け付けていません。