背景
先日、社内勉強会でプルリクエスト（以降 PR と呼ぶ）に関する勉強会を開いたのですが、思ったより反応が良かったので記事にしてみました。
※ PR を依頼する人のことをレビューイ、PRを確認する人のことをレビューアーと言います。
レビューイが持つ悩み
レビューをお願いする際、以下のような悩みはありませんでしょうか？
- レビューがなかなか返ってこない
- レビューでコメントがつきまくって対応に追われる
- 気づいたら巨大なPRとなっている
- mergeまでに時間がかかってしまう
僕は全部経験してきました...
「急いでいるのにどうして早くレビューしてくれないんだ...!」
「いつもたくさんのコメントがついて、なかなかタスクがDoneにならない...」
「実装よりもレビューの期間が長いなぁ〜」
と、正直にいうと他責思考も混じっていました。
今思えば、恥ずかしいことだと思います。
しかし、
レビューを出す前の配慮・工夫で上記の悩みは大きく改善されました。
前述した４つの観点から、述べていこうと思います。
レビューがなかなか返ってこない
自分の感覚ですが、原因は大きく二つ
- 変更ファイルが多すぎる
- コードが難しすぎる
変更ファイルが多すぎる
githubのURLを開いた5秒後には❌でタブを消したくなりません？
レビューアーからすると「変更多すぎてすぐに取り掛かる気起きねー」って感じですよね。
その結果「後で見るか...」と後回しになっちゃいます。
難しすぎるコード
これはパッと見で「コードの意図が理解できない」時です。
実際のコードは載せられないので、例えです。
const reverse = (obj) => {
const result = {};
Object.keys(obj).forEach(k => {
result[k.split('').reverse().join('')] = obj[k];
});
return result;
};
上記のコードはじっくり読めばやっていることは理解できますが、時間がかかります。
複雑なコードだと、解読するのに負担・時間を必要とします。
大前提、難しい（読みにくい）コードは書いてはいけない
レビューイも悪意を持って
「読みにくいコードを書いてやろう」と思ってはいません。
PRの内容がベストと思っているはずです。
その結果「複雑」だと思われてしまうのは仕方ないと思います。
技術の問題なので、小手先のテクニックではなく、
レビューの内容を自身で内省したり、
設計の勉強をするなど少しずつ改善するしかありません。
ただ、レビューイは「シンプルなコードを書く」
ということを日頃から意識した方がいいと思います。
レビューでコメントがつきまくって対応に追われる
コメントがつく原因、それは 書いているコードが難しい からだと思います。
- ぱっと見で何がしたいのか分からない
- 変更の意図が伝わらない
そんな時、親切なレビューアーは丁寧に質問してくれます。
「背景なんだっけ？」、「どうして〇〇にしたの？」とか
ただ、「その質問に答える→新しい質問がある」
というように、質問のラリーができてしまうとなかなか前進しません。
質問のラリーはレビューアーにも負担を与えるし、
時間がかかるのでやりたくありません。
世界一流エンジニアの思考法
かなり有名な書籍なので読んだことがある人も多いはず...
それに以下の話がありました。
てっきり、まわりの辣腕エンジニアは、私の書いたコードを読んでわからないはずがないと思っていた。だが、レビューする人はコンテキストがそもそもない。
…略
プルリクのレビュー数を減らすためには「読んだ人がどう感じるのか？」を意識しながらコードを書く必要がある。コードもまた「読み物」だ。
この節を読んだ時は、胸に深々と突き刺さりました。
「わかりにくいコードを書いてしまう自分に原因があるんじゃないか...!!!」
そこからは 「シンプルなコードってなんだろう」
と考えることがとても増えたと思います。
どうして分かりくいPRを書いてしまうのか
レビューイとレビューアーでは、持っている情報量に大きな差がある からです。
レビューイは、以下の内容が頭に入っています。
- 背景の調査（変更のきっかけとなった業務知識）
- パターンの洗い出し（どんな解決方法があって、今回の変更にした理由）
- 実装時の落とし穴（どんな実装がバグに繋がりうるか）
こういった諸々を理解している人が見る景色と、
理解が浅い人（レビューアー）の見るコードの景色は全然違います。
レビューアーがちゃんと理解するには、
先にレビューイの情報量に追いつく必要があり、かなり負荷がかかります。
どうしたら、レビューアーの負荷を軽減できるのでしょうか？
PRの説明を充実させる
PRのリンクを叩いた時に、最初に見えるのがdescriptionとタイトルです。
ここを見るだけで、最低限、以下の内容が理解できるようにしたいです。
- PRを開けることになった背景・目的
- 実装内容
- 見てほしい箇所・みる必要のない変更（モックデータの準備など）
また、UIに変更を加えた場合はキャプションも掲載したいです。
必要ならば、動画も載せて、 具体的な変更をわかりやすくします。
GitHubでテンプレートを設定できるのですが、自分は作っていません。
大体メンテされず形骸化しがちというのと、
「何を書けばレビューアーに分かってもらえるのか」 を毎回考えたいからです。
例）description のサブタイトル
「どうすればレビューアーが助かるのか」を考えて書けるといいと思います。
先回りでコメントを残す
変更を見ていて、引っかかりそうなところに先回りでコメントをつけます。
レビューアーが疑問に思いそうな部分に先回りで情報をおくことで
不要な質問ラリーを回避します。
自分は、フロントエンドエンジニアということもあり、主にUIの変更などで、
具体的な変更イメージを持ちやすいように、キャプションを掲載することが多いです。
あとは、今回の変更の動機など、コードにコメントを残したくない時です。
例）変更の動機
ただ、後から見ても不思議に思ってしまう処理（特定の業務知識が必要なロジックなど）は、
コードの中にコメントを残すなどの対応をする時もあります。
（ただ、自分はセマンティック命名を意識して、なるべくコードにコメントをつけないように考えます。）
PRで細かい指摘を受けないようにする
PRはコードをチェックする場で、間違い探しの場ではない
「文末改行してください」とか「タイポ直して〜」などの細かい指摘は
本来PRで指摘されるべきではありません。
ただ、これを100%達成することは難しいです。
人間だもの。たまにケアレスミスはします。
ただ、しょうーもないミスをしないように工夫はすべきです。
自分は、コードを書いてから丸一日寝かします
一日じゃなくとも、散歩したり、お手洗いに行ったり、昼寝したり、タスクから思考を完全に切り離すタイミングを作ります。その後でもう一度自分の書いたコードを見ると大体ケアレスミスを発見します。
心理学的にも、人間はタスクの途中で思考を切り離すことは
集中力が回復や、ストレス軽減に役立つという研究結果もあります。
納期という名の死神に睨まれている時もあるでしょう。
それでも自分は、「自分が書いたコードを吟味するのは必要な時間」
と考えてPRを開けるまで余裕を作ります。
ケアレスミスが混じったPRを作って、レビューアーとのやりとりが増えるくらいなら、
多少時間をとってでも、ミスの少ないPRの方が
レビューアーとレビューイのお互いにとって親切だと思います。
気づいたら巨大なPRとなっている
巨大なPRはどうして'罪'なのか
自分なりの考えを述べたいと思います。
- 変更ファイルが多いので、他の開発者とのコンフリクトリスクが高い
- 触っているコードも多いので障害リスクが高い
- レビューするコードが多いのでレビューアーを疲弊させる
- 修正すべきコメントが多くなって、なかなかマージできずに病む
個人的には4番目が一番キツイと思います。
特に自分は自意識過剰といのもあり、同じタスクをホールドしていると、
「いつまで同じタスクやってんだよ」とか「早く終わらせろよ」
と周りが思っているんじゃないか？と感じてしまうのです。
次第に日々の進捗共有で
「レビュー修正があるので、まだ終わっていません...」
と報告するのが辛くなっていきます。
チームの足並みを乱している感覚に襲われて仕事もやりにくくなります。
巨大なPRはレビューアーだけでなく、レビューイにとっても罪な存在なのです
なぜ巨大なPRができてしまうのか
ついで実装が含まれているから
大袈裟な図を書くと、そのPRで実装したい変更以外に
「バグ修正」や「リファクタリング」など
本筋から若干それる実装が含まれる時はありませんか？
これもレビューイに悪気があるわけではありません。
「よかれ」と思ってやっているのです。
そして、レビューアーもそれを察して、
「しょーがないなー」と思いつつレビューを始めてしまうのです。
スコープを分割する
（レビューをもらいやすい細かいプルリクの切り分け方が大変勉強になりました。）
PRのスコープは何を決めます。
その後、変更内容がスコープに入るかを吟味します。
先ほどの例だと、バグ修正・リファクタリング
を完全に別のスコープとして切り出します。
スコープの中を細分化する
次に、スコープの中でも
- 既存モジュールを拡張
- 完全に新しいドメインオブジェクトの作成
のような 既存の振る舞いを変えない機能は先にPRできます。
（一時的に参照されないファイルが存在することもあるが、許容している）
理想的なPRの変更行は 200~400行
変更行が400行を超えると、「欠陥を見つける検知力」が下がる
というデータも存在しています
Pull requestの理想的なサイズとその理由 にて詳しく解説されています。
これも納得の話で、脳が一度に処理できる情報量には限度があり、
その限度を超えないようなPRを作った方が良いです。
慣れない内は git diffコマンドなどで
現状の変更量を意識するのがおすすめです。
以下は、 develop ブランチ と feature ブランチの差分を確認しています。
レビューアーの声がけも大事
結構大事なポイントだと思うのですが、
レビューアーから声掛けすることがかなり重要 だと思っています。
というのも、レビューイは実装における背景知識や実装内容が頭に入っているので、
以下のような傾向があると感じています。（自分がそうだった）
- 書いたコードが読みにくいと気づけない
- 変更行が多すぎると気づけない
- 早くmergeしたい欲が先行し、上記から目を無意識に逸らしている
PRを分割するのはテクニックです。
「これからは分割しようね〜」と言って
すぐに対応できる人はあまりいないでしょう。
レビューアーこそがPRの異変に気づいて、
「diff多いから分割しようか！」と声がけすることが
取り組みの初期段階ではとても重要だと思っています。
「せっかく書いて貰ったしな...」とか思って遠慮すると、
分割PRは一生上手くなりません。
レビューアー側も積極的に分割PRを作るように取り組む必要があります。
頑張っても巨大なPRになってしまう
Railsなどフレームワークの都合や、
既存コードの設計の問題で どうしても変更行が大きくなる時もある と思います。
そんな時に「変更多くてごめん」だけで「ドーン💥」と渡すのではなく
レビューをモブで共有するなどのアプローチも手だと思います。
俗に「モブレビュー」と言われるものです。
複数の開発者が集まり、1つのプルリクエストを一緒にレビューする手法
モブレビューを導入して分かったメリットとデメリットについてが参考になります。
これも、レビューイから話を切り出すことが案外難しいです。
レビューアーも遠慮しないで
「変更が分からないからモブでレビューしません？」と声掛けするのが良いと思います。
そうしたフィードバックがあってレビューイも
「自分が書くコードは難しいんだ..」と現状を把握することに役立つと思います。
merge までに時間がかかってしまう
今まで紹介した以下のことはもちろん重要です。
- 小さなPRを積み上げる
- レビューしやすいようなアプローチを心がける
それだけでは不十分で、レビュアーも頑張る 必要があります。
課題
「あの..僕のPR見てくれてますか？」 と確認したことはないでしょうか？
（僕は何回もあります）
これも組織やチームの風土によって違うとは思うのですが、
当時の僕チームではPRを見るタスクは優先度が高いものではありませんでした。
（確認するのは朝の1時間だけとか...）
なるべく早く '反応' する
レビューイは自分のタスクがレビュー中かどうかがとても気になる生き物です。
（レビューさえ終われば、タスクが完了だから）
それゆえ 「レビュー依頼気づいてますよ〜」と反応することが
レビューイを安心させることに繋がります
すぐにレビューする必要はないんです。
slack 文化なら、以下のようにスタンプを押すだけでいいんです。
（もちろん文章として「明日見ます！」と返信してもいい）
こう言ったコミュニケーションが、レビューイとレビューアーとの
信頼を築くにあたって重要だと感じています。
レビュー＞自分のタスク
僕は「レビュー＞自分のタスク」と レビューを最優先事項まで引き上げています。
なぜか
今はカンバンをチームの運用に取り入れており、
カンバンでは、いかにWIP（作業中タスク）をDONE（完了）に
持って行くかに焦点を当てた議論が起こります。
レビュー中という状態のタスクは最もDONEに近いタスクです。
自分のタスクって作業中（DOING）なんだから、
レビュー中のアイテムの方が優先度は高いよね？
という考えがあるので「先にレビューしよう！」となるわけです。
（ノっている時などすぐにレビューできない時はスタンプ置いたりします）
仮に、「レビューよりも大事なタスクがあるんだ！」という場合は、
ちゃんと周りに共有しましょう。コミュニケーションなので。
マインドセット
原則、レビューイは以下の考えを持った方がいいと感じています
- レビュアーは自分よりも時間あたりの労働価値が高い
- レビュアーの優秀さに甘えない
- PRは小さければ小さいほど良い
レビューアーは技術的スキルレベルが自分と同等、
またはそれ以上であることが大半でしょう。
そんな労働価値の高い人の時間を闇雲に奪うべきではありません。
「時間を奪わないPRを作る」 ことを意識したいです。
また、「あの人は優秀だし..」「頭いいから分かってくれるだろう」は怠慢です。
むしろ 「あの人全然分かってないだろう」くらいの気持ちでいた方が
親切なPRを作る意識ができる と思います。
加えて、コードが難しいと自覚があるなら「モブレビューで説明したいです！」と切り出すのもいいと思います
最後に、巨大PRはレビューイ・レビューアーにとって大罪です。
なるべく小さいPRを作ることを心掛けたいです。
（僕は関心が分離されているなら、たった1行の変更でも大歓迎です。）
次にレビューアーの観点です
- レビュアーは即レスを美徳とする
- PRの違和感に対して素直になる
「反応」は早ければ早いほどいいです。
すぐにレビューする必要はありません。
でもレビューの優先度は上げておきたいです。
レビューアーからの提案はとても大事だと思います。
「変更多いけど、せっかく書いて貰ったし見るか」、「よく分かんないけど解読するか...」
と遠慮する必要はないと思います。
そこで指摘しないと、「レビューイ」のPRの作り方は一生上達しません。
レビューイのためを思って、
「このリファクタ部分、めっちゃありがたいんだけど、別でPR作れる？」
「ごめん、ぱっと見何をしているか分からなかったから、モブでレビューしません？」
と提案していくべきだと思います。（優しく伝えるのも大事です）
振り返り
レビューがなかなか返ってこない
小さなPRですぐレビューできるようにする
レビューでコメントがつきまくって対応に追われる
原則、わかりにくいコードを書かない
補足説明は丁寧に書く
気づいたら巨大なPRとなっている
関心ごとを整理して分割PRを作る、無理ならモブレビューを提案する
mergeまでに時間がかかってしまう
レビュアーも頑張れ
まとめ
ここまで見て頂きありがとうございますm(_ _)m
プルリクエストのテクニックではレビューイに焦点を当てた記事が多い印象ですが、
自分は「レビューアーからの寄り添い」は一定必要だと感じています。
レビューイがレビューアーに敬意を払い、レビューアーがそれに応じるといt
双方からのアプローチで、より良いレビュー環境を作っていきたいと思います。