オフィス増床により快適な環境で業務に取り組めている nazo です。
社内でコードレビューする時に、「ここは絶対指摘するよな」とか「ここは微妙かも？」みたいなところを思いつく限りでまとめてみました。
- コメントが書いてない、スペルミス、規約違反などの基本的なことには触れません。規約などは CI でチェックできる環境を用意し、人力でチェックする負荷を最小限にしましょう。HoundCI や SideCI のような外部サービスを利用するのも良いかと思います。
- あくまで「個人的な意見」です。一般的にこうだというわけでもありませんし、「うちではそんなことしないしそれは間違っている」というのであれば、それはそれでいいと思います。様々な論点があると思いますので、一例としてご覧頂ければと思います。
コメントが直訳になっている
とりあえず基本ですが、直訳のコメントは必要ありません。コメントはコードの内容を説明するのではなく、コードが存在する理由など、コードから読み取れない点を中心に書くようにしましょう。
例としては以下のようなものがあります。
// CSVをパースする func ParseCSV(csv string) : []Row {
この例だと返り値の型からある程度想像はできますが、一方で CSV をどうパースするのかまでは記述されていません。例えば、１行目はヘッダ行だから飛ばす、空行があったらどうする、エラー行はどうする、重複行はどうする、といった情報が必要になると思います。それらの情報があると、より誰にでも使いやすく、丁寧なメソッドになったと言えるかと思います（それらが必要かどうかも内容によると思いますが）。
また、公開メソッドのコメントを書くのが面倒で、 getUser のコメントが get user になっていたりすることもあります。getUser なら見ればわかるような気もしますが、「見ればわかるじゃん」がいつのまにか拡大解釈されて、本人以外意味のわからないものなのにコメントがないということが発生することもあるので、正しく書く癖を付けておいたほうがいいです。
公開メソッドやクラス等は、基本的に「誰かに利用されるために作るもの」です。自分以外の人間が理解できるか、自分自身も１年後に読んで理解できるか、よく考えた上で記述しましょう。
コメントアウトを残している
コメントアウトを残すと後でそのコメントアウトが何の意味があったのかを把握するのは困難です。また、残っているものを後で削除するのは心理的ハードルが高いです。
int process(int n) { n = n * 2; // n = n + 1; return n; }
例えば上記のコードだと、+1 が何のために必要だったのかがわからず、このあたりでトラブルが発生した時にこのコメントアウト部が必要だったのか、という余計な思考が発生します。実際に必要だったりすると更に厄介です。
コメントアウトは原則残さず、どうしても残したい場合（何らかの理由で作業中など）は、その旨をコメントで書いておき、後日責任を持って処理しましょう。
変数名の意味がわからない
ちょっと極端な例ですが、以下のようなコードがあったとします。
datetime = service.getDateTime() return render_template('index.html', datetime=datetime)
この場合、 datetime 変数が何を意味するのかわかりません。 getDateTime() もよくわからないですね。日付が datetime なのはあまり重要な情報ではなく、これが何の用途のものなのかを的確に表すほうが重要です。当然、 getDateTime() は、何の目的の日付を取得するのかを表す必要があります。
このあたりの項目に限らず、リーダブルコードはコードを書く上の要点がまとまっていて、品質の向上に役立つ情報が多いです。弊社でも読書会が開催されています。
突然見たこともない要素（ライブラリ・ミドルウェア等）が出てくる
個人開発と違い、社内での開発は、知識も技術力も様々な人間がサポートを行う可能性があるため、ライブラリ・ミドルウェアの選定は慎重に行う必要があります。
- それがないと絶対にできないことはあるのか
- そうでない場合、それがどのくらい広範囲で開発効率を向上するのか
- 同種のライブラリや、既存の機能（あれば）との違いは何か
- 既存のもので代用できる場合、代用しない（＝新しいものを導入する）ことでどれだけのメリットを得られるか
- 採用する場合、責任を持って利用価値と利用方法を広めることができるか
以上を踏まえた上で、それでも必要性を感じられる場合は導入すべきです。
一方で、あまり萎縮しすぎてあれも駄目、これも駄目となってしまわないようにする必要があります。合意がスムーズに進むように、普段からメンバーと会話して共通認識を持っておくようにしましょう。特に Web の世界（ブログや SNS 等）では次々に新しいものが紹介されているので、そういうものに流されず、しっかりと検証して利用することが大事です。
データベース設計の変更
データベース設計は大事ですが、残念なことに何らかの理由で設計の変更が入ることが避けられないということは少なからず存在します。
設計の変更は、特に運用中は大きなリスクを伴います。MySQL の場合、オンラインでのスキーマ変更の充実や pt-online-schema-change 等により、変更のリスクは大幅に減りましたが、それでもプログラム側との情報のずれによって停止してしまう可能性などを考える必要があり、簡単に行かないことが少なくないです。
変更を入れないように早い段階から設計を詰めておくことは大事ですが、変更しないといけない場合は、どのくらいのリスクがあるのか、場合によってはメンテナンスが必要なのかを検討しましょう。またメンテナンスを入れる時、一部分の機能だけメンテナンスを入れれるような設計にしておくと更に良いです。
関数内が長すぎ
ベタですが、ぱっと見て長過ぎると感じる場合、大体何らかの問題があります。
「単一責任の原則」などにもあるように、１つのメソッドで行う処理が複数存在するのは良くない設計と言えます。機能が少なければコードも比較的少なくなると思われます。コードを書いている時に「ついでに」という単語が出てこないように注意しましょう。
特に、ドメイン駆動設計で登場する「声に出してモデリングする」という表現はとても良いと思っており、不自然な設計の場合によく引用させていただいております。指摘する時も「関数名はこういう意味なのにこの機能はそれから逸脱してますよね？」みたいなことを言うことが多いです。
（プログラム以外の）説明文/コミットメッセージがわかりづらい
Pull Request を提出する時に、説明文も重要です。特にレビュワーが仕様を把握しているとは限らないので、レビュワーにわかるように伝えることが大事です。コミットメッセージも同様です。
駄目なタイトル/コミットメッセージの例としては「バグの修正」が代表格です。「何がまずかったのか」「何を解決したのか」が簡潔にわかるような内容にすべきです。行ったテストの内容も書いておくと良いと思います。
github.com では、現在テンプレート機能が追加されたので、積極的に利用し、説明文を充実させましょう。
バグの修正にテストが含まれていない
既発のバグということは、普通の現象より再発の可能性が高いです。
バグの修正の場合は、基本的に「修正前にエラーになり、修正後に通るテストコード」を添付してもらうようにしています。これにより、再発の可能性を大幅に減らすことができます。
コーディングしすぎ
設計を固めるのは大事ですが、固めすぎて「今それは必要ないだろ」というところまでコーディングしてしまっている事例がいくつか存在します。将来的に使う可能性が極端に低い機能を先に実装してしまう、などです。
使わない機能でプログラムが大きくなりすぎると修正が困難になっていきます。また、業務での開発ですので期日もあります。どれが必要でどれが必要ではないかは内容によりますが、過剰なコーディングは行わないよう、日常的に相互に進捗を確認するようにしましょう。
まとめ
ここでは社内で実際に起きたレビューの例を掲載しました。もちろんこれが全て正しいというわけではないと思いますし、これ以外のパターンもあると思いますが、一例としてレビュー時の参考にしていただけばと思います。チームメンバー間でこのようなことを話し合っておくと、お互いのレビューの時間を削減することができ、開発の効率化に繋がるのではないかと思います。
スマホアプリ、ゲーム、SPAのようなプレゼンテーションにロジックが入るアプリケーションでも大きくは変わらないですが、レイヤが１段階増え、常時動作しているような処理なども入ってくるため、それらの分離が行われているかという点や、状態管理が正しく行われているか、などのチェック項目が増えてきます。機会があればそれらも紹介したいと思います。
UUUMでは、的確なコードレビューで品質を向上できるエンジニアを募集しています。詳しくは以下からご覧下さい。今なら広くなったオフィスで快適に業務に取り組めます！