id:antipopさんや
id:studio3104さんに機会をもらえて、CROSS 2021に参加させてもらい、はてなでのレビューの話を軽くさせてもらった。はてなからは僕と
id:hakobe932さんとで参加した。
- http://blog.kentarok.org/entry/2014/01/18/204552
- 2014/1/17 #cross2014 コードレビューCROSS 〜ぶつかり稽古 2014初場所〜 - Togetter
それで、今回参加して他の会社の人のレビューの話も聞いて、あーそれはあるあるとか、そういう問題解決するためにこういうことしてますとか、他の会社ではこういう時どうしているんだろとか、幾つかおもうところがあったので、もう少しレビューのことについて書いてみる。
レビューと関係性問題
レビュアーとレビュイーの関係に関して - 職質アンチパターン
コードレビューと関係性という話はいろんなところで出てくる。今回のイベントでも、確かにコードが悪くてもあんまり言い過ぎると良くないんですよねーとか、コード書けない人が萎縮してしまうようにしちゃうと駄目だよねとか、そういう話が少しずつ出てきて、みんな何かしら工夫をしなければいけないという感じになってるのだなと思った。
コードレビューは下手すると関係性を悪化させる。例えば以下の様なことが起こる。
- レビューの内容で喧嘩になり、関係が悪化する
- 目上の人から強めに言われることにより萎縮してしまう
- この人が言ってるんだから疑問に思わず直そうみたいに全部従ってしまう
こういうことは今の所属しているチームで昔は頻繁に起こってしまっていた。普通にpull request内で喧嘩になってしまうこともあったし、変に横槍を入れてしまってせいで数の圧力みたいなことも起こったこともある。
こういう反省があって、最近はチーム内や自分自身で幾つか気をつけている事がある。例えば
- 「こうしなさい」ではなく、「こうしたら良くなりそうだけどどう思う？」
- 文字情報は情報量が少ない
「こうしなさい」ではなく、「こうしたら良くなりそうだけどどう思う？」
喧嘩になったり、萎縮させてしまう理由として、レビュワーが「こうしなさい」と言ってしまうことが一つの原因なのではないかと思っている。なので最近は「こうしなさい」ではなく、「こうしたら良くなりそうだけどどう思う？」というような相談形式でレビューを行うことが多くなった。
具体的な話をする。
チームではGitHubのpull requestを使ったコードレビューをする。つまりdiffとline commentを使った形式。 この時各コメントに対して、昔は[MUST], [SHOULD], [MAY]というラベルを付けて、修正の優先度を表すみたいな形式でやることが多かった。しかし最近はなんらかの根拠*1が無い限り、[MUST], [SHOULD]のラベルを付けることが少なくなった*2。なぜかというと[MUST], [SHOULD]ってこうしなさいっていう命令形なので、受け手を苛つかせてしまったり、萎縮させてしまったりすることがあると思うからだ。
なので最近はそのようなラベルの代わりに[comment]とか[相談]とか[question]とかそういうラベルを使うことが多くなった。
・ [comment] この書き方はこういう書き方でもじつは書けるよ
・ [相談] このあたりは別のクラスに分割してあげたほうがいいと思うんだけどどう思います?
・ [question] このへんのクラス設計の意図ってどういう感じですか?
みたいな感じ。このような形式でレビューして、話し合いながら設計を収束させていっている。
実際はどうなったかというと、昔よりはGitHub上で突然喧嘩するとかは少なくなったかなと思う。もちろん気をつけているけど断定で言ってしまうことも結構あって、そういう時はいつもまたやってしまったとか思ってる。
こういう話、実はチームギークっていう本で学んだ。
Team Geek読んだ - はこべにっき ♨
文字情報は情報量が少ない
僕たちはインターネット上で活動してたりするので、GitHub上でline commentでやりとりすれば十分じゃんと思いがち。でも実際には文字情報は情報量が少ないということを最近は自分に言い聞かせている。それを意識しないでやりとりしてしまうと、誤解による喧嘩や無意味なやりとりが発生してしまうことが多い。
そこで最近は以下の様なことをやるようにしている。
- レビューした後に少し複雑な話は直接話し合って相談する
- レビューしたらその修正をペアプロで直してしまう
こうすると文字の情報の少なさを口頭などでもう少し補完できるし、お互い納得してコードの修正を出来ることが多くなったと感じる。
結局どうするの
コードレビューというのは結局プロジェクトやチームをうまく回すためのツールでしか無い。なのにそれにより関係を悪化させて全てを台無しにしては本末転倒だと思う。なので多少面倒でも高圧的にならないとか、フォローを行うとかはやっていったほうがいいのではないかと思う。
レビューの際に空気を導入しない
今回のぶつかり稽古で一番印象に残ったのは、
id:ryopekoさんが「空気を導入しない」と言っていたこと。これは本当にそうだと思う。空気読まないとレビューが出来ないとレビュワーに無駄な調査が発生する。
最近このようなことを避けるために、例えば以下の様なことをするようにしている。
- 必ずこのpull request概要を付ける
- 仕様ややったこと
- 動作画面の画像とか
- ちょっとこの辺自信ないんでなるべく気をつけてみてくださいとか
- レビュー依頼出す前に自分でレビューして、この辺どう思います？みたいなline commentをつけておく
- レビュワーはdiff見てレビューしたりするので、この辺自信ないんだなみたいなのを知らせられる
誰がレビューするか問題
最近一番困っているのが誰がレビューするのか問題。他の会社の人にもうちょっとこのへんの話を聞いておきたかった。
具体的に困っているのは
- スマフォ技術に詳しいエンジニアはチームに一人しかいないけど、誰がレビューするの?
- チーム外? チーム内でなんとかする? しない?
みたいな話。
個人的にはレビューを全くしないというのは結構リスク高いなと思っている*3ので、チームをまたぎながらどのようにうまくレビューを回していくのかについて気になっている。うちの会社ではこうしているよっていうのがあれば教えていただけるとありがたいです。
まとめ
今回こういう話す機会をもらえ、改めてレビューについて自分で考え直せたのは非常に良い機会だった。id:antipopさんやid:studio3104さん、ありがとうございます。
最近GitHubの力などもあって、いろんな会社でコードレビューというものが導入されつつあると思うけど、今回CROSSで話を聞いていると、やっぱりコードレビューのやり方についてはまだ試行錯誤な状態という感じだった。なので、うちの会社ではこうしてますとかそういう話があったら、是非共有されると嬉しいなと思いました。
