はじめに
コードレビューはエンジニアの実務においておそらく欠かせないものであるかと思います。
自分自身も日々コードレビューを行っていますが、自分が正しくコードレビューできているのだろうかと不安になる機会が時々あります。
そんな中で上司がお勧めしていただいた本がこちら
「Looks Good To Me 〜みんなのコードレビュー」
全体所感としては新しいものを得るというのもありましたが、今までやっていたことで良かったんだなと自身の行なっていたレビューで間違っていなかった事を実感でき、良い復習の機会を得られた感覚がありました。
Chapter1 コードレビューの重要性
このチャプターではコードレビューとは何か、なぜ行うのかという内容メインで期待されていました。
コードレビューとは何ですか？とふと質問された時、皆さんはどう答えますか？
自分はパッと答えられそうにないです笑
本書では以下のように記載されていました。
ソフトウェア開発者が互いのコードを検査し、合意された一連の基準を満たしているかを確かめるプロセスです。
ということは改めて考えてみるとただ何となくコードレビューをするのでは駄目なんだと改めて気付かされますね。合意された一連の基準を満たしているか確かめるために一連の基準を相互で定める必要がありそうです。
Chapter2 コードレビュー徹底解剖
このチャプターではコードレビューのワークフローや実際のプルリクエスト(以降PR)を作成する際の基本的なプラクティクスに関して、またレビューする際の心構え的なところが解説されてました。
コードレビューワークフローは特に目新しいものがなく、まだレビュー文化がない組織向けに解説してる感がありました。
PR作成に関しては詳しく解説されてました。全体所感としてはレビュワー(レビューをする人)の認知負荷をいかに減らすかにフォーカスされている感じがしました。
その中において改めてPR作成で意識すると良さそうだと感じたのは以下
- タイトルは分かりやすく簡潔に
- レビューの状態
- レビュー担当者
- PRを管理しやすくする
タイトルは分かりやすく簡潔に
ダメな例一覧
- 請求書の問題のバグ修正
抽象的 - 課題#1462の修正
タイトルからは読み取れない - 請求書の誤りを修正（課題#1462）
課題を見に行かないと読み取れない - fix:小数点の位置が間違っているため請求書の計算が間違っており、少数が間違っている
タイトルが冗長すぎる
適切
- fix: 小数点の位置が間違っているため、請求書の小計が誤って計算される
個人的にはラベルで判断できるのでプレフィックスはなくても伝わるような気がしています
レビューの状態
皆さんは実際にレビューを担当した際に、このPRはレビューできる状態なの？レビューまだできないの？どっちなん？と思った経験はありませんか？
転職してから知ったのですがPRにはステータスを付与することができます。
個人的にはレビューできるかできないかが分かれば良いので本書にも出てくるドラフト(Draft)を使用しています。
ドラフト状態にするとこのPRはまだレビューできる状態でないのがレビュワーに伝わるのでレビュワーはPR作成者にレビュー可否に関して確認する必要がなくなります。
レビュー担当者
この項目ではレビュー担当者が意識することに関して書かれていましたが、特に以下が印象的でした
レビュー担当者としての役割には、2つの原則が特に重要です。それは「あなたには多くの影響が与えられていること」、そして、「あなたのレビューを通過するものはあなたの責任となること」です。これらの原則を無視することはレビュー担当者の役割を十分に果たしていないことを意味します。
かなり厳しい意見ですね。バグが起きたら実装者の責任だろ！なんでレビュワーも責任を負わないといけないんだよ！と思う方もいるかも知れませんがこれを意識しないと責任を取らないからいい加減にレビューしちゃおうみたいな感情になってしまいます。
しかしですよ、巷ではレビュワーとレビュイーの責任を取る比率は5:5なんて言われてたりしますが、ここまでレビュワーが責任を持つ必要はないんじゃないかと思ってるんですよね。
半分半分の責任となると、レビュワーも責任取るから動作確認は任せればいいや的な感じになり今度はレビュイーが責任怠慢になりがちな気がします。
なので個人的にはレビュワーとレビュイーの責任を取る比率は3:7、レビュワーも責任重大だけど最終的な責任はレビュワーが持つ、それくらいがお互い良い責任感の下相互レビューできる気がしております。
PRを管理しやすくする
PRを管理しやすいサイズに保つことが相互レビューにとって重要だと改めて感じました。管理しやすいサイズに保つにはPRを適切に分割することが重要だと思ってます。
扱いやすいPRとはどういったものかを本書では列挙されており、自身が基準として良さそうなものを以下にピックアップしました。
扱いやすいPRとは
- 10〜20分でレビューできる可能性が高い
- 通常、コードの変更は500行以下
- 通常、変更ファイル数は20未満
レビュー時間に関してはそうですねー1時間以上かかるようだったらしんどいなーって感覚になりますかね。
個人的な感覚では100行以下が軽微な修正、300行以下が普通、1000行超えてくるとレビューする気が失せます笑
なので500行は感覚的にも理にかなっている感じがしました！
ただ、どうしても分割したらコンフリクト起きやすいとか適切に分割できない場合は仕方がないです。それは細く説明などで補うようにはしてるつもりです。つもりなだけかもですが笑
ファイル変更数に関しては特に気にしてなかったです。コード差分を気にしていれば大体この目安の変更数で収まると思います。
Chapter3 チームの最初のコードレビュープロセスの構築
このチャプターではまだコードレビュー文化がないチームに対してどうコードレビュープロセスを構築していくかが記載されていました。
自身が所属しているチームは既にコードレビュー文化が構築されていたのですが、コードレビューで何を達成したいか、コードレビューの目標設定はできていないなと感じました。
本書では以下コードレビュー目標の選定をする際の項目が記載されていました。これらについて自身が実際の業務で感じたことを書き連ねたいと思います。
- バグの発見
- コードベースの安定性とメンテナンス性
- 知識移転・共有
- メンタリング
- 記録の保持・作成
バグの発見
バグを未然に防ぐため、コードレビューの大きな目的の一つではあるかなと思います。がしかーし！バグの発見をコードレビュー目標にするのはむずくね？と個人的には思いました。
それができたら全くバグの起こらないアプリケーションが作れますが果たしてそんなことは可能なのでしょうか？バグはどうしても起きてしまうものだと思います。
全てのバグを発見することを目標にするのは無謀な気がしていますが、例えばhotfixしなければならないような主要機能が使えなくなるようなバグは発見するくらいな目標はありな気がしました。
結論：これは目標としては設定しなくても良いかなと思いました。
コードベースの安定性とメンテナンス性
この目標にはコードベースの明確さとメンテナンス性を向上させる。テストとドキュメントのカバレッジを強化するなどが入ってきます。これは目標にしておきたいですよね。
ここで重要なのはチームで一度何が安定なの？メンテナンス性の高いコードってどんなコード？どんなコードが可読性の高いコードなの？等に関して擦り合わせるミーティングやコーディング規約などを定めることです。
この辺はそれぞれのエンジニアの主観が極めて入ってきやすく、俺はこう思う、私はこの方が読みやすいなどレビューでバトルことがしばしばあります笑
チームでそれらの基準を定めておくことで、相互間で揉めたときに、指標に沿って折り合いを付けることができます。
結論：チームで事前に擦り合わせ、コーディング規約等ドキュメント化した上で目標として設定するのはあり寄りのアリ！
知識移転・共有
この目標は以下のように記載されていました。
チームメンバー間での知識移転（ある人やグループから、別の人やグループに知識を移す意図的なプロセス）
意図的なと言うところが重要に感じました。それではどのようにして意図的に知識移転・共有をするプロセスを作成できるのでしょうか？
共感したのはまず第一にその分野において技術力の高い人からレビューを受けることです。ほんと技術力の高い人はなんでそんな考えになるの？良い意味で頭おかしい！と思うほど自身では思い付かないような実装をポンポンレビューで提案してくれます。
また、企業での実際の業務ではやはり効率が求められ、基本的にはバックエンドが得意だったら、バックエンド。フロントエンドが得意だったらフロントエンドのタスクを振られることが多いです。よって知識に偏りが生じます。
知識の幅を増やすには他の人のPRを除いてみるのは非常に良い試みだと思う一方で、やはり自身がレビューをしない限りは余裕がなく見る気になりません。
そこで一つの方法として情報共有レビュー担当者を設定すると言うのが記載されておりそれは良さそうだと感じました。
情報レビュー担当者は任意レビュー担当者とも呼ばれ、PRの承認は必須ではないものの、割り当てられたPRを見たことを示すように求められる。
これによってレビュー責任は伴わないけどPRは強制的に閲覧させることにより、知識移転・共有という目標が達成できると言うわけです。
個人的にPRのレビューを色々閲覧する程技術力向上にもってこいなものはないと思っているのでこのプロセスを導入するのはチームとしてすごく良さそうだと感じました！
結論：目標として設定するのが良さそう
メンタリング
前項とやることは結構似ていると思ってまして違いは相互に明確な上下関係というか技術力の差がある場合かなと思います。メンタリングを目標に含めるかはチームのリソースによって判断する必要がありそうです。育成にリソースを割けるなら是非注力してやるべきだと思います。
結論：育成リソースが割ける状態なら目標として設定しても良さそう
記録の保持・作成
この目標に関しては以下引用の通りPRを作成、レビューしている時点で実施していることになっているとのことです。
PRのベースノークフローを使用している場合はで既に実施していることになります。重要なのは個々がPRを完全に準備するためにどれだけの努力をするのか、そして努力にすることに対して、どれだけうまく互いに責任を持てるかです。
そもそもなぜ記録の保持・作成が必要なんだろう？実装者、レビューする人がその実装に対して覚えていれば記録なんてしておかなくてもいんじゃね？と思われるかもしれませんが実際必要だと感じる場面は多いです。
個人的に記録があってよかった！あるいはなんで記録が残っていないんだと思う時は時限爆弾のように時差でバグが発覚したり、だいぶ前に実装していた機能の仕様変更があった時等です。
機能変更を実装を進めている時に、なんでこんなコード、仕様だったんだろう？と思うことがあります。その際にgit logでcommit履歴を追ったり、そこの実装しているPRを覗きに行ったりして当時の実装を把握しにいくことがあります。
その際に、適当なcommit履歴(commitメッセージがfixだけとか)やほぼ説明のないPRだったりすると跡を辿れなくなってしまいます。実装した本人に聞けば良いと思うかもしれませんが1年前の実装は意外と覚えていないものですし、エンジニアは移り変わりが激しいのでもう実装者はその企業にいないかもしれません笑
よって後の開発者のためにも、未来の自分のためにも記録の保持・作成が必要だと改めて感じました。
ちなみに最近大事だと感じるのはレビュー間で口頭確認して解決した内容もPRにちゃんと残すことです。
口頭確認して満足し、そのまま記録に残らないことが多く、そういった内容に限ってその実装において重要な内容だったりします。
結論：既に実践しているため目標にするとしたら記録の仕方を明確に設定するのが良さそう。
このチャプターまとめ
チームとしてまずは一つこの中から重視するものを決め、単一目標を達成できるよう日々取り組んでいくのが良さそうです。
おわりに
いかがだったでしょうか？今回はPart1部分の感想を書き連ねました。コードレビューに正解はないとつくづく感じるのですが、コードレビューの指針はしっかりと存在し、それらを日々のコードレビュープロセスと比較しながらうまく取り入れて行くのが良いと感じました。
毎回技術書のPart1部分ばかりしか記事書けてないのでそれ以降も記載できるように頑張ります笑
でもこの技術書はPart1部分が大半を占めている気がするので続編書かないかも笑