読者です 読者をやめる 読者になる 読者になる

Appresso Engineer Blog

アプレッソのエンジニアが書く技術ブログです。

ペアプログラミングしていてもレビューは必要か?

開発

こんにちは、開発部の野口です。

この記事では、私が所属していたスクラムチームでペアプログラミングが始まって、コードレビューがどう変わっていったかというストーリーをお話しします。

目次

ペアプログラミングが始まる

私が所属する DataSpider Servista の開発チームでは、昨年の夏から、スクラムフレームワークにのっとって開発を行っています。

人数の多い *1 チームでスクラムをやっていると、自然とペアプログラミングが始まります。
ただでさえ短いスプリント(一週間)のなかで、仕掛り作業を増やしても効果的でないからです。

たとえば、6 名のプログラマが「せーの」でそれぞれにタスクを取ると、6 つのストーリー *2 に並行着手することになります。
すぐに終わるものもあれば、時間がかかるものもあるでしょう。

さて、計画やふりかえりの時間を除くと 4 日間しかない作業期間において、最も時間がかかるストーリーに着手した人が、次の日風邪で休んだらどうなるでしょう?(答 : おしまい)

こういった問題を防ぐため *3、チームは自然とペアプログラミングをするようになりました。
私は 2017 年の 1 月末までチームにいたのですが、その頃には、計画の日を除いて、毎日 4 ~ 5 時間ほどをペアプログラミングに費やしていました。

GitHub:Enterprise 上でのレビュー

本題に入る前に、少しだけアプレッソでのレビューのスタイルについて説明しておきます。

アプレッソでは、svn を使用していた頃の「コミッター(レビュイー)の横にレビュアーがついてコードをウォークスルーし、レビューが完了したらコミットする」スタイルを経て、現在は、GitHub:Enterprise の Pull Request 上でレビューをするスタイルを取っています。

Pull Request の導入によって、以下のような利点が生まれました。

  • 非同期でレビューできる
  • レビューの経緯がコメントとして残る

一方で、以下のような課題も生まれました。

  • いつレビュー(マージ)してもらえるのか不明瞭になりやすい
  • 大きめの Pull Request について、ゼロベースでコードを見ると、仕様(意図)の理解から始めるのでレビュアーの負担が大きい

このような課題は、「適宜声をかける」「Design Doc をつける」「必要なときにはレビュイーを呼んで説明してもらう(一緒にコードを見る)」等の運用でカバーしていました。

ペアプログラミングしているとレビューが楽になる

さて、一方で、スクラムチームではペアプログラミングが始まっています。
厳密に計測はしていませんが、ここ 2 ~ 3 ヶ月は、チーム内で書かれたコードの 8 ~ 9 割程度はペアプログラミングによるコードではないかと思います。

とはいえ、ペアプログラミングはただ自然発生的に始まっただけだったので、従来通り Pull Request 上でのコードレビューは行っていました。

ペアの誰かがレビューする

コードレビューの原則は、「コードを書いた人以外がレビューする」です。

ところが、ペアプログラミングが本格化してきたある時期、チームの中から「ペアの中で Pull Request のレビューもまかなったほうが効率的では?」という意見が出ました。

当然、「しかし、それではレビューにならないのでは?」という意見も出ます。

そこで出た折衷案が、「1 つのストーリー(→ Pull Request)にレビュアーを 2 人つけ、ペアの中でも、書いたコードのドライバーではなかった人が必ず(相互に)レビューする」でした。 *4

ペアの中で Pull Request のレビューをまかなうと効率的

実際にやってみると、とても「効率的」です。

ペアで書いたのだから、よく知っているコードです。とてもスムーズにレビューを進めることができます。
個人的には、「(はじめは)よくわからない、コードの塊をレビューする」とき特有のストレスがなくなっただけでも嬉しいと思います。

しかし……、そもそも、ペアで書いたコードにレビューは必要なのでしょうか?
あるいは逆に、「ペアの中で Pull Request のレビューもまかなう」ことが、「本当にレビューになっている」のでしょうか?

ペアプログラミングしていてもレビューは必要か

本題の「ペアプログラミングしていてもレビューは必要か」にたどり着きました。

いま、2 つの問いが生まれています。

  • 「すべてペアで書いたコード」を、あらためてレビューすることが必要か?
  • 「ペアの中で相互にレビューする」ことは、Pull Request のレビューとして十分か?

「すべてペアで書いたコード」を、あらためてレビューすることが必要か

ペアプログラミングは「継続的なコードレビュー」とも言われます。
だとしたら、「すべてのコードをペアで」書けば、コードレビューは不要になるのでは?

実際のところ、8 ~ 9 割のコードをペアで書いてみての感覚としては、「やはり必要」です。

ペアは自信を持って速く進む

