前書き
みなさま、プログラミングを楽しんでますか？
私は1994年にプログラマーとして就職して以来、ずっと飽きることなくプログラミングを楽しんでいます。
今やアラフィフですが、iOSアプリ開発者として毎日バリバリコードを書いています。
…が、まだコードレビューで情けないケアレスミスの指摘をいただくことがあります。
本来コードレビューで議論すべき観点1に集中するために、コードレビューを依頼する前にケアレスミスがないかセルフチェックすることは大事ですよね。
ということで、自戒の念を込めて
「コードレビュー依頼前にケアレスミスを見つけるためのセルフチェックリスト」
を作成してみました。
「仕様に則していること」とか「規約に則していること」とかは自明なので割愛しています。
もし「こういうポイントもセルフチェックすべき」という項目がありましたらコメント等で教えていただけると幸いです。
チェックリスト
- スペルミスがないか
- デッドコードや意味のないコメントアウトが残っていないか
- コメントがコードと食い違っていないか
- コピペした箇所を見直したか
- インデントがズレていないか
- コンテキストと名前が一致しているか
- 型が大き過ぎないか、関数が長すぎないか
- 一行が（横に）長すぎないか
- IDEに警告が発生していないか
- 横展開調査したか
- 既存の共通処理があることを見逃していないか
各項目の理由や背景など
スペルミスがないか
- ケアレスミスの代表的なヤツです。
- スペルミスが致命的なバグの直接原因となることも珍しくないですが、後からコードを検索するときにヒットしてくれないことも地味にヤバいです。
- 英単語のスペルは、少しでも不安があったらググりましょう。
デッドコードや意味のないコメントアウトが残っていないか
- 「試行錯誤の段階で書いたコードがそのままレビュー依頼時まで残ってしまった」というのが、ありがちなパターンです。
- **後々コードを読んだときに、試行錯誤コードの意図が読み取れません。**後々に誰かに「削除すべきに見えるけど、削除してバグったらイヤだな…」と余計な手間をかけさせます。
- また上の項目と逆に、後からコードを検索するときにヒットしてしまうことが地味にウザいです。
- 私は試行錯誤のつもりで書くコードにはあらかじめ、
TODO: 検証用コードみたいな自分なりのマークを決めて、コードレビューを依頼する前に検索したりします。
コメントがコードと食い違っていないか
- ありがちなパターンは、「コピペしたときに、コメントとコードをコピー元からコピペして、そのコメントがペースト先にはマッチしていなかった」という場合です。
- これも後々に誰かに**「コメントの誤りのように思えるけど、実は何か裏事情があるのだろうか…」と余計な手間をかけさせます。**
コピペした箇所を見直したか
- 実装の意図を誰かに問われた時「あ、それは○○からコピーしただけです（だからツッコまないでください）」と答えていませんか？
- 第一に、ロジックが共通であれば共通化を検討するべきです。2
- 第二に、コピー元のコードはテストされてうまく動いていてもペースト先で期待通りに動作する保証はないので、十分に検証すべきです。
- そのようなコピペを許す文化になると、そのコードはまた誰かによって深く考えることなくコピーされます。そうやってリポジトリがカオスに向かいます。
- ということで、コピペにトラップが潜んでいることは多いですので、コピペした箇所は綿密に見直すべきです。
インデントがズレていないか
- コードの体裁も、命名と同じく後々にコードの意図を理解するスピードに影響を与えます。
- これは変更差分だけでは気づかない場合もあるので要注意です。
- これもやはり、コピペ起因のことが多い印象です。コピペ元と先でネストの深さが違う場合にズレてしまうということです。
- たいていのIDEには自動整形機能がついているので、自動整形のショートカットを指に覚えさせておいた方が良いです。
コンテキストと名前が一致しているか
- クラスの名前と責務、関数の名前と処理内容、変数の名前と用途、です。
- 後々に誰かがコードを読むとき、名前から多くの情報を得ます。プログラムは書く時間より読む時間の方が格段に多いため、命名は生産性や品質に多大な影響を及ぼします。
型が大き過ぎないか、関数が長すぎないか
- この辺りはできれば、Linterライブラリを入れて機械的にチェックすべきです。
- 目安としては以下の通りです。3
- クラスや構造体の行数は200行以内
- 関数の行数は40行以内
- この問題のテーマは行数が長いことで理解が困難になることだけではなく、**本質的には「責務が大きすぎる可能性があるので分割を検討しましょう」**ということです。
- その観点では、一人で悩むより早めに第三者の知恵を借りることもアリかと思います。
一行が（横に）長すぎないか
- 目安としては 120文字以内 です。3
- フレームワーク等により自動生成されたコードも長い場合もあるので、そこまで重要性が高いわけではないですが、やはりコードの読み手への気配りは心がけとして大事かと思います。
IDEに警告が発生していないか
- Linterを入れていても、Linterによる警告を無視してしまうと台無しです。
- いわゆる「割れ窓理論」というヤツで、「この軽微な警告ぐらい…」と見過ごしてしまうと、すぐに警告数が何十・何百個となり、本当に重要な、対処すべき警告を見逃してしまう場合があります。
- Linter以外にも、言語やIDEによって、未使用の変数であったり、メモリリークであったり、いろいろな警告を出してくれると思いますが、有識者の先人の知恵によるものなので、基本的には全て対処すべきと考えます。
横展開調査したか
- 特に新規ではなく改修の場合、修正すべき箇所が他にないかプロジェクトソース全体に対して検索をかけて調査した方が良いです。
- 手を入れて「いない」箇所が問題ないかという視点は実務的に大事だったりします。
既存の共通処理があることを見逃していないか
- こちらも上の項目と類似で、「書いたコードは正しいが、そもそもコードを書く必要がなかった可能性はないか？」という視点です。
- 共通処理がすでにあるのに自前で実装を増やしてしまうと、将来の仕様変更時に横展開調査・改修が必要になってしまいますし、漏れる危険性もあります。