コードレビューが苦手な人が覚えとくといいこと
たまに「コードレビューって何を見たらいいのかよく分からない」という話を聞きます。
たしかに、合否の観点なんて誰かが教えてくれるわけでなし、何をどう判断すべきかなんてよく分かりません。
そこで今回は、誰でも理解しやすいコードレビューの観点について、私見など挙げてみたいと思います。
――いつもお読みいただきありがとうございます。
または初めての方も、この記事を見つけてくださって嬉しいです。
私は、都内某所にてエンジニアなどして生きております、中島と申します。
月月火水木日日くらいのザックリした精神でがんばっております。
この 絶対バグらないシステム作ろうぜの会 では、バグの出ないシステム・問題を起こさないチーム運営・ＡＩには作れない設計論などのコラムを、なるだけ面白く・分かりやすくお伝えする主旨で記事を配信しております。
1. コードレビューはテストの一部である
もし、「テストとは動作確認のことである」という考え方の人がいるなら、そういう人はレビューというのが何なのか理解しにくいかもしれません。
一般にＩＴ業界でテストといえば、プログラムを実際に動かして行う行為の総称だからです。
ですが実際には、テストは『将来のトラブルを少なくするために行う行為の総称』と捉えた方が座りがよさそうです。
たとえば広告業界では、《テスト》という言葉がＩＴ業界よりもはるかに広い意味で用いられています。
顧客に見せて印象を調査する
できあがった原稿をスポンサーに見せる
同僚に見せて印象を質問してみる
これらの行為を全て《テスト》と呼ぶんです。
もちろん会社によりますが、紙ぺらを隣の同僚に見せるのもテストなわけ。
コードレビューもそれと同じ。
『将来的なトラブルを少なくするために、今できる改善案を捻出する』という意味合いの行為を全てテストと捉えれば、コードレビューで何をやればいいかはとてもシンプルになります。
レビューをテストの一部と捉えれば、『動作確認すれば分かることは、レビューで見る必要はない』という理屈が成立するからです。
つまり、動かすだけでは分からない確認を、コードレビューでやるのです。
2. 動かすだけでは分からないトラブルの例
でもよく考えたら、実際に動かしてみても分からないというのなら、そんなのトラブルといえるのでしょうか。
プログラムなんて正しく動けばそれでいいのですから、だとするとコードレビューなんてやる意味はないことになってしまわないでしょうか。
将来なんて関係ないと思うならね。
言うまでもなく、プログラムコードは今日明日だけ動けばいいというものではありません。
１年後も２年後も、変わらずに同じ動作をし続ける必要があります。
動作が未来永劫ずっと変わらないことを保証するためには、
多少の状況変化では挙動が変わらないこと
将来の基盤変更の影響を受けにくいこと
といったことも確認しなければいけません。
そのためには『ルールに則ったコード』であるかどうかを確認する必要があり、それは試しに動かしてみるだけでは分からないのです。
3. ルールとはトラブルを防ぐためにあるもの
プログラミングにはルールにいくつかの種類があり、
一般的なコードフォーマット規約
ネーミングルール
アーキテクチャールール
ビジネスルール
などがあります。
それらのうちコードフォーマットについては、昨今ではフォーマッターがかなり発達しているため、人間が目検でチェックする必要性は下がってきています。
ですがそれ以外はどうでしょう。
『その場に適した変数名をその場で考える』
『その場で作られたアーキテクチャールールをその場で順守する』
『一般的なビジネスメソッドに沿ったロジックを考える』
これくらいならＡＩにもできるでしょうが、１０年２０年のビジネス継続性まで考えた長期的な視点での設計は無理でしょう。
なぜならそれらは、人間都合の “我々はどうしたいのか” の話だからです。
そしてこの “どうしたいか” の部分を長期的に維持していくためにルールは作るのであって、ですからコードレビューも、ルール通りであるかを確認する必要があるのです。
4. コードレビューの観点例
では、ビジネスを長期的に維持していくのに必要なルールとは、具体的にどういったものでしょうか。
いうまでもなく、その根本に来るルールは、
『正しいビジネススタンスに基づいているか』
です。
観点例1. プログラムがビジネスフォーマット通りかどうか
ビジネスとは、一定の行為を繰り返すことによって売り上げをあげる仕組みのことをいいます。
一般に企業では従業員の行動の大部分がフォーマット化されているもので、『やってることが毎日違っていて、その共通化も不可能』なんてことはありえません。
仮にありえたとしても、そのような企業はコンピューターを導入なんてできないので、プログラマーである我々がそんな顧客の存在を加味する必要はないでしょう。
1. 広報を行う
2. 顧客を探す
3. 注文を取る
4. 契約を結ぶ
5. 納品する
6. 代金を受領する
一般企業は最低でもこれくらいのフォーマット化は行われているもので、基本的には例外はありえません。
であるならば、全てのプログラムには『業務ルーチンのどの部分を担当するのか』『その業務を正しく運用するために、どんな挙動が必要なのか』という２つの観点が最低でも存在します。
また、全ての企業の全ての業務には『顧客を喜ばす』という至上命題があるわけで、もしかするとプログラムはその部分を担当するかもしれません。
その場合のプログラムのレビュー観点は、
この手順で顧客は本当に喜ぶのか？
顧客を喜ばすための手順を、本当に正確に実現できているか？
といったものになるはずです。
もしくは、そういう目的で作られたことが分かりやすいコードになっているか？（なんのために作られたのかが、ドキュメントを読まないと分からない状態になっていないか？）といった観点なども、動作確認だけでは分かりえない部分だし、ＡＩに任せておける部分でもありません。
観点例2. 手順が“越境”を起こしていないか
たとえば、『ユーザー情報から書類を自動生成する』というプログラムがあったとします。
そしてそのプログラムは、以下のような順序で動作していたとします。
1. ユーザー情報を取得
2. エラーチェック
3. 書類のフォーマットを作成
4. ユーザー情報に従ってフォーマットに値を入力
5. 画面にプロンプトを表示し、足りない情報を入力してもらう
6. 書類の作成完了をユーザーに通知
7. 5. で入力してもらった情報をフォーマットに追加
8. 保存処理
この手順書、なんかおかしくないでしょうかね？
おかしいとかそれ以前に、変に煩雑で分かりづらい感じもします。
なぜなら、同じ役割の処理が同じ個所にまとまっていないためです。
プログラムを設計する際に、いきなり細部から考え始めてしまう人が、よくこのような順序のバラバラなロジックを組んでしまいます。
そうでなく、書類の自動生成ロジックは、概略として以下のような手順がまず存在するはずです。
1. ユーザー情報を集める
2. それらを真っ新な書類に書き込む
3. 完了報告
なぜなら今回の書類自動生成処理は、“ユーザー情報を書類に書き込む” という目的だからです。
全ての処理は必ずこの 1. ～ 3. のいずれかに属していなければおかしいし、全ての処理はこれらのどこかに属すために作られたものでなければいけません。
1. ユーザー情報の収集
1.1. データベースからユーザー情報を取得
1.2. データベースにない情報を画面から入力してもらう
2. 書類の作成
2.1. 書類のフォーマットを準備する
2.2. 書類に情報を書き込む
2.3. 書類を保存する
3. 完了報告
3.1. 作成完了をユーザーに通知
プログラムは「動きゃいいじゃん」という認識で適当な順序で作っていると、改修されるたびに内容がどんどん複雑怪奇になっていってしまいます。
そうならないためには『同じ種類の処理が１ヶ所にまとまっているか』という観点が必要だし、たまたま流用できそうだったからという理由で全く違う時期に作られた処理を呼び出していてもいけません。
このような、
必要な処理がちゃんと一塊になっておらず、処理があちこちに飛んでいる
全然関係ないタイミングで作られた処理を、たまたま使えたからといって流量している
といった状態のことを“越境”といいます。
『プログラムは正しい順序で組まれているべき』というコンピューターの絶対則を無視していて、変なところに変なふうに手を伸ばしていることです。
越境の多いプログラムは、そのときはよくても、時間がたてばたつほど品質が劣化しやすい状態になってしまいます。
5. コードレビューは１観点につき１回見る
とりわけコードが大きい場合、「１回で全てをチェックしたい」という気持ちになったりします。
数百・数千ステップあるような大きなコードに対し、数十個以上のチェック項目がある場合などは、同じコードを何度も読むのはとても面倒です。
個人的にもそう思います。
ですが残念ながら、複数の観点でのチェックを同時にやるとレビューのクオリティは間違いなく低下しますし、第一結果的に余計に時間を食ってしまいます。
たとえば５００ステップのプログラムコードを、
インデントが正しいか
private とすべきメソッドを誤って public にしていないか
メソッドの順序が用途順に並んでているか
メソッドの記述順が詳細設計書と同じになっているか
変数名が、その役割と一致しているか
この５つの観点でチェックするとします。
この場合、これら５つの観点でのチェックを同時に１度に行うよりも、１観点ずつ５回読みなおしを行った方が結果的に早く終わります。
直感的には５００ステップに対して５観点でのチェックを同時にやった方が早いように感じるかもしれませんが、それは『頭がこんがらがって読み直しが発生している時間』を無視して考えてるからそう感じるだけです。
レビューは、１観点につき１回、レビュー観点数の数だけ読んだ方が絶対に早いです。
そしてそのためには、自分が『なんのためにレビューしてるのか』といったことを理解・明文化してレビューすることが大事なのであって、そうすることでプログラムのクオリティはあがっていくものなんじゃないかなと、私なんかは思うわけです。
ではまた。
この記事が気に入ったらサポートをしてみませんか？