はじめに
スターフェスティバル株式会社エンジニアのまつやです。
記事の作成から逃げに逃げていたんですが、ハンターに捕まりました。
今回の記事は、タイトルだけでは伝わりにくいですが、プルリクエストを作成・依頼する際、知らず知らずのうちにレビュアーにビッグバンアタックをしていないか、私が意識している内容を書き綴ろうと思います。
※ちなみに、私はビッグバンアタックがどういった攻撃かはまったくわかってません。
プルリクエスト（PR）でのいくつかの悩み
ベテランの方々は直面しない悩みかもしれませんが
私は以下のことを考える時があります。
PR作成の悩み
- 変更ファイル数が膨大になり、同時にコミットすべき範囲がわからない。
- リファクタリングが混ざり、本筋の変更とこちゃまぜになっている。
- サービスロジックが複雑で、PRの説明だけでは背景が伝わらない。
悩みに悩んだ末に、口頭で謝罪した上で同僚にビッグバンアタックをしてしまいます。
レビュアーの悩み
悩むのはレビュイーだけではありません。レビュアーの悩みも同様に切実です。
- 「ファイル数が多いな。見るのに時間がかかりそうだから、後回しにしよう。」
- 「更新行数がものすごいな。見る時間取られそうだから後にしよう。」
結果、現実逃避してタブをそっ閉じする。私もこのような経験が多々あります。
PR作成とコミット時の意識と行動を変えれば、レビュアーのこのような悩みはある程度解消できます。
悩みを解決していこう
変更ファイル数が膨大になり、同時にコミットすべき範囲がわからない
開発にノリノリになっているとき、変更ファイル数が増えるのは「あるある」です。
ある程度書いていき、変更範囲が広がり、気づけば機能実装まで到達してしまう。
何も悪いことじゃない。お前はすごい。東大に行け。
問題はこの後のコミットです。
変更ファイル数をみて「多くなっちゃったけどいいや、コミットしよ」ではなく、踏みとどまり、どこを分割できるかを考え抜いてみましょう。
例えば、メール送信処理の修正を行う際、
【諦めたコミット】
- メールの送信処理を実装した
と、よくあるそのまんま書いちゃうパターンに陥る時がありますが
- メール送信のために必要なデータを作成するBuilderを新しく実装した
- 値をメールのフォーマットに合わせるためにDecoratorを新しく実装した
- Decoratorから渡された値を使用してメール文を生成するロジックを実装した
このようにコミットを落とし込めれば、レビュアーは処理の流れを段階的に理解できます。
さらに、1, 2, 3をそれぞれ別々のPRとして出すことで、レビュアーの脳内メモリをパンクさせない量のファイル数でレビュー依頼が可能です。
「別々で出しちゃうと動作が担保できないのでは？」と思うあなたはテストを書きましょう。
また、デフォルトブランチにマージしても全く影響がないもの（新規ファイルや新規メソッドの追加のみ）は、先にPRを出してしまって問題ありません。
ただし、そのコミットが単一責務を満たしており、コミットメッセージで動作を説明できる範囲であることが条件です。
影響がないものを先にマージしておくことで、ビッグバンアタックならぬビッグバンリリースにならないようにもできます。
ファイル数が多い時は、できる限り分割してコミット・PR依頼をすることで、同僚が弾け飛ぶことはなくなります。
リファクタリングも混ざってこちゃまぜになってる
コードを実装している中で、静的解析ツールで自動的に修正が入ってしまうことがあります。
これもまた、レビュアーにとって読みにくさを倍増させてしまいます。
静的解析ツールにより自動修正が入ることがわかっているのであれば、事前にその修正のみのPRを作成して、デフォルトブランチに雑にぶん投げちゃいましょう。
その方が実装中のインデントやスペースの違和感も消え、モヤモヤが解消されます。
同僚も間違いなく泣きながら喜びます。
また、「ここ綺麗にしておくかー」と思いリファクタリングを行うこともあります。
リファクタリングに関しては、
- 本筋のコード実装のPRを出す。
- その後、別のブランチに切り出してリファクタリングPRを作成する。
上記のように本筋のコード実装と分けて、今このPRで何を修正しているかをはっきりさせましょう。
そのリファクタリングにより、既存の動作が少しでも変わる可能性があるならなおさらです。
リファクタリングによってテストの修正も増え、修正範囲が大きくなる可能性だってあります。
リファクタリングを「やらない」のではなく、「本筋の修正とは完全に分けて考える」ことが、レビュアーへの配慮です。
レガシーコードでテストがない場合は、リファクタリングの前にテストのみを先にコミットし、動作の担保をしてから修正に取り掛かるのがよいでしょう。
サービスロジックが複雑で、PRの説明だけでは背景が伝わらない。
どんなにわかりやすくPRを分割したところで、サービスの事前知識やドメイン知識がないと本質的に理解できない修正もあります。
その場合は、恐らく説明の記述が足りていない可能性があります。
「どのくらいの説明を書いたらいいの？」と不安にならず、知識共有は大切なので、恐れずすべて書いてしまいましょう。関係ないことでなければ、背景、影響範囲など、思いつく限りドキュメントに書いてしまって問題ないと思います。
書きすぎて情報過多になりすぎるのであれば、真面目なレビュアーが「ここは不要」とちゃんとコメントで指摘してくれるはずです(若干他力)
まとめ
私が意識していることを書かせていただきましたが、これだけが正解ではありません。
自分なりのやり方や整理の仕方でわかりやすくPRを出せればなんの問題もないので、一番自分に合ったやり方を見つけてみてください。
レビュイーとレビュアーに関するもっとちゃんとした記事を見たい方は先週の記事を見てください。
Discussion