Pull Requestのセルフレビューでやっていること
この記事は LITALICO Engineers Advent Calendar 2021 その2 の7日目の記事です。
Pull Requestを作るときに割と入念にセルフレビューしてからレビュー依頼するようにしており、また他メンバーのもののレビューをしているときに「これは事前にセルフレビューして修正しておいてほしいなぁ」と思うことがあり、セルフレビューの重要性を感じるこの頃です。
レビュー時に都度指摘してもよいのですが、意外と観点が多いこともあり、思考の整理がてらアウトプットしてみるか、という試みです。
なぜ自分でレビュー？
まず、そもそも自分で書いたコードを何故自分でレビューするのか？という点について書いておきます。
よくプログラミング一般の議論で「N日前のコードは他人のコード」と言われます。ということは、Pull Requestを作成した時点の自分から見て、該当コードを書き始めた時点のコードはある程度他人のコードだと言えそうです。
また、"コード書きとしての自分"と"レビュアーとしての自分"では、思考形態や着眼点が多少なりとも違うのではないかと思っています。その意味でも、セルフレビューをしようとしている自分とコードを書いた主はある程度別人だと言えるでしょう。
つまり、目の前にあるPull Requestは何やら他人のものなのです。
他人のコードとなれば、それはレビューすべきですね。
しかし他人ではあれど、そのコード作者は大変良く知った仲であり長年の付き合いがあり遠慮も全く無い相手です。なので他のメンバーに先駆けてレビューを行い、コードの中身からPull Requestの書き方など細かいところまで積極的に改善点を洗い出し、修正しておきます。
他のレビュアーは一次レビューを通った状態で見ることになるので、未レビューのものよりも幾分とスッキリとしたものを見ることになり、より時間をかけずに快適にレビューができることでしょう。
...というようなモチベーションでセルフレビューをしています。以降、実際にどんなことを見ているかを書いていきます。
やったこと全体を見直す
まずは全体やったことを見直し、頭に入っているものでやったことは全部だっけ？というのを確認します。ここで思っていたものに漏れがある（Pull Requestの本文に書き漏れている）場合は追記や修正をします。
また筆者はここで事前の検証項目を洗い出すこともしています。もちろんPull Request全体の変更に対して検証するものもありますが、加えて実装を見ながら漏れがないか確認していくとより確実になります。
しょうもないミスの発見
コードのロジックやインタフェースの切り方のような話ではない、ただのミスとしか言いようのないものも確認しておきます。
よくあるtypoの他に、関係ない別の作業の差分の混入、自動補完・フォーマット系機能による意図せぬ変更、Mac特有の謎の制御文字[1]、ファイル末尾改行の有無など、何も考えずに直せるようなものが案外あったりします。
これらを他のレビュアーに指摘させるのは大変無駄なので、事前にキレイに直しておきます。
コード全体を俯瞰したレビュー
他人のPull Requestをレビューするような、いわゆるコードレビューも真面目にやります。自分の書いたコードですが、前述のように他人のコードでもあるので、全体を書き終えた上で見ると案外直せることがあったりします。
詳細はレビュアーとして技術になりそれだけで記事や本が書けてしまうので割愛しますが、全体を俯瞰してみるとここの関数名や変数名はもう少し良くできるなとか、ここはコメント添えたほうが良いなとか、直せそうなところは積極的に直していきます。
直すのは自分なので、他人にレビューで言うには細かすぎるようなものも遠慮なく直せます。
前述のミスの修正も含め、作文における推敲をしていると思うとしっくりくるかもしれません。自分のコードをより良い状態にしておき、その良いコードへレビューを貰って更に良い状態を目指します。
レビューしづらさの解消
指摘するようなレビューもやりますが、レビュアーの負荷軽減を目的とした"レビューしづらさ"の検知と対応もやっていきます。
ここちょっと意図わかりにくいかも？
意図を直接表すようなコードにリファクタしたり、コメントを添えたりして解消を試みます。
作者である自分からみても怪しいと思うのであれば、他のレビュアーから見ればより厳しいと思って良いでしょう。
なお自分のPull Requestでコメントを書く手段は下記の二通りがあります:
- コード中にコメントを書く → コメントを添えたい理由が恒久的であり、将来そのコードを見た人も同じ疑問を持ちうる場合
- Pull Requestの差分にAdd single commentする → 元の書き方をやめた/コードを削除した理由を書きたい場合や、直近の出来事などに関連する場合
セルフレビューの段階では後者のほうが作業的に楽ではありますが、理由が前者のそれであればサボらずにコードに追記していきます。
この実装微妙だしもっと良いやり方ありそう？
悩んだが良い方法が思い浮かばない、もしくは良い方法があるが事情により採用できないのであれば、FIXMEコメントを添えてお気持ち表明しておきます。
方法が思い浮かんでおり実施への障害も無いのであれば、その通りに実装して改善します。
実装当時から時間が経っていると、案外名案が浮かぶこともあります。
差分ファイルめっちゃある...
それは複数のPull Requestする機運ではないか？と考えます。
大枠で一つの目的のためのコード群であっても、考え方を変えれば別々の小タスクにできることが多く、可能な限りやっておきます。
変更をスムーズに遂行するためのリファクタリング / 変更本体で二つに分ける、というのはよくあるパターンの一つです。
他のレビュアーがコードの詳細を見る前からつらい気持ちにならないよう、負荷を下げておきます。
インデント変えただけなのに全域が差分扱いになっている
大きなifブロックをealry returnに変えた場合やHTMLで親のタグの囲いを増減させた場合など、意味的な変更は非常に少ないながらdiff表示の範囲が広くなる場合があります。
レビュー画面のHide whitespace changes機能でうまい具合に差分を見ることができるので、Pull Request本文や該当行へのコメントなどを使いレビュアーにその旨を告知しておきます。
複製して修正したので全部差分として出ている（本質的な差分が分からない）
ABテストの兼ね合いなどで既存のコードを複製した後に一部だけ書き換える、といった場合、git上のdiffとしては複製+修正コードのすべてがaddとして表示されます。
この状態では作者が実施した実装を正確にレビューできないため、なんらか差分だけを認識できるように工夫します。
確実なのは複製と編集を別々のPull Requestに分離することですが、他にも純粋な複製のみのcommitと追加の編集commitを分離し、後者だけの差分リンクを添えるという方法もあります。
Pull Requestでは下記のように特定commit範囲の差分リンクを作ることができるので、そのリンクをレビュアーにわかるところに提示すると良いでしょう。
その他、なんらかわかりにくいのだけど仕方ない
如何ともし難い特殊ケースもたまにはあります。
可能な限りシステム的にわかりやすく表現できないか考えるべきですが、難しいのであればせめてその旨と元の意図をコメントなどで添えておきます。
まとめ
対応に少々手間や時間がかかるものもありますが、レビュアーの手間を削減できるのであればトータルでペイできるものが多いと思います。
特にレビュアーが複数人の場合、効果は減らせる時間x人数で効いてくるため、積極的に対応したいです。
また、レビュアーに指摘や質問をされる可能性のある部分を事前に対応できていると、のちのコミュニケーションコストや所要時間が下がり、手早くmergeできます。
効率化ができるのもさることながら、せっかくなら一発OKでサッと通せると嬉しいですね。
-
過去の自分を含む一部のMac使いに稀にある、謎の文字がテキスト内に気付かぬ間に混入している現象。web上やdiffなどでは見えるがMac上では不可視なため非常に気づきにくい。実際のところ何が起こっているのかはよくわかりません... ↩︎
Discussion