チームメンバーの一部の書くコードがトラブルを出しすぎてまともに商用に使えないレベルなので、
半年ほど徹底的なコードレビューに取り組んできた。
どの指摘も「プログラマならできて当たり前」と思っていたのであえて教えたりしていなかったが、どうやら当たり前ではないらしい。
会社での教育資料として使えるように簡単にだが箇条書きにした。
同じようなことで悩んでいる人がいるかもしれないので、ここに転載してみる。会社ではVC++とC#でWindowsプログラムを作っているので、それを想定したもの。
○必須
・処理失敗時は必ず「失敗した処理・そのエラー内容・解析に必要なパラメータ」を入れたトレースログを出す
・入れるコードは全て自分で理解する（未理解コードのコピペをしない）※2
・APIを使うときは必ずリファレンスを読む。VC++や.NETならMSDN。※2
・メモリ・ハンドル等のリソースは確実に解放する。特に、「APIがポインタを返す」ケースと「.NETのクラスがIDisposableを持っている」ケースが見落としやすいので注意
・これから手を入れる関数の機能を明確にし、その機能を損なっていないかチェックする（デグレしないようにする）
・強制キャスト(Cのキャストと、C++のreinterpret_cast)は、特別の事情がなければ使わない。特に、組み込み型もしくは組み込み型のポインタを強制キャストしているときは9割がたミス。
・複数スレッドから同時書き込みする変数は、排他ロックする（プリミティブな型以外は、読み書きにかかわらず排他ロックする）
・必要以上にパフォーマンスを落とさないように注意する。アプリでは神経質になる必要はないが、例えば1度だけでいい時間のかかる処理を何度も呼ばれる場所に入れるようでは×。また、大量に呼ばれる処理には必要以上のトレースログを入れない。
○コードの読みやすさ（つまりミスしづらさとデバッグのしやすさと保守性）のため意識すべき
・関数・変数の名前は意味がわかるものにし、分からなければせめてコメントを付ける。例えばtrue,falseを表す変数にbFlagとか付けるとtrueが何を意味するのかわからない。例えばbPermitならまだ分かる。
・数字を直接書くことを避け、constやenumで命名して使う。
・カプセル化をする。つまり、不要な情報を外に見せないようにして結合度を下げる。
・変数のスコープは最小限にする。1つのifの中でしか使わない変数を100Stepの関数の先頭に定義したりしない。そうすることで、改修時の影響範囲が特定しやすくなり、流用ミスも避けられる。（カプセル化の一種）
・関数が何の役割を果たすかのコメントを付ける。公開関数のJavaDocコメントは必須というルールにしているが、慣れないうちはすべての関数に中身を書く前にコメントを付けると良い。
・(C++)動的配列を作るためにnewを使わない。STLのコンテナを使う。newは扱いづらい上に、必須のシーンはほとんど無い。
・(C++)文字列にはCString(ダメならstd::string)を使う。文字型の配列を使う理由はほとんど無い。
・if文のネストによるリソース解放をしない。「3段目以降のネスト通過時はCloseHandleする」のようなコードは改修が困難。
・(C++)ヘッダに他のオリジナルクラスのヘッダを入れることは避ける。参照関係が複雑になって結合度が上がるため。必要ならヘッダを入れずにクラスの事前定義をしてポインタをメンバに持つ。
・既存処理にさらに分岐を足すときは、既存部に手を入れない拡張ではなく、既存部から必要な部分を切り出して並列化する。※1※2
・(C#)古い技術に依存したコードを避け、.NET4以降の技術を使う。特に、TaskとLinqは必須。これが理解できないと新しいAPIを使うこともできない。
・(C#)エラー時の情報は例外(System.Exception)に入れる言語設計なので、これを無理にエラーコードreturn方式に書き換えない。エラーコードだけではデバッグが困難。トレースログにはSystem.Exception.ToString()を出す。
○コーディング・デバッグの速度向上のためにやるべき
・難しいロジックは結合テストをする前に、単独で呼び出せるように切り離して単体テストをする※2
・エラーメッセージを理解し、相談するときは説明できるようにする。「何かのエラーが出てビルドできません」といった発言はNG。※2
※1 つまり、既存部に手を入れずに無理な分岐を足すのではなく、既存部から分岐したい部分を切り出す。もっと具体的に言うと、A>Bという処理にBの代わりにCを実行する分岐を足す時は、A>Bという流れをそのままにして「特定条件の時だけC」という分岐を足すのは良くない。Bを切り出して、A>分岐判定>下位関数BorCのようにBとCが並列になるようにする。こうすると、BとCのどちらを通っても問題ないかどうかのチェックがしやすい。Cを通った時にBではやっていた処理が欠けているためにデグレ、といったことが起こりづらい。
※2
同じ内容をもっとわかりやすく解説したページを見つけたので、リンクさせてもらう。
http://qiita.com/hirokidaichi/items/27c757d92b6915e8ecf7
