はじめに
こんにちは、品質改善チームのrjgeです。
RubyKaigi 2019のレポートぶりに担当が回ってきたので、チーム活動の一環として行なっていたソースコードレビューのお話をしようと思います。
そもそもの発端
品質改善チームでは、ANDPADの開発で使用している開発言語やフレームワークのアップデート、パフォーマンスチューニング、自動テストの実装・環境整備など、新規開発を行う際に置いてけぼりにされやすい箇所のフォローを主に行なっています。
ANDPADは比較的新しいプロダクトなどの新規開発箇所についてはレポジトリが徐々に分かれてきているのですが、本体レポジトリはそれなりの規模の入り組んだソースコードになっています。
このレポジトリ、実は私の入社当時（2018年夏頃）は自動テストがほぼなく、そもそも実行しても動かない状態でした。自動テスト書いてるよ！と面接の際に会話したのですが、動かない自動テストはただの負債です。そんなこんなで、当時から今まで色々なメンバーの協力を仰ぎつつ、自動テストは当たり前に書くものだよねという文化をどうにか根付かせる草の根活動をしてきました。
その草の根活動の一環的として、先日のゴールデンウィーク明けまで本体レポジトリの全プルリクエストの総レビューを数ヶ月ほどやっていました。総レビューといっても基本的には各チームでのレビュー後に抜け漏れがないかの最終レビューをしていた形ですが、開発チーム内で一番プロダクトのソースコードを見ていた自信があります。
しかし、順調に開発メンバーが増えたことでプルリクエストの数も増え、しかし私は分身するわけにもいかず…。
一人でできることには限界がある！ということで、誰でもコードレビューができるようにと簡単なチェックリストを作成しました。
本題
社内向けに作ったソースコードレビューチェックリストの中で汎用的なもの、レビューする前に意識しておくといいのではないだろうかというポイントをいくつかピックアップして解説してみます。
どういったメンバーがレビュワーをするかによって多少変わると思いますが、基本的には開発メンバーは全員レビュワーができたほうがいいよねという考えのもとでの内容です。
そのため、当然では？という内容もあるかもしれませんが、その「当然」を明確にすることが大切という前提で書いています。
確認するプルリクエストに興味を持つ
個人的にはコードレビューって開発に比べるとあまり楽しくないです。どんどん実装したり仕様を考えたりしているときのほうがずっと楽しいのでやらなくていいならやりたくない作業です。
だからといってレビュー文化のない環境で開発したいかと言われるノー。みんなで少しずつ分担しながらいい感じに開発したいですよね。
コードレビューの一番最初の段階は、何をしているのか興味を持つことだと思っています。
個人差はあれど、興味のないものに集中するのは難しいものです。どういう機能なのか、何のために行うのか、どういうアプローチが取られているか。
そういった点を気にしながら見ると、自分の持っていない知識を知れたり、逆に相手にそれを提供できたりということに繋がりやすいように思います。
改善提案や指摘の際にはできる範囲で一次ソースかそれに類するものを提示する
忙しいときなど私もついやってしまうのですが、理由や具体例のない指摘はその後のコニュニケーションコストを相手に肩代わりさせることになります。
レビュワー・レビュイー共に習熟しており、お互いのことをよく知っているということであれば問題ないかもしれませんが、チーム人数が多くなればそういうわけにも行きません。
「なぜその指摘をしたのか」や「他にどういったアプローチがあるか」を記載することで不要にコミュニケーションコストを上げずに済みます。
時間は無限ではないので、自分で一次ソースを辿る時間がない場合もあるので、そこはできる範囲でということで！
ちなみに、自分で調べてみてほしいということであれば、ディスコミュニケーションが発生しやすい文字ベースでのやり取りよりも、他の方法のほうがよいかもしれません。
単純に他により良いメソッド等がある場合は、一次ソースを示すと早いです。
OSSを利用している場合はそのソースコードを示すだけで話が済みます。あるいは、手元のコンソールで再現した結果をコメントに添えるのも良いでしょう。
自分で調べたほうが身になるといった気遣いがあるかもしれませんが、そこをコードレビューで担保するかはチーム方針次第かと思います。
教えられたことであっても一次ソースを調べる人は調べますし、自分で調べた結果として一次ソースにたどり着けないまま怪しい記事を鵜呑みにする人もいるので、どういった方法が相手にあっているのかはコードレビューとは別に擦り合わせが必要になるものです。
忖度はしない、曲解もしない、一見してわからないものは見返す
忖度力に依存したソースコードは良くないです。
「●●さんだからここはこうやりたくてこういう実装をしたんだろうな」といった具合に考えてしまうコードはかなり忖度力が求められるコードです。
小さなプロダクト・小さなチームであれば、そういう連携が開発速度アップにつながることもありますが、そのまま巨大なプロダクト・チームになっていくと後から加わったメンバーからすると意味がわからなかったり理解するために時間がかかるコードになりやすいです。
フレームワークを利用しているのであれば、標準から大きく外れた方法をとったり、入り組んだ方法を採用するしかない場合には、ドキュメントやコメントの整備をきちんとしておくのが未来のためになると信じています。
ただ、自分（レビュワー側）の理解力が足りていない可能性も考慮する必要があります。
標準的な方法やメソッドを知らないことで理解ができない可能性もあるので、わからないと思ったら調べてみたり少し休憩してからもう一度見てみるというのも必要です。
常に100％の集中力を持続し続けられる人間のほうが少ないので、注意散漫になっていると思ったときは飲み物をとりにいってみたりトイレに行ってみたりして、少し頭をクールダウンさせるのも大切です。
コード規約化されていない箇所は戦争が起こらない程度に
改行やインデント、コミットサイズやコミットメッセージの内容、特定の実装方法やコメントの残し方など、開発者によって好みが分かれることがあります。
この辺りは、コード規約化をしていないなら執拗な指摘・議論をレビュー時にはしないほうがお互いのためと思っています。開発全体に関わる内容が特定のプルリクエスト上の井戸端会議で決まってしまうのは避けたいところです。
チームメンバー全員特にこだわりがないということであれば、気になるメンバーが荒れる前に規約化するのもありですし、逆にこだわりがあるメンバーが複数人いるのであればチームで一度話し合うのが良いでしょう。
Lint等も最近は色々な限度で整備されているので、人間が一つ一つ指摘しなくてもbotやエディタにいい感じに指摘させることが容易になっていますしね！
ちなみに弊社も最近rubocopとreviewdogによる自動化が行われたおかげでかなり楽になりました。
おわりに
記事を書いていて、ソースコードレビューというのは、チーム体制やプロダクトに影響を受ける部分も多々あると再認識させられました。
弊社もまだまだより良いやり方を模索していっているところではありますが、少しでもこの記事の内容が誰かの参考になれば幸いです。
私個人としてもソースコードレビューも一つの技術としてスキルを磨いていきたいなと思います。