ペアプログラミングの効果の一つとして、「必要なことに集中(フォーカス)できる」ということがあげられます。

ナビゲータは、大きな問題を塊で考えずに、一つひとつのタスクに集中するようにドライバーに促します。
結果として、テンポよく実装を進めていくことができるのですが、ときには、「イマイチな実装」で「とにかく進んだ」あと、その部分がなんとなくそのままになってしまうことがあります。

もちろん、理想はナビゲータが注意深くコントロールすること、あるいは「レッド→グリーン→リファクタリング」のサイクルを厳密に守ることなのですが、私たちのチームは、「完全」にはなりきれていません。

ペアによって「今必要なこと」に集中し、自信を持ってぐんぐん実装を進められることは、ペアプログラミングの大きなメリットです。
一方で、それをコントロールしきれず、細かい問題の処理が漏れることがあります。

ここで、私たちのチームでは、ペアで書いたコードも改めてレビューすることで、ペアで漏れた部分をレビューでカバーする、というやり方が機能しています。

分断された知識をレビューで拾い集める

また、これは私たちのチーム特有の部分かもしれないのですが、一つのストーリー(→ Pull Request)に対して、複数のペアでローテーションしながら開発を行っています。
コードの知識を偏在させず、「誰でも、どのコードも触れる」状態を目指してのことです。

具体的には、だいたい 1 件の Pull Request に対して 2 ~ 3 名(2 ~ 3 アカウント)のコミットが含まれます。つまり、3 ~ 4 名が関わります。
多いときは、5 名にのぼることもあります。

f:id:enk_enk:20170224184400p:plain
▲比較的関係者の多い Pull Request の例 *5

ペアは原則として 1 人ずつ交代(1 人が抜け、1 人が残る)するようにしてはいますが、どうしても知識が分断されてしまう部分があります。
たとえば、ストーリーに対して最初にコードを書いた人が「あとでやろう」と思っていたちょっとしたリファクタリングが、入れ替わりの中で忘れられ、コードの重複が最後まで残ってしまったりします。

こういった問題も、マージ前のコードレビューによって回収することができます。

コードの重複は、見ればすぐにわかります。
「ペアで書いたから」と思っていきなりマージしてしまうと重複がそのままマージされてしまいますが、少しだけ時間をかけて Pull Request 全体を見渡すことで、このタイプの「ペアとペアの間に落ちた問題」も拾い集めることができます。

他には、「ボトムアップでテストを書いてはきたが、最終的にカバレッジが足りているかどうか」みたいなことも、最終的なレビューのタイミングで確かめることができます。
ペアプログラミングをしていても、コードレビューを設けると、このように Pull Request の全体像を見渡すいいタイミングになります。

「ペアの中で相互にレビューする」ことは、Pull Request のレビューとして十分か

このように、経験から、コードの大半をペアプログラミングで書いていても、コードレビューは有用だと感じています。

では、逆に、今採用している「ペアの中で Pull Request のレビューもまかなう」方法は、本当にコードレビューとして十分機能しているのでしょうか?

コードへの愛着と批判的なコードレビュー

ペアでコードを書いていると、ドライバーではなくナビゲータであっても、自然とコードに愛着がわきます。

よく「一度書いたコードを捨てるのは難しい」と言われますが、それはペアの片割れであっても同じことです。
コードレビューの際に、「私たちが書いたコード」という意識をまったく持たないことは不可能だと思います。

かつて、「誰かが書いたコード」を「別の誰か」がレビューしていた頃は、かなり、批判的にレビューを行っていました。

まずビルドしての動作確認を行いながら、コーディング規約もきっちりと頭に入れておき、綿密に一行一行を確認し、また動作確認を入念に行い……。
こうして、何十件もの指摘を積み上げることも少なくありませんでした。*6

このように批判的にレビューすることで得られていた綿密さは、失われてしまったのでしょうか?

数値化できていないので、主観的な感覚ですが、失われている部分もある、と思います。
別の言い方をすると、ペアプログラミングを行いながらも、あえてコードに一切関わらなかった人が批判的なレビューを加えることで、よりコードの品質が高まる可能性がある、と思います。

圧倒的なコードの共同所有

ところで、先ほど「私たちが書いたコード」というフレーズが出ました。

ペアをローテーションしながら、コードレビューも複数名が入り混じって行っていると、コードはまさに「私たちが書いたコード」としか言いようのないものになっていきます。

XP のプラクティスの一つに「コードの共同所有」がありますが、この状態は言うなれば「圧倒的なコードの共同所有」と言えます。

ある意味、「時間差のあるモブプログラミング *7」とも言えるかもしれません。
複数の頭脳があわさって、一つの(とても賢い)頭脳となってコードを書いている状態です。

このとき、「そのコードに直接関わらなかった人による批判的なレビュー」をあえて追加投入する必要があるか? と考えてみます。

真に「共同所有 = 一つの頭脳」の状態になっていれば、コードに関わる人が単純に 3~4 名から 4 ~ 5 名に(1 名)増えただけ、ということにもなりえます。もはや、別段、批判的でもありません。
この場合、「どちらでもいい」ということになりそうです。純粋に「今よりも目の数を増やした方がいいか(→ ちょっとだけ賢くした方がいいか)どうか」で考えるわけです。

一方、本当に「一つの頭脳」にはなりきっておらず、「直接関わった人」と「直接関わらなかった人」とで多少なりとも視点の違いが出るようなら、そこには摩擦が生まれます。
この場合、「その摩擦が効果的な摩擦か」が判断基準になりそうです。

それが主にコーディングスタイルの問題に分類されるような摩擦なら、おそらく非同期のコードレビューよりも、ペア作業の中ですり合わせて解決していくほうが、効率的でしょう。

一方、「視点が異なることによって、バグを見つけ出す」ような摩擦なら、「別の人がレビューする」価値はあるといえます。

「ペアプログラミングしている」という事実から「コードレビューが必要か」はわからない

こうして考えてみると、一つだけはっきりとわかることがあります。

それは、「完全な共同所有かどうか」も、「効果的な摩擦が生まれるかどうか」も、「ペアプログラミングしている」という事実だけでは判断できないということです。

どのようなスキルと性格を持ったメンバーが、どのような特性を持ったプロダクトに対して、どのようにペアプログラミングをしているか。
そういったさまざまな要素によって、チームの状態が変わってくるはずです。

これは「どのようなタイプのコードレビュー *8 が最も効果的か」という議論にも似ているのかもしれません。
大量のサンプルを集めれば、おそらく一般的な傾向は見出だせるのだろうと思います。そういったデータが手に入れば、自分の状況と照らし合わせて、大いに参考にできます。

しかし、「ペアプログラミングしていてもレビューは必要か」という問いに一般的で簡潔な結論を出すことは難しそうです。

まとめ : プラクティスの効果には常に意識的でありたい

スクラムチームでのコードレビューの変遷から「ペアプログラミングしていてもレビューは必要か」について考えてきましたが、明確な結論は出ませんでした。

あえて言うなら、正しく「状況による」というのが答になりそうです。

現在私たちのチームでは、チーム固有の状況から、「ペアの中で相互にレビューする」ことが機能しています。
しかし、これも将来変わっていくかもしれません。

よって、この記事の結論は、「プラクティスの効果には常に意識的でありたい」ということになります。
ペアプログラミングしていてもレビューは必要か? と問い続け、可能ならばデータも集めることで、必要な場合には行い、不要な場合には行わず、行う場合のやり方もより効果的にしていくことができるでしょう。

関連エントリ

まだスクラムを始めていなかった 1 年前には、こんなことを考えていました。

appresso.hatenablog.com

こういったスタイルの背景となっているスクラムの実践についてはこちらをご覧ください。

appresso.hatenablog.com appresso.hatenablog.com

*1:プログラマ 6 名、テスター 4 名ほど。最適な人数(7±2 名)をやや超過しています……。

*2:正確にはストーリーとは限らず、Product Backlog Item(PBI)です。ですが、この記事では、通りがよい「ストーリー」という用語を使います。

*3:正確には、もっと色々な理由があります。たとえば、Sprint Backlog にも優先順位があり、上から順に終わらせていくというルールにしています。その場合、たとえば 6 つ同時に着手すると、いくつものストーリーが半端なまま完了できない可能性も出てきますが、上から順に 1 つずつ集中して終わらせていくと、少なくとも半分以上は確実に終わらせられます。実際、はじめの頃は、完了した PBI がたったの 1 件、というスプリントもありましたが、数ヶ月すると、完了できないものがあったとしても、一番優先度の低い 1 件だけ、といった状態になっていました。

*4:ペアプログラミングのスタイルによっては、ドライバーとナビゲータはとても頻繁に交代するので、「ドライバーではなかった人」という表現は正確さを欠くかもしれません。ここで意図しているのは、「(直接)自分が書いたコードを自分だけがレビューしておしまいにする」ことがないようにする、ということです。2 人で交代しながらコードを書くとして、2 人がそれぞれに(やや冗長性を持って)コードをレビューすることで、「ある人が直接書いたコード」を「他の人がレビューする」ことのカバレッジは 100% になる、はずです

*5:participants のうち一人は QA エンジニア

*6:当時は Pull Request そのもののサイズが大きかったこともありますが

*7:https://www.youtube.com/watch?v=dVqUcNKVbYg

*8:ウォークスルー、机上チェック、公式なインスペクション等