こんにちは。DXプラットフォーム部のエンジニアの伊藤（亜紀）です。
データクレンジングツールMassteryの開発を担当しています。
この記事のテーマは「ソースコードレビュー」です。突然ですが皆さん、ソースコードレビューはどのように実施されていますか？
- 頭では重要とわかっているつもりだけれども、手を回せていない
- やってはいるけれど、時間をかけすぎている気がする
- やってはいるけれど、何をどこまでやれば十分か、悩ましい
そんな方も多いのではないでしょうか。
私が担当しているMassteryの開発チームでは、ちょうど1年ほど前にソースコードレビューのやり方を変更しましたが、ソースコードレビューが開発速度の向上に寄与している実感を持てています。
今日はそのソースコードレビュー、題して「開発速度志向のソースコードレビュー」について、ご紹介させていただきます。
最適なソースコードレビューの進め方は、アプリ・チームの規模・求められるサービスレベル・リリース頻度等々によって異なりますが、何かしらヒントになる情報をご提供できたら幸いです。
Masstery開発チームでソースコードレビューを変更した経緯
Masstery開発チームでは、ちょうど1年ほど前、開発メンバーが2人から3～4人にふえたタイミングから、ソースコードレビューのやり方を変更しました。
それまでは2人目メンバー（アプリ習熟度が浅い）が開発したら、マージリクエスト（プルリクエスト）を作成して、1人目メンバー（アプリ習熟度が深い）に見てもらう、というやり方でしたが、新しいメンバーがふえたことで
- 必要なレビューの総量がふえた
- 大きめの機能追加が並行するようになり
- メンバー同士、最新のアプリ状態にキャッチアップが追いつかないことがふえた
- コンフリクト（ソースコード・仕様とも）がふえた
という状況になったためです（※）。
とくに後者について、
- キャッチアップ不足に基づきリリースに不具合が混入し、緊急対応が必要になる
- コンフリクトの解消に時間を取られる
- 他メンバーが実装した箇所に改修を加えたくなったときに、ソースコードの理解に時間がかかる
といったことで、開発の進行に影響が出ていた点が懸念でした。
そこで、このタイミングで開発速度を向上させるためのソースコードレビューを考えました。
過去に所属していたチームのソースコードレビューのいいとこどりをしながら、Masstery開発チームにとって最適なやり方を検討しました。
※参考：リリースは週1回で、開発メンバーごとに開発範囲を決めることはしていません。
基本的な考え方
具体的なやり方ついてお話する前に、まずは基本的な考え方についてピックアップしてご説明します。
開発メンバー全員参加のMTGでレビューする
レビューは基本的に、開発メンバー全員参加のMTGで行います。Masstery開発チームでは毎週火曜と木曜の週2回、ソースコードレビューのMTGを設けています。週2回では足りない場合や多すぎる場合には、適宜調整しています。
正直なところ、定期開催・全員参加のMTGにするかどうかは悩んだポイントでした。個々人の開発時間を確保するために、できるだけMTGは減らしたいと考えていたためです。
ただ最終的に、全員がアプリの状況を把握するからこそのメリットの方が大きいと判断し、定期開催・全員参加にしました。
全員参加のメリットについては後ほどご説明します。
「全員がいち早く"概要理解"に達する」ことを目標に、実装者が口頭で説明をする
開発速度向上を目的としているので、レビューでまずめざすゴールは、いろいろ欲ばりたい気持ちを抑えて「全員が対応の概要を理解すること」としました。
ここで「対応の概要」というのは「なぜ・何を・どのように実装したか」です。
- なぜ：対応の目的
- 何を：どの機能を追加or修正したか。ユーザや外部連携システムから見た変更点は何になるか
- どのように：ソースコードの変更要点。その設計思想
受け売りを含んでもよいので、とにかく全員が上記のポイントを抑えた状態をめざします。
そして、その状態にいち早く達することができるよう、レビューは
- 実装者が、ソースコードの差分をレビュアーに見せながら、対応概要を口頭で説明
- レビュアーは、説明を聞いている途中で疑問があれば、その場で質問
という進め方をします。実装者自身が着目すべき差分をピックアップすること、そしてインタラクティブに進めることで、素早い理解につなげます。
「全員が対応の概要を理解」した後は、レビュアーから「このケースの動作確認はきちんとできている？」「こう書かないとメンテナンス性を損なうのでは？」といった、マージするために必要な質問・コメントを行います。
ただしあくまで「対応概要の説明を聞いたうえで気になった範囲」で行います。
このゴール設定は重要なポイントなのですが、その理由については後ほどご説明します。
極力その場でマージする。開発を止めない
「開発の効率をあげる」ことが一番の目的です。そのため、できる限りレビューの場でマージまでするように心がけます。
その場でマージすることが難しい場合も、「この点がクリアされれば、セルフマージしてOK」というように、マージ条件をレビュー中に明示するようにします。
もちろん、バグや、メンテナンス性を著しく損なうような変更をリリースしては問題なので、そのような場合は修正後のマージとします。
しかし開発速度が最優先であり、「たしかにそう書いた方がきれいだけど、今後の開発効率に大きく効いてこないのでは？」という場合はマージします。
特に不具合修正やユーザビリティ改善の場合、ユーザに早く快適に使ってもらえるようにすることが先決なので、その場はマージしたうえでリファクタリングは別のタスクに切り出すことも多いです。
具体的な運用
運用は以下のとおりです。
準備
- 実装者は、マージリクエスト（プルリクエスト）を作成する。
- 「機能の概要」「ソースコードの変更点」「動作確認内容」「特に見てもらいたい点」を簡単に記載します
- しっかり書くとそれだけで時間がかかってしまうので、「簡単に記載」がポイントです！
- スムーズに説明するためのカンペ程度に書いています。
- 実装者はレビューMTG開始前までに、今日のMTGで見てほしいマージリクエスト（プルリクエスト）と、その説明所要時間を申告
- 特別な仕組みはなく、slackに書いています。
- 大きな対応でも、説明所要時間は最大10分程度におさまるようにします。
- どうしても10分でおさまらない大作は、レビュー効率も悪くなりがちなので、マージリクエスト（プルリクエスト）を作成する段階で、レビュー単位が大きくなりすぎないよう気をつけます（大枠ができた段階で一度見てもらうなど）。
レビューMTG
30分のMTGで、チーム全員でレビューします。
- 申告されたマージリクエストのうち、対応期限などを考慮してその日見るものを相談・決定
- 実装者は、ソースコードの差分を見せながら、対応概要を説明
- 「なぜ・何を・どのように実装したか」「動作確認の内容」「特に見てもらいたい点」
- レビュアーは、疑問があれば適宜質問
- 説明後、レビュアーから気になった点があればコメントする
- 問題なければその場でマージ
- 問題がある場合も、どうなったらマージ可能か、できるだけその場で明確にする
メリット
この進め方は開発速度を重視したものですが、開発速度以外にも以下のようなメリットがあります。
悩む時間がほとんどない
実装者が対応について口頭で説明し、インタラクティブに質問しながら進めるので、レビュアーが悩む時間が少なくて済みます。
マージリクエスト（プルリクエスト）に対応内容を丁寧に記載する方法は、記録がしっかり残る点はよいのですが、文章で説明している対応内容とソースコード変更点の対応づけに、地味に時間がかかるものです。
とくに、アプリ習熟度が浅いメンバーほど、ちょっとしたことで理解に躓きがちで、しかも1つ躓くとその先が理解できないことも多いです。
それを都度マージリクエスト（プルリクエスト）コメントで訊いていると、待ち時間を含めて理解までにかなり時間がかかってしまいましたが、インタラクティブに進めることでその問題は解消します。
さらに、MTGは時間の制約があるので、つい深入りして時間をたくさん使ってしまった、ということも防ぎやすいです。
アプリ固有知識の平準化が図れる
全員でレビューすることで、アプリの固有知識や設計思想を共有することができます。
そういった知識は、ドキュメントや概念図にまとめられているのが理想ではありますが、実際の開発現場では
- 変わらないと思っていた要件・前提・設計がどんどん変わり、ドキュメントの更新が追いつかない
- どうせすぐ書き換えることになるので、ドキュメントには大枠だけ書いておき、あとはソースコードを正とする
- 大きな変更が並行して進行する
などといったことは日常茶飯事で、ドキュメントに落とし込みきれていない暗黙知というのはどうしても生じてしまうものです。
ですが、対応概要だけでもレビューしていれば「こういう設計になっているのでこう対応した」という理解を積み上げていくなかで、暗黙知をレビューの場で補完していくことができます。
さらに全員いると、アプリ固有知識への習熟度が高い人と低い人が必ず同席するので、「詳しくない人が調べる」のではなく「詳しい人が説明する」という方法で疑問を解決できるので、アプリ固有知識の平準化はさらに促進されます。
また、
- 全員が理解していれば、リリース後しばらくして不具合が発覚したときに、仮に実装者やその箇所に詳しい人が不在だったとしても、原因のアテがつけやすい
- 全員が最新の開発を把握していると、コンフリクト（ソース・仕様とも）に気が付きやすい
というのもメリットです。
レビューが滞留しない
1～2人にレビューを割り当てる方式だと、レビュアーが忙しかったり割り当てられたことを見落としたりすると、そこでレビューが滞ってしまいます。
そこは「優先度をあげて、きちんとやりましょう」という話ではあるのですが、他ならぬ私も、レビュータスクが苦手な人間です（笑）。
たくさんタスクを持っているときに、差し込みのタスクが1つ飛んでくると、気づかぬうちにちょっとした心理的負担になってしまうのですよね。
多くのレビューは数分では完了しないので、完了するまでの間「やらねばやらねば」とレビュータスクがTODOリストと頭の中に存在し続けますし、緊急のものでなくとも相手を待たせると小さな罪悪感を感じてしまいます。
レビューを定期MTGにして、そこで全員の知見を結集して素早く疑問や懸念を解決し、マージしていくようにすれば、レビューが溜まりません。
もちろんMTGなので、毎度待ち時間のオフセットはつきますし固定の拘束時間にはなるのですが、それでもトータルで見ると快適になったと感じています。
レビュータスクが特定の人に集中しない
レビューを全員でやると決めてしまうことで、特定の人（そのアプリに最も習熟している人）ばかりレビューすることを防げます。
時折「ここの実装は問題ないか、後でxxさんに詳しく見てもらった方がいいね」となることはありますが、本当にその人が入念に見るのが最適なものだけに限定することができます。
知識の平準化を図ることで、「対応内容の妥当性をチェックできる人」がふえていくので、そういった「入念な確認」も徐々に分散させていくことができます。
実はこれだけで品質担保にもかなり役立つ
開発速度向上を志向したソースコードレビューについてお話してきました。
レビューの第1のゴールはあくまで「全員が対応の概要を理解する」ことで、対応に問題がないかのチェックはあくまで「説明を聞いて気づいた範囲」で行うことを意外に感じられた方もいらっしゃるかもしれません。
このやり方ではバグを発見しきれず品質は担保が不十分になるのではとも感じられたかもしれません。
私は、これでも品質の担保にもかなり貢献していると感じています。それは、以下の理由からです。
（なお大前提として、バグはレビューだけで100%検知しようとするものではなく、テストやソースコードの質などで総力的に品質担保すべきものと考えています。）
口頭で説明すると問題点に気付きやすい
これは個人的な経験則ですが、文字で説明するよりも口頭で話した方が問題点に気づきやすいと思います。
実装者は自身の対応内容を口頭でレビュアーに説明しますが、話している最中に実装者自身が問題点に気づくことがよくあります。
また口頭で説明すると、理解度・自信度合い・迷いなどが口調に如実に表れるので、問題点を検出するうえで重要な手がかりになります。
他メンバーの「視点」が入るだけでも問題発見効果がある
これも経験則になってしまいますが、本当に致命的な不具合というのは
- 要件や仕様の理解不足
- そんなことを考慮する必要があるとは思ってもみなかった
- そんなところに影響するとは思ってもみなかった
というケースが多いです。ローカルなところで起こるというよりもグローバルなところで起きます。
こういったケースは、ソースコードやテストケースとにらめっこせずとも、「わかっている人」が見ればすぐに気付けます。
その点、異なる視点から「この観点には気をつけた？」というキャッチボールをたくさんできる進め方は効率的です。
もちろん、話した結果としてソースコードやテストケースを熟読する必要があると判断した場合は、熟読します。
さいごに
いかがでしたでしょうか。
冒頭でもお伝えしたとおり、最適なソースコードレビューの進め方はアプリ・チームの規模・求められるサービスレベル・リリース頻度等々によって変わってきます。
とくに開発メンバーの人数がふえたらどうするかは悩ましい点ですが、今後も状況変化に合わせて試行錯誤と改良を加えていきたいと思います！
この記事を書いた人
伊藤 亜紀
DXプラットフォーム部エンジニア。2015年新卒入社。
データクレンジングツールMassteryの開発を担当。
2021年10月よりMassteryのnote始めました。
よろしければ是非こちらもご覧ください！
Discussion