コードレビューをしよう

f:id:naoto5959:20140819160636p:plain

Sendagaya.rb #86 - Sendagaya.rb | Doorkeeperに参加してコードレビューについて思うところをみんなでディスカッションしたので、自分の思いをまとめておきます。（ポエム）

ソニックガーデンの @mah_labさんのスライドにある7つの秘訣に同意です基本。あと、github上のプルリクでコードレビューするという前提でまとめます。

1. レビューの観点を明確に

いま何の話をすべきかという状況を判断して、どの観点で物を言うのかっていうのはコードレビューだけに限らず打ち合わせ時にも大事ですね。今この観点で議論を進めますよ！ということを表明することで齟齬が防げます。

観点が明確だと議論のテーマが明確になるので、コードレビューで修正すべき点も分かりやすくなりますね。

コーディング規約の観点
セキュリティの観点
メンテンスの観点
設計の観点
リリース前にこれで出来るレベルかどうか？の観点

とかとか。コーディング規約に関する観点でのレビューは、瑣末なことになりがちなので、犬にやらせるのがヨサソウです。HoundCIでリポジトリに番犬を飼おう - blog.takuyan.comが詳しいです。犬がいれば一人コードレビューも寂しくないですね！やった！

コードレビューする側は、観点を明確にして「何故この指摘をするのか！」を意識しましょう。コードレビューされる側は、観点を意識して改善点を受け入れましょう。

コードレビューされる側がプルリクを送る際に、確認して欲しい観点を明確にするのもいいですねー。

コードレビュー時に「個人的な思い」と「リリースブロック」になるかを分けるとヨサソウ #sendagayarb
— 🌈KOSHIKAWA (@ppworks) 2014年8月18日

「個人的な思いである感想レビューにたいしては『ふーん』て返す」www #sendagayarb
— 🌈KOSHIKAWA (@ppworks) 2014年8月18日

2. 我が身に返ることを恐れずに

ほんとこれ。自分を棚に上げるっているのはすごく大事で、コードレビューされる側も理解しておくべきことです。「そんなこと言っているけどお前のコードはどうなんだよ」と思わずに済みます。

そして指摘した方としては自分のことを棚に上げている感を出しながらレビューすると、少しは場が和むかもしれません。「自分が出来てないので恐縮なんですけど、こうした方がきっとメンテナンス性上がると思うんです。ん、、、思っているなら自分もやろう。ごめん直してくる！」

コードレビューしているかと思いきや、されていた！というのは良くあることです。

3. 悪いコードの説明を論理的に

ちゃんと納得させようぜってお話。自分の書いたコードが説明出来ないのはダメだよってのと同じで指摘も何故そうなのかを説明できないとだめ。

とはいえ、昔「#念のため2回実行」というコメントを見た時、「なんとなくダメ」な気がしました。

「リーダブルコードぐらい読んでおけよ」とかヨサソウ。

4. 良いコードについての共通の認識を

良いって何よというのを1にあげたような観点で日頃からディスカッションするといいのかなと思います。ドキュメントにすると廃れるし、生きたコードの中で日頃からディスカッションする方が新鮮だしいいんじゃないかなあ。

「コードレビュー、github上で炎上しだしたら、面と向かって話し合うと落ち着いてディスカッション出来て良い」 #sendagayarb #sg_study
— 🌈KOSHIKAWA (@ppworks) 2014年8月18日

こういうことだと思います。文字だけで炎上させてないで面と向かって話そうよ。ソニックガーデンではランチ時にそういうディスカッションを良くしているとのこと。ヨサソウ。ランチディスカッションでは、誰かのエディタでコードを開きながらみんなでコードレビューとかありそうなのでそういうときには、

「コードレビューしてもらっている最中にコメントで指摘内容を書いてメモる」 SGでレビューディスカッションしているときに、確かにこれやってていいなーと思った。 #sendagayrb
— 🌈KOSHIKAWA (@ppworks) 2014年8月18日

ってのがいいと思いました。今回の差分以外をついでにレビュー、となった際にも対応できますし。

5. 小さい単位でレビューを繰り返す

