どうも初心者エンジニアのみなさん。日々職場で、先輩レビュワーから「ここはなんでこう書いてるの？（このコードはクソだねぇ...）」と言われていることかと思います。
僕はコードを書き始めて2年ですが、コードレビューをされた経験は1年と少し程度でまだまだクソコードを量産する日々です。先輩エンジニアのレビュー時間を無駄にしまくっている自分に日々鬱々としています(´-ω-`)
しかも、初心者エンジニアとしてはありえないくらい恵まれている環境で、今レビューをしてもらっている方がエンジニア歴20年の大ベテラン。
初心者エンジニアは自分の無知を知ろう
人間の年齢に表すと、20歳の成人式を終えた人が、そろそろ立ち始めるかな？という2歳児を相手にフィードバックを与えるようなものです。
立つ方法も、服を着る方法も、軽快に話す方法も20歳から見ると何もかも未熟で、何から指摘すればいいのか分からない...。という状況になってしまう。
これがコードレビューにおいても言えます。エンジニア歴20年のベテランから見ると、エンジニア歴2年の書くコードは、ツッコミどころがありすぎてレビューが膨大になること必至。
「なんでこの内容のコメントしたの？これ読んでなんの役に立つの？」
「なんでこのライブラリのメソッド使ったの？こっちのライブラリの方が効率いいね。」
「ユニットテストの網羅性ない。どうやってテスト項目決めた？カバレッジ確認した？」
「変数の定義場所と呼び出し場所離れすぎ。スコープって知ってる？」
「なんでこのクラスにメソッド書いた？役割的にこっちのクラスに書くべきじゃない？」
「なんで何度も呼ばれるメソッドの中で同じ値の変数初期化してる？定数使わないの？」
（...このあと194個ほどレビューコメントが続く。そしてGithubのページは重くなっていく...。）
さすがに200個もレビューコメントをするのも、レビューコメントを直すのも大変ですよね。今回は、2歳児が200個もレビューコメントを受けないように。最低限50個くらいのコメントに減らすために、事前に知っておくべきことをまとめておきます。
初心者エンジニアはこれらを知っておくだけでも相当数のレビューコメントを減らし、チーム内のレビュー工数を削減することができると思います。
レビュー依頼前に確認するべき23項目
ここからは初心者エンジニアが先輩エンジニアにレビューを依頼する前に確認しておくべきことをまとめていきます。
① 変数・クラス・メソッドの命名は適切か？
クラス・メソッド・変数など、それぞれの役割を的確に表した命名をする必要があります。また、その命名が文法的に間違っているものではダメで、文字数も多すぎるとダメです。さらに、自分の関わっているプロジェクトのソースコード中で使われている命名規則から逸脱するものであってもいけません。
検索を行う場合はselectなのか、searchなのか。データの登録をする場合はupdateなのかinsertなのかregisterなのか。検索して値があれば更新し、値がなければ新規登録するメソッドはsearchUpdateなのか、searchRegisterなのか、selectRegisterなのか。考え始めるといつまでも考えてしまい、いつになっても最適な命名がなんなのか迷ってしまいます。
命名時に考えるべきこと
- 役割を明確に表しているか。
- 英語として文法がおかしくないか。
- 適切な短さの文字数になっているか。
- プロジェクトの命名規則に則っている（他の似た意味合いを持つものと同じ命名になっている）か。
以上のことを満たしている命名である必要があります。最初は適切な命名に膨大な時間がかかります。
おすすめなのは、公式のライブラリの命名やプロジェクトにある既存の命名を参考にする方法です。公式のライブラリの命名は多くの優秀なエンジニアのレビューにさらされているので、あなたがちょっと考えた命名より数倍的確なものになっています。また、既存のプロジェクトの命名を参考にすることで、他のコード箇所との意味の整合性を取ることができます。
とはいえ、既存のコードをそのまま使うのは危険です。既存のコードはレビューを経ているとはいえ、それは時間に追われてさっと目を通されただけの準クソコードかもしれません。しっかりと自分の命名に理由と自信を持てるまでは頭を働かせる必要があります。
② 処理は適切に共通化されているか？
同じような処理を行なっている箇所は、共通化しておきましょう。メール送信の処理を実装するときに、メールの件名と本文を作成し、誰かに向けて送信するという処理は、メールのサービス種類によらず共通なはずです。その共通の箇所をコピペを駆使してソースコード中に乱立させると、散らかった部屋のような状態になってしまい、のちのち処理を修正する必要が出てきたときに一度乱立された処理を整理してから修正する必要が生まれます。
目の前の処理の最適化だけではなく、今後の拡張性も考えて、このメソッドは将来的にどのように使われうるかな？同じような処理を複数箇所に書いていて後から修正するときに困らないかな？と常に考えてコードを書くだけで、未来の自分もしくは後輩エンジニアを救うことができます。
また、Interfaceの仕組みを早期に実感値を持って学ぶとよいです。Interfaceを駆使し、引数と返り値の型だけを定義しておくことで、処理の中身がデータベースアクセスなのか、Spread Sheetへのアクセスなのか、テキストファイルへのアクセスなのかによらず呼び出している箇所の変更を行わなくてもよくなります。
③ 変数のスコープは最小限になっているか？
プログラミングでのスコープ（scope, 可視範囲）とは、ある変数名や関数名といった名前を参照できる範囲のこと。
（引用：スコープ - Wikipedia）
メソッドが数十行になっている場合に、メソッドの後半でしか使わないような変数の初期化をメソッドの序盤にまとめてしまうと可読性が一気に落ちます。変数の初期化は、なるべく変数が利用される場所の近くにしておきましょう。
また、メソッドやクラスに適切なアクセス修飾子をつけているか見直しましょう。同じクラス内でしか呼ばない想定のメソッドにアクセス修飾子publicをつけていると、他のクラスでも呼ばれてしまって悲劇を生んでしまうかもしれません。メソッドはどこから参照されることを想定されているのか考えて適切なアクセス修飾子をつけましょう。
④ コメントにはWhyを書いているか？
これは最近よく指摘された内容です。僕はコメントには処理の内容を書くことが多かったのですが、そんなものはほとんど不要だと。コメントに書くべきなのは、「なぜその処理をすることしたのか？」だそうです。
数年後に自分もその時の仕様を知らないエンジニアが処理を読むときに、処理の内容はコードを読む力があれば理解できます。しかし、なんでそのような実装にしたのか、なんでここでこの処理を加えたのか？という実装の理由が書いていないと、その処理を残しておくべきなのか、もはや不要になったものなのかが判断できず困り果てます。
特に人の入れ替わりの激しいベンチャーでは仕様が文章として残っていないケースも多いので、「なんでこの実装にしているのか？」がコメントされているとめちゃくちゃありがたいです。Whyコメントほんと大事。
⑤ メソッド・クラスのコメントを書こう
Javaで言えばJavadocコメントを書く。これは所属しているプロジェクトの方針にもよるかと思いますが、基本的にクラスやメソッドにはその役割をコメントで残しておく方が良い。
メソッドやクラスを実装した自分でさえ、半年も経つと自分の実装したコードの処理内容を覚えてない状況に陥ります。よほどの容量のDBを脳に搭載していない限りは、自分の記憶力を信用しない方がいいです。
新規に作成したクラスやメソッドにはその入出力と注意点は最低限書く。既存のクラスやメソッドを変更したなら、コメントを編集する・もしくはコメントを追加することが賢明です。
⑥ 処理の内容は標準クラス・メソッドを使えないか？
僕もそうなのですが、初心者の頃はコードを自分で書くことが楽しくて、どんな処理も自分で書いてしまいがち。でも、よく使われる処理は標準クラスやメソッド（あらかじめ用意されているクラスやメソッド）にある場合が多いです。処理を書く前に、その処理内容は標準クラス・メソッドに用意されてないかな？と公式ライブラリに少し目を通してみるとよいです。
最初の頃は自分の知っている標準クラスが少ないので、「ああ。この処理はあのメソッド使えるな。」と思いつくことが少ないですが、標準クラスに目を通しておき徐々に引き出しが増えてくると、自分で書く必要がある処理が減ってきて楽になっていきます。もちろん、全部処理内容を覚える必要はなくて「こんなメソッドあるんだな〜」とさらっと目を通しておくだけでも脳に蓄積されていきます。
↓普段触っている人が多そうなJavaとPythonとRubyの公式ドキュメントとよく使う標準クラス・メソッドの紹介がされている本
昼休憩の暇なときや、実装していて標準クラスありそうだな〜と思ったときに少し目を通すだけでも続けていき、少しずつ自分の脳にインデックスを貼っておきましょう。
⑦ 使っていないゴミは残っていないか？
「こんなんやるやつおらんやろ！」と思いますよね。でも、セルフレビューをしないと、結構やってしまう人はいると思います。僕はたまに使われていないimport文が残ってしまって、指摘されると死にたくなります。
不要なimprot文や、使われていないメソッド、使われていない変数などはないか、レビュー依頼する前にチェックしておきましょう。これをするだけでも不要なレビューを防ぐことができるし、客観的にコードを見直すこともできます。
使わなくなった処理が残っていると、数年後にそのコードが本当に使われていないか調査する工数がかかってしまいますし、不要なコードを消す工数もかかってしまいます。
ボーイスカウトには大切なルールがあります。それは、「来た時よりも美しく」です。たとえ自分が来た時にキャンプ場が汚くなっていたとしても、そしてたとえ汚したのが自分でなかったとしても、綺麗にしてからその場を去る、というルールです。
（引用：ボーイスカウト・ルール | プログラマが知るべき97のこと）
やっぱり人間だからケアレスミスで見逃してしまうことはある。ケアレスミスを自分で見つけるか、他人に見つけてもらうかの違い。
⑧ 例外ハンドリングは適切か？
例外発生時に不整合が発生しないようにトランザクション境界を正しく設計してロールバックされるようにできているか、例外発生して処理が中断してもそのことを検知する仕組みを作れているか、など設計しておくことが必要です。
初心者の頃は、正常系の処理しか想定できず、例外発生時にどうやってデータ不整合を解決するかなどにまで考慮が及ばないと思います。しかし、いつまでもレビューに頼っているとどのような例外が発生しうるのかいつまでも引き出しが増えません。どのような処理を行うとどのような例外が発生しうるのか、過去のエラーログを追って学んだり、先輩エンジニアの例外処理を読んでみてわからなければ質問するなど、例外処理のユースケースを学んでいきましょう。
僕のように、例外発生時にどのユーザーの処理で例外が発生したのかログ出力する処理を書いてなかったから、どのユーザーのデータがおかしくなっているのか調査できない...。という事態に陥ってしまわないためにも。備えあれば憂いなし精神で備えると良きです。
⑨ 定義を知らずにコピペした箇所がないか？
ググるのをやめるとプログラムの生産性が上がるかもしれない - メソッド屋のブログ
これは僕が1年目にずっとやってしまった悪習です。既存のコードのメソッドの処理を使いまわして、継ぎ接ぎのコードを書いてしまう。「このクラスの定義知らないけど、テストOKになって想定通りの挙動をしてくれたしOK！一旦レビューに出そう！」とレビューに出して、レビューの嵐にさらされる。結局クラスを一からしっかり読み直す...。
「定義を確認する→利用方法をイメージする→実装する→テストする」
という順番で実装を進めるべきところを
「実例をイメージする→利用方法をイメージする→実装する→テストする（→定義を確認する）」
という順番で実装を進めてしまう。
この手順を続けた結果、クラスやメソッドの定義が自分の中に蓄積されていかないので、実装やデバッグの速度がいつまで経っても速くなりませんでした。
どんなに時間がかかっても定義を確認してから実装をすること。実例のブログを読むのは最終手段と考えること。ベテランエンジニアの先輩社員を見ていると、これを意識して実装しているから実装スピードは格段に速いし、他の言語を初めて書くときも定義が分かっているからすぐに実装できるようになるんだろうなと。
ちなみに、ベテランエンジニアはコードの話をするときに、「プログラム言語で会話できるか？」という表現をよく使っています。プログラミングネイティブじゃない初心者エンジニアは、プログラム言語で会話できるようになるために、基本的な文法をしっかりとおさえて学んでいくべきなんだろうなと。
まとめ
以上、初心者エンジニアが先輩エンジニアの時間を奪わないためにレビューもらう前に知っておくべきこと23個でした！
＊ちなみに、内容は初心者エンジニアの僕が社会人1年目に受けた1年分のレビューなので、ツッコミどころは多々あるかと思います。ツッコミもらえれば修正しますのでコメントなどに書いていただければ。
先輩エンジニアのレビュー工数が6時間かかっていたのが、3時間に削減するだけでも1年間に換算すると150時間 = 60万円以上の削減にはなるはずです。レビュー依頼前に自分のコードをセルフレビューすることには、それだけの価値があると思います。
一応、お約束の本とサイトも貼っておきますね。
xn--97-273ae6a4irb6e2hsoiozc2g4b8082p.com
では！
