俺とレビュー

はじめに

mixiグループアドベントカレンダー7日目になりました。

近日「俺とレビュー」という内容で社内でLTする予定なので、その時に話そうと思っていたことをリメイクしてお届けします。というか、リメイクしすぎて原型がほぼなくなりました。
今年度に入って、業務内においてレビューする側される側の割合が半々くらいになってきた中で思ったことを中心に書いていきます。

TL;DR

• レビューは人の書いたコードをdisるのではなく、お互いの成長のきっかけになると良い
• レビューにかかる時間は短くなる方に収束するのが理想的

レビュアーとしての「俺とレビュー」

レビューをする側としてのポエムです。

レビューは一方通行ではない

レビューはコードを介したコミュニケーションだと自分は考えています。
例えば設計が悪いものについて、修正をお願いするコメントをしても、工数やスケジュールを考慮するとそれが難しいといったケースもあるかと思います。

そういった場合には代替案を提示するか、TODOとしてissueを切り、リリース後に対応していただくような迂回策を取るケースもあります。あくまでレビューというのはコードを書く側にとっての都合であり、俯瞰的に見ればより大切なのは顧客に価値を提供することのほうが優先度が高いためです。

一方的に修正をお願いするのはレビューではなく、ある程度のルールの範囲内でお互いの納得する合意点を相談しつつ見つけ、それを通じてブラッシュアップさせていくのがより良いレビューだと考えています。

成長のきっかけになるようなコメントをする

コメントを書く際に、「XXをYYにしてください」「hogehogeはhugahugaに書き換えたほうが良いです」などと修正する作業内容だけコメントしていくのはあまり良くないレビューだと自分は考えています。

そのコメントを見た時に、書いてある内容にしたがってただただ直すだけではレビューをされる側の成長のきっかけにつながりません。
直接修正してほしい内容をコメントするのではなく、時にはあえて少し遠回りした書き方をします。そうすることによってレビューをされる人に考えてもらい、その場限りではなく今後を見据えて書き方を改善してもらえるとより良いレビューになるはずです。

良い書き方はこっそり盗む

レビューをしていく中で、「これは良い書き方だ」と思える差分を見ることができると、レビューをする側にとっても成長のきっかけになります。

指摘することがないような綺麗な書き方をしている差分を見た時には、むしろ自分の書き方に反映して改善できる部分がないか、ということを考えながら自分はレビューをしています。

レビューイとしての「俺とレビュー」

レビューをされる側としてのポエムです。

受け身にならない

Aさん「hogehogeなんですけど、○○じゃないですか」
↓
自分「修正しました」
↓
Bさん「hugahugaですが、××では？」
↓
自分「修正しました」
↓
Cさん「piyopiyo、△△したほうがいいんじゃないでしょうか」
↓
自分「修正しました」
↓
自分「あれ、なんか全体見てみると変なことになってるな」

レビューでコメントを貰ってそれに対して脊髄反射で修正をしていると、全体を見た時におかしくなっているということは間々あります。

これは自分の中で確固たる実装方針が決まっていない時に起きます。個人的には、レビューとは一方的に受けるものではなくて、コミュニケーションを取りながらより良い実装にしていくものだと思っています。

Aさん「hogehogeなんですけど、○○じゃないですか」
↓
（コメントを書く前に、一度全体の実装方針を確認する）
↓
自分「おっしゃるとおりなのですが、XXなのでこうしたいです」
↓
Aさん「なるほど、それであれば今のままでOKです」

他の人からもらったコメントは、一度自分の中で噛み砕いて解釈して、その上でどう実装するか考え直すのが良いですね。

レビューを成長のきっかけにする

他の職業でもそうだと思いますが、エンジニアという職業は常に知識を身につけ続けないと食いっぱぐれてしまうと自分は思っています。

技術書や他の方のブログを参考にして得られる知識は一人でもできるものです。それに対し、レビューを通じて得られる知識は一人では身に付けることは出来ません。自分の書いたコードを他の人にレビューしてもらい、そこでもらったコメントを通じてコードの書き方をブラッシュアップさせていくことを意識するのは大切だと考えています。

レビューで自分が気づかなかった視点に気付かされるというのは貴重な経験です。それを元に、より気を配りながらコードを書くことが出来ます。

例えば、ざっくりと自分は以下を心がけながらコードを書いています。

• 書いているクエリが大丈夫か？
  • where句にindexが貼られているか
  • クエリが冗長でないか
• キャッシュは適切に使われているか？
  • 不整合がおきないようなset/expireのロジックになっているか
• メソッドは適切な粒度で分割されているか？
  • テストしやすい状態になっているか
  • スピード重視で、後でリファクタしたい場合はTODOをつけておく
  • わかりにくいビジネスロジックにはNOTEをつけておく
• 命名が長ったらしくなっていないか?
  • メソッド名・変数名共に長い時は設計が悪い時

これは、後から自分で差分を見直した時に経緯を追うコストを下げることと、差分をみてもらう時のコストを下げるために気をつけていることです。

同じ指摘は何度ももらわない

これは上の項目に通ずるものがあり、当たり前のことかもしれませんが、同じ指摘を二度、三度と貰わないように気をつけるというのは思ったより大事です。

同じ指摘を貰い続けている人は、指摘を受けて自分のコーディングスタイルを改善する意識が低い人と見受けられてしまいますし、そこで成長のきっかけを失ってしまうことにもなります。

他の方に見てもらう前にセルフレビューする

人に見てもらう前に自分の書いたコードの全体を眺めることで、レビュー時のコミュニケーションコストを下げることが出来ます。
些細なtypoやdebugコードが残っていないかなどのチェックは可能であれば他の人に見てもらう前に潰しておくのが理想であり、個人的には最低限のマナーだと思っています。（急ぎの時は確認が漏れてしまいがちですが）

レビューを受けて何度も修正するのはお互いにとってその差分に拘束される時間が長くなってしまい生産性を下げることにつながります。レビューされる側が気をつけてコードを書いていけば最終的にはレビューにかかる時間というのは短くなる方に収束するはずです。

おわりに

コードを書くだけなら一人でもできることですが、レビューというのは複数人いないとできないことです。コードの書き方はこれが正解という唯一解があるものだけではなく、同じ解釈のできる様々な書き方ができるケースも多々あります。

レビューを通じて、そういった多様な書き方から多くの人が読みやすいコードへ方向性を向けられると理想的ですね。技術的な内容の薄い長文駄文で失礼いたしました。

明日は takaya1992さんが頑張って何か書いてくれます。