WIPな状態でプルリク作っておくのは良い。もはやコードもなくて良い。

空コミット便利！git commit --allow-emptyでgitを使った開発フローを改善 - fukajun - DeepValley -

ってな感じに git commit --allow-empty してWIPプルリクいくぞ！って張り切るのいいかも。

小さい単位でレビューするためには開発する単位が小さくないといけないはずで、それってそのシステムを開発している背景やその開発におけるビジネスモデルにも関連しそうだなーと思ったんですが、小さく出来ない事情があったとしても、細かくワケて開発することは出来るかもしれないなーと思いました。やってないので分かりません。

やるとしたら、

完成形のbranchを用意しておいて、そこに細かなbranch単位でプルリク出して行って進める
1つのプルリクに対してcommit単位で小さなレビューを繰り返す

後者はシンプルな感じしますが、commitが整理されていないとつらいですね。

とはいえ

「受託開発の中でコードレビューを適切に行うためには、やっぱりビジネスモデルの改善が必要となる。」 #sendagayarb
— 🌈KOSHIKAWA (@ppworks) 2014年8月18日

ということもありそう。

6. 指摘は素直な気持ちで受け止める

なんかもうコミュニケーションの話なんですよ、コードレビューってさ。あと向上心的なものもありそう。「はいはい、わかりましたよ、言われたから直しますよ！！！」という態度でどんな成長があるのかわからないし、なんか違う意味でダメそうな雰囲気ある。

指摘したら、無言でpushされて何もレスポンスない時って、コードレビューした方としては「なんか言い方まずかったかな。。。」とか思っちゃいそうだし、絵文字でも1つなんかコメントしておけばいいんじゃないですかね。もしくはお前ら2人でランチでも行け。

7. 人格否定しない

超大事。

「こういうコードを書いてきたお前の人生ってくそだよね」とか言うのはやめよう #sendagayarb
— 🌈KOSHIKAWA (@ppworks) 2014年8月18日

それな。「コードにはあなたの人生が反映されるんですよ、なのでコードをdisるということはあなたの人生をですね」とかちょっと涙が出ちゃう可能性があります。例えそう思っても、なんか違う言い方しましょう。そして受けた方もあくまでもコードの話をしているだけというのを意識しましょう。帰りにご飯でも行こう。

コードレビューのコツ「まずは褒める」その後「ダメ出す」 #sendagayarb
— 🌈KOSHIKAWA (@ppworks) 2014年8月18日

コードレビューしたいし、されたい

コードレビューしたい！ウズウズしているときには

一人プルリクエスト・一人コードレビューオススメです。 #sendagayarb
— 🌈KOSHIKAWA (@ppworks) 2014年8月18日

自分一人でコードレビューする際にも7箇条を思い返してやると、割りと最初は気づかなった事に気づいたりします。時間をほんの少しでも置くといいかもしれませんね。プルリクするときに「ちょっとイケてないかも。。。ごめん」てな箇所に言い訳コメントしておくと、後で見た自分が「甘えるな、いますぐリファクタだ」とか言い出して、改善されるかもしれません。されない可能性も十分にあります。

アーキテクチャレベルのレビューについて

railsでservice導入するかどうかって確かにコードレビューの議論でありえそう #sendagayarb
— 🌈KOSHIKAWA (@ppworks) 2014年8月18日

「新しいアーキテクチャを導入するときにはチームで十分に話し合う機会が必要だろう」 #sendagayrb
— 🌈KOSHIKAWA (@ppworks) 2014年8月18日

これは先のランチディスカッション的な感じで、別途ディスカッションして共通認識を持ったほうがいいと思います。私は割りと「それはrailsの思想に乗っているか」で判断するのがシンプルでいいなーと思います。後はケースバイケース。頭を柔らかくしつつ、必要以上のこだわりは捨てる。

テストについて

コードレビューにおいてテストは大事かなーと思います。テストがあるとこの仕様はどうのこうのという説明すら省けて良いですね。プルリクに対してテストが通っているかどうかに関しては、そのプルリクがどういう状況で送られたものかというコンテキストに寄ると思いますね。WIPであればまだ通らないテストで仕様を伝えることもあるでしょう。