はじめまして、2021/10/01にジョインしたQAチームのいさな(@isanasan_)です。
当面は主なミッションとして現システムのCakePHP4移行を担っていく予定です。
※移行プロジェクトの詳細はSREチーム(兼QAチーム)の金澤さんの過去記事をご覧下さい
モチベーション
筆者はランサーズにジョインしてはじめてコードレビューを経験しました。1ヶ月経ち、コードレビューで気を付けていることを、レビュア側、レビュイ側でそれぞれまとめたので、この機会に公開しようと思います。
また、本記事はあくまでも現在のスナップショットとなります、今後考え方の変化があれば追って記事にしていくつもりです。
コードレビューで気を付けていること
現時点で筆者がコードレビューで気を付けていることは下記の8点になります。
レビュアとして
- wikiに記載されている実装方針を熟読しておく
- まずは動作検証からはじめる
- “なぜ”そうしたのかが分らなければ質問する
- 似た処理を行っている既存実装と比較する
レビュイとして
- wikiに記載されている実装方針を熟読しておく
- 実装を初める前にPRを開き、背景・機能・実装方針を書く
- PRに検証方法をできるだけ分かりやすく書く
- rebaseを使ってコミットログを整形する
以下、それぞれの項目について詳しく見ていきましょう。
レビュアとして
wikiに記載されている実装方針を熟読しておく
lancersでは実装方針がwikiにまとめられています。大きめのスコープではcakeの機能の利用方針など、小さめのスコープではmailの送信やslack通知、ログ出力についてなどがあります。
あたり前ですが、これらを把握した上でコードレビューを行わないと適切なレビューが出来ないため予めしっかりと読んでおきます。
まずは動作検証からはじめる
これもあたり前ですが、レビュアはレビューしたコードの共同責任者となるので、そのPRが動くのかを自分の目で確かめる必要があります。また、別の側面として、動作検証をすることでPRの中身を理解することが出来ます。
はじめてコードレビューをアサインされた時に陥いったのが、いきなり、ただなんとなくGitHubのFiles changedで実装を眺めてしまい「なんかよくわかんないなぁ」と思いながら時間を溶かすという状況です。
まず最初に動作検証を行うことでそのPRで何を実現したいのかを理解することができます。場合によっては動作検証にあたって事前に必要なレコードを作る作業が発生するのですが、この事前準備を通して、より具体的に機能がどのような実装になっているのかを伺い知ることが出来ます。
“なぜ”そうしたのかが分からなければ質問する
動作検証を終えていれば、コードを読んだ時にそのコードが”何”なのかはほぼ理解できる状態になっています。しかし、何故そうしたのか、あるいはしなかったのかについてはコードを読んだだけでは分かりません。
PR上で質問や議論することで数年後に第三者が見たときに困らないように経緯を記録することが出来ます。必要であればコードにコメントを残してもらうことも検討します。
また更に、質問や議論の過程を踏むことで、より良い実装が導かれるということもありました。
似た処理を行っている既存実装と比較する
これもあたり前ですが、コードの書き方は何通りも選択肢があります。そして、どう書くかは究極的には好みの問題にいきつく場合がほとんどだと思います。
もう少し具体的に言うなら、Modelに書くのかControllerに書くのかといったことや、Componentに切り出すか否か、等があります。名著リーダブルコードにも書いてありますが、正解を求めるのではなく一貫性を保つことがこのような場面では重要となるでしょう。PRのFile changedだけを確認して終りにせず、既存コードと比較することが大切だと思います。
レビュイとして
続いてはレビュイとしてコードレビューを依頼する時に気をつけていることです。
wikiに記載されている実装方針を熟読しておく
とても大切なことだと思うので2回書きました。
当然の話ですが、レビュアだけでなくレビュイにとってもこれは重要なことでしょう。統一された実装方針に則ることはレビューコストを下げることに繋がるし、余計な出戻りも無くすことが出来きます。
実装を初める前にPRを開き、背景・機能・実装方針を書く
レビュアがスムーズにレビューできるように、ドキュメントを充実させておくことは非常に重要なのですが、きっちりとしたドキュメントを書くのはとても骨が折れる作業だし、頑張って実装を終えた後にPRを開いてちまちまとドキュメントを書いていくのはなんだか興醒めする体験だと思います。
コードを書き初める前にざっくりとで良いからドキュメントをまとめておき、実際にコードを書く時はドキュメントとのギャップを埋めるようなイメージで作業を進めることで、コーディング、ドキュメンテーション、レビューがスムーズに出来ると思いました。
また実装方針などは特にですが、コードを書きながら変わっていく部分もあるので、適宜PRの内容をアップデートしながら作業することで頭の整理にもなるしポモドーロテクニックとの相性も良かったです。
PRに検証方法をできるだけ分かりやすく書く
上述の通り、動作検証は品質の担保に限らずコードレビューをする上で重要な役割を担っていると思います。
検証方法を記載すること自体は当たり前なのですが、検証方法がわかりやすいか、検証がスムーズに行えるかはPRのレビューコストを大幅に左右するので、検証方法は簡潔かつ明快に、分かりやすく書くことを意識しています。
rebaseを使ってコミットログを整形する
個人的にはコミットログも重要な成果物の一つだと考えています。適切な粒度のコミットログに丁寧なコミットメッセージが残されていると、PRの本文からだけでは読みとれない情報を得ることが出来るので結果としてレビューコストの低下に繋がるからです。
まとめ
以上、ジョインして1ヶ月の筆者がコードレビューの際に気を付けていることをまとめました。
内容的には至極あたり前なことしか書いていないのですが、自分なりに言語化することで良い感じに整理することが出来ました。