さんちゃのblog

プログラミングのことなど

機能追加と同時にリファクタリングをしてもいいか

「機能追加と同時にリファクタリングをしてもいい」という記事がはてブに上がっていたので、思うところを述べる。

scrapbox.io

結論

問題意識

機能追加をするときにリファクタは避けられない。既存の処理を共通化して再利用して機能を追加した方が効率的だからである。

しかし、機能追加とリファクタが混ざったPRを出すのはダメだ。なぜなら、リファクタによって機能が失われていないかどうか確認するのが面倒になるからだ。

解決策

では、どうすればよいのか?

開発中はリファクタと機能追加を平行して行い、リファクタと機能追加のPull Requestを別々に出せば良いのである。

手順

この解決策を実行するには以下のようにする:

  1. 開発中はリファクタと機能追加を同時並行で行い、適宜コミットする。
  2. PRを作成する前にgit rebase -iコミットを入れ替え 、「リファクタのみのブランチ」と「機能追加のみのブランチ」を作成する。
  3. それぞれのブランチについてPull Requestを作成する。
  4. レビュアーは、「リファクタのみのブランチ」をレビュー&マージした後に、「機能追加のみのブランチ」のレビューを行う。

つまり、開発完了時の

* implement B <-- some_new_feature
*  refactor Y
* implement A
*  refactor X
*        base

という歴史を、

* implement B <-- some_new_feature
* implement A
*  refactor Y <-- refactor_for_new_feature
*  refactor X
*        base

のように書き換え、2つのPull Requestを作成するのである。

このようにすれば、「機能追加とリファクタは同時にやりたい」という欲求と、「機能追加とリファクタは別にレビューしたい」という欲求を両立させることができる。

元記事に対する感想

「リファクタと機能実装を分けると、リファクタの方のPRでは「設計が壊れ」ているコードをレビューしなければいけないので良くない」みたいな記述があったが、ちゃんと意味のあるリファクタであれば、それはそれとして独立した一つのPRになると思う。「リファクタのPRは『設計が壊れ』ている」ことは、変なリファクタをしてるからなのではないかと思う。

また、「githubのクソUIのせいで、リファクタと機能実装が混ぜ込まれたPRがレビューしづらい」という意味の記述があったが、ほならねとしか言えない。そもそもdiffを出してるのは、変更範囲だけを見たいという欲求があるからであって、「全体を見てもらえばもっとわかりやすいんです!!!」というのはレビュワーに対する負荷を増やすだけだ。