🏳

リリースフラグによってプルリクを小さくする方法

2022/01/22に公開

GitHub

先にまとめ

変更は追加とリリースと削除が混同するので、レビューコストが高い
リリースフラグを用い、旧実装を残すことで、新実装を追加行のみで表現できる
リリースフラグの反転だけで新実装への切り替えができ、切り戻しなどが容易になりやすい

解説しようとしていること

「変更」を「追加」と「削除」に分ける

リファクタリングと仕様変更はプルリクとしては分けた方がレビューコストが下がる、というのは有名な話ですし、実際にレビューをしたことがある方々なら実感できるでしょう。
参考: レビューをもらいやすい細かいプルリクの切り分け方 - Software engineering from east direction

本記事では加えて、「変更」を「追加」と「削除」に分けた方が良い、という話をします。
追加・削除・変更のプルリクを見たときに、レビュアは以下のような観点でそれぞれレビューするかと思います。

追加: 追加行が中身。追加する箇所が仕様の機能を実現できているかをレビューする。設計が正しいものかの観点でもレビューする。
削除: 削除行が中身。削除する箇所が不要となっているかをレビューする。削除漏れなどもチェックする。
変更: 追加行と削除行が混在。旧実装で実現できていたことが新実装に切り替えても問題ないかをレビューする。本当は設計や削除漏れも細かく見た方が良いが、あまりそこまで頭が回らない。

考えてみれば、変更は追加と削除を合わせたものです。
追加単体・削除単体のプルリクと比較したら、レビューコストは飛躍的に上昇してしまいます。
レビューコストは 変更 > 追加 ≒ 削除 ということになります。
可能であれば、変更は追加と削除に分けた方が良いでしょう。

「変更」と「リリース」を分ける

また、「変更」と「リリース」も分けた方が良い、という話をします。
マージするとリリースされてしまう機能だというのは、それだけで懸念点が増え、レビューコストが上がります。
レビュアも以下のようなコメントを残すことに集中してしまうでしょう。

「基本的にはLGTMですが、この機能はXX月XX日にリリース予定ですよね？今マージするとまずい気がしてます。そういえばデータベースのマイグレーションも行わなければならないですね。あと互換性が・・・」

変更とリリースにはそれぞれ以下の特徴があります。

変更: 本当は設計として正しいか・仕様を満たしているかを中身にレビューしたい
リリース: リリースタイミング・マイグレーション・互換性などがレビューの観点となる

レビュアを新実装の設計などの本質的な箇所に集中させるためにも、この二者も可能であれば分けた方が良いでしょう。
本記事では 変更 = 追加 + リリース + 削除 であるとし、変更のプルリクを分割する方法について解説します。

実装

説明ではGoを使ってますが、あんまり言語は関係ないです。日本語だし。

プルリクを分けないで「変更」する

変更前

package sample

func 実装() {
    return 旧実装()
}

上の旧実装から、下の新実装に改修するとします。

変更後

package sample

func 実装() {
    return 新実装()
}

このdiffを取ると、以下のように変更(= 追加 + リリース + 削除)として出てしまいます。

 package sample
 
 func 実装() {
-   return 旧実装()
+   return 新実装()
 }

実際には旧実装も新実装もlines of codeはそこそこ多いものします。
削除行と追加行が混在することになり、レビューコストが高くなります。

追加・リリース・削除でプルリクを分けるやり方

そこで、以下のように実装します。

変更後

package sample

const リリースフラグ = false

func 実装() {
    if リリースフラグ {
        return 新実装()
    }

    return 旧実装()
}

ポイントは以下です。

リリースフラグを設ける。これを反転させれば新実装がリリースされることになる。機能開発時には旧実装の方に処理が進むようにする。
新実装は追加行だけで表現する。
旧実装はまるごと残す(削除しない)。

こうすることで、diffは以下の通りになります。

 package sample
 
+const リリースフラグ = false
+
 func 実装() {
+   if リリースフラグ {
+       return 新実装()
+   }
+
    return 旧実装()
 }

プルリクをコードの追加だけで表現できるようになりました。
以下の効果があります。

旧実装はレビューしなくて良いと即判断できる(というかそもそも差分として出ない)
リリースフラグは旧実装に向いているので、すぐマージして大丈夫かの観点でレビューする必要が無い。

これをリリースするときには、以下のような改修を行います。

 package sample
 
-const リリースフラグ = false
+const リリースフラグ = true
 
 func 実装() {
    if リリースフラグ {
        return 新実装()
    }
 
    return 旧実装()
 }

こうすることで、リリース時の変更行を1行〜数行に抑えることができます。
そして、旧実装が不要と判断できるタイミングになったら、以下の改修を行います。

 package sample
 
-const リリースフラグ = true
-
 func 実装() {
-   if リリースフラグ {
-       return 新実装()
-   }
-
-   return 旧実装()
+   return 新実装()
 }

見た目上の差分は多いものの、実際には旧実装を消して新実装の階層を浅くしただけです。
プルリクを出した際にはその旨をコメントすれば、レビュアも「変更」としてレビューしなくて良いと判断できるでしょう。
既に新実装の方で動き始めており、新実装側をいじりさえしなければ新実装側に影響は出ません。
旧実装側の消し忘れ等がないかにレビューを集中させることができ、プルリクのレビューコストは比較的小さくなります。

具体例での実装

以上、日本語で公式っぽく書いてしまいましたが、例が無いと分からんっていう人もいると思うので、例を作りました。
上記だけで理解できれば、スキップしても大丈夫です。

具体例

前提

私たちが所属するA社の運営するサービスには、宝くじを申し込む機能があります。
宝くじの申し込みを受け、その申し込み情報やユーザ情報によって、運用により実際に宝くじを出していました。
この運用業務を外部委託することになり、外部委託先をHoge社とします。
Hoge社にはWEB APIによって連携することになりました。

旧仕様: 申し込み情報を保持しておき、その情報を元に通知を作成する。
新仕様: 申し込み情報を元にHoge社にAPIを出し、その情報を保持しておくだけでOK。通知はHoge社から出されてしまうので、入れないことにした。

簡単にまとめると、既に動いている機能の仕様を変えようとしているってことです。

プルリクを分けないで「変更」する

旧仕様の実装は以下です。

package usecase

import (
	"errors"
	"example.com/go-mod-test/flag/pkg"
)

type RegisterLotteryOrderUsecase struct {
	lotteryOrderRepo *pkg.LotteryOrderRepository
	notificationRepo *pkg.NotificationRepository
}

func (u *RegisterLotteryOrderUsecase) exec(uid string) (lotteryOrderID string, err error) {
	id, err := pkg.GenerateID()
	if err != nil {
		return "", err
	}

	// 宝くじの注文を作成
	lotteryOrder, err := pkg.NewLotteryOrder(id, uid, "", "")
	if err != nil {
		return "", errors.New("failed to create new lottery order")
	}

	// その注文を永続化
	err = u.lotteryOrderRepo.Save(lotteryOrder)
	if err != nil {
		return "", errors.New("failed to save lottery order")
	}

	// 宝くじの注文の通知を作成
	notif, err := pkg.NewNotificationOfRegisteringLotteryOrder(lotteryOrder)
	if err != nil {
		return "", errors.New("failed to create notification")
	}

	// その通知を永続化。通知も飛ばしてくれるとする
	err = u.notificationRepo.Save(notif)
	if err != nil {
		return "", errors.New("failed to save notification")
	}

	return lotteryOrder.LotteryOrderID, nil
}

新仕様の実装は以下です。

package usecase

import (
	"errors"
	"example.com/go-mod-test/flag/pkg"
)

type HogeClient struct{}

func (c *HogeClient) postLotteries(_ string) (string, error) {
	// emailを使って、Hoge社の宝くじの注文するAPIを叩いて、Hoge社管理の宝くじIDが返ってくるとしましょう
	return "12345", nil
}

type RegisterLotteryOrderUsecase struct {
	hoge             *HogeClient
	userRepo         *pkg.UserRepository
	lotteryOrderRepo *pkg.LotteryOrderRepository
}

func (u *RegisterLotteryOrderUsecase) exec(uid string) (lotteryOrderID string, err error) {
	id, err := pkg.GenerateID()
	if err != nil {
		return "", err
	}

	// Emailを出すために該当ユーザを抽出
	user := u.userRepo.FindByUID(uid)

	// Hoge社にAPIで連携。このときに通知も飛ぶ
	hogeLotteryOrderID, err := u.hoge.postLotteries(user.Email)

	// 内部で管理するため、宝くじの注文を作成
	lotteryOrder, err := pkg.NewLotteryOrder(id, uid, "hoge", hogeLotteryOrderID)
	if err != nil {
		return "", errors.New("failed to create new lottery order")
	}

	// その注文を永続化
	err = u.lotteryOrderRepo.Save(lotteryOrder)
	if err != nil {
		return "", errors.New("failed to save lottery order")
	}

	return lotteryOrder.LotteryOrderID, nil
}

旧実装と新実装のdiffを取ると以下のようになります。

 package usecase
 
 import (
        "errors"
        "example.com/go-mod-test/flag/pkg"
 )
 
+type HogeClient struct{}
+
+func (c *HogeClient) postLotteries(_ string) (string, error) {
+       // emailを使って、Hoge社の宝くじの注文するAPIを叩いて、Hoge社管理の宝くじIDが返ってくるとしましょう
+       return "12345", nil
+}
+
 type RegisterLotteryOrderUsecase struct {
+       hoge             *HogeClient
+       userRepo         *pkg.UserRepository
        lotteryOrderRepo *pkg.LotteryOrderRepository
-       notificationRepo *pkg.NotificationRepository
 }
 
 func (u *RegisterLotteryOrderUsecase) exec(uid string) (lotteryOrderID string, err error) {
        id, err := pkg.GenerateID()
        if err != nil {
                return "", err
        }
 
-       // 宝くじの注文を作成
-       lotteryOrder, err := pkg.NewLotteryOrder(id, uid, "", "")
+       // Emailを出すために該当ユーザを抽出
+       user := u.userRepo.FindByUID(uid)
+
+       // Hoge社にAPIで連携。このときに通知も飛ぶ
+       hogeLotteryOrderID, err := u.hoge.postLotteries(user.Email)
+
+       // 内部で管理するため、宝くじの注文を作成
+       lotteryOrder, err := pkg.NewLotteryOrder(id, uid, "hoge", hogeLotteryOrderID)
        if err != nil {
                return "", errors.New("failed to create new lottery order")
        }
 
        // その注文を永続化
        err = u.lotteryOrderRepo.Save(lotteryOrder)
        if err != nil {
                return "", errors.New("failed to save lottery order")
        }
 
-       // 宝くじの注文の通知を作成
-       notif, err := pkg.NewNotificationOfRegisteringLotteryOrder(lotteryOrder)
-       if err != nil {
-               return "", errors.New("failed to create notification")
-       }
-
-       // その通知を永続化。通知も飛ばしてくれるとする
-       err = u.notificationRepo.Save(notif)
-       if err != nil {
-               return "", errors.New("failed to save notification")
-       }
-
        return lotteryOrder.LotteryOrderID, nil
 }

diffの中に旧実装の削除と、新実装の追加が混在しています。^[1]

追加・リリース・削除でプルリクを分けるやり方

新実装の方を、以下のように実装します。

package usecase

import (
	"errors"
	"example.com/go-mod-test/flag/pkg"
)

const registersLotteryWithHoge = false

type HogeClient struct{}

func (c *HogeClient) postLotteries(_ string) (string, error) {
	// emailを使って、Hoge社の宝くじの注文するAPIを叩いて、Hoge社管理の宝くじIDが返ってくるとしましょう
	return "12345", nil
}

type RegisterLotteryOrderUsecase struct {
	hoge             *HogeClient
	userRepo         *pkg.UserRepository
	lotteryOrderRepo *pkg.LotteryOrderRepository
	notificationRepo *pkg.NotificationRepository
}

func (u *RegisterLotteryOrderUsecase) exec(uid string) (lotteryOrderID string, err error) {
	id, err := pkg.GenerateID()
	if err != nil {
		return "", err
	}

	if registersLotteryWithHoge {
		// Emailを出すために該当ユーザを抽出
		user := u.userRepo.FindByUID(uid)

		// Hoge社にAPIで連携。このときに通知も飛ぶ
		hogeLotteryOrderID, err := u.hoge.postLotteries(user.Email)

		// 内部で管理するため、宝くじの注文を作成
		lotteryOrder, err := pkg.NewLotteryOrder(id, uid, "hoge", hogeLotteryOrderID)
		if err != nil {
			return "", errors.New("failed to create new lottery order")
		}

		// その注文を永続化
		err = u.lotteryOrderRepo.Save(lotteryOrder)
		if err != nil {
			return "", errors.New("failed to save lottery order")
		}

		return lotteryOrder.LotteryOrderID, nil
	}

	// 宝くじの注文を作成
	lotteryOrder, err := pkg.NewLotteryOrder(id, uid, "", "")
	if err != nil {
		return "", errors.New("failed to create new lottery order")
	}

	// その注文を永続化
	err = u.lotteryOrderRepo.Save(lotteryOrder)
	if err != nil {
		return "", errors.New("failed to save lottery order")
	}

	// 宝くじの注文の通知を作成
	notif, err := pkg.NewNotificationOfRegisteringLotteryOrder(lotteryOrder)
	if err != nil {
		return "", errors.New("failed to create notification")
	}

	// その通知を永続化。通知も飛ばしてくれるとする
	err = u.notificationRepo.Save(notif)
	if err != nil {
		return "", errors.New("failed to save notification")
	}

	return lotteryOrder.LotteryOrderID, nil
}

こうすることで、diffは以下の通りになります。

 package usecase
 
 import (
        "errors"
        "example.com/go-mod-test/flag/pkg"
 )
 
+const registersLotteryWithHoge = false
+
+type HogeClient struct{}
+
+func (c *HogeClient) postLotteries(_ string) (string, error) {
+       // emailを使って、Hoge社の宝くじの注文するAPIを叩いて、Hoge社管理の宝くじIDが返ってくるとしましょう
+       return "12345", nil
+}
+
 type RegisterLotteryOrderUsecase struct {
+       hoge             *HogeClient
+       userRepo         *pkg.UserRepository
        lotteryOrderRepo *pkg.LotteryOrderRepository
        notificationRepo *pkg.NotificationRepository
 }
 
 func (u *RegisterLotteryOrderUsecase) exec(uid string) (lotteryOrderID string, err error) {
        id, err := pkg.GenerateID()
        if err != nil {
                return "", err
        }
 
+       if registersLotteryWithHoge {
+               // Emailを出すために該当ユーザを抽出
+               user := u.userRepo.FindByUID(uid)
+
+               // Hoge社にAPIで連携。このときに通知も飛ぶ
+               hogeLotteryOrderID, err := u.hoge.postLotteries(user.Email)
+
+               // 内部で管理するため、宝くじの注文を作成
+               lotteryOrder, err := pkg.NewLotteryOrder(id, uid, "hoge", hogeLotteryOrderID)
+               if err != nil {
+                       return "", errors.New("failed to create new lottery order")
+               }
+
+               // その注文を永続化
+               err = u.lotteryOrderRepo.Save(lotteryOrder)
+               if err != nil {
+                       return "", errors.New("failed to save lottery order")
+               }
+
+               return lotteryOrder.LotteryOrderID, nil
+       }
+
        // 宝くじの注文を作成
        lotteryOrder, err := pkg.NewLotteryOrder(id, uid, "", "")
        if err != nil {
                return "", errors.New("failed to create new lottery order")
        }
 
        // その注文を永続化
        err = u.lotteryOrderRepo.Save(lotteryOrder)
        if err != nil {
                return "", errors.New("failed to save lottery order")
        }
 
        // 宝くじの注文の通知を作成
        notif, err := pkg.NewNotificationOfRegisteringLotteryOrder(lotteryOrder)
        if err != nil {
                return "", errors.New("failed to create notification")
        }
 
        // その通知を永続化。通知も飛ばしてくれるとする
        err = u.notificationRepo.Save(notif)
        if err != nil {
                return "", errors.New("failed to save notification")
        }
 
        return lotteryOrder.LotteryOrderID, nil
 }

新実装を追加行だけで表現することができました。
あとは前述の通り、リリースフラグを反転させるプルリクと、旧実装を削除するプルリクをそれぞれ作成すれば良いことになります。

副産物

切り戻しが容易になる

リリースまで行った後に、バグが発覚したとします。
切り戻しの判断は、プルリクが大きくなればなるほど困難になりやすいです。
revertするとしても、その後にマージされていたマージした実装などがあると、コンフリクトを解決するのも一苦労です。

一方、本記事で解説したやり方であれば、機能追加とリリースを分けています。
よってrevertする必要はなく、基本的にはリリースフラグを反転させれば良いだけになり、切り戻しが明示的です。

また、変更行が少なくでき、リリースフラグの使用箇所での影響範囲さえ調べれば良くなります。
もちろん、データベースのマイグレーションなどの問題は残るものの、切り戻し判断・作業が比較的簡単にできるのではないでしょうか。

旧実装のコードを一旦残すことができる

新実装でバグが見つかった時に、「旧実装ではどう実装していたっけ？」を確認するケースがあると思います。
もちろんhistoryを追っていけば旧実装には辿り着きますが、正直面倒だと思います。

本記事で解説したやり方であれば、旧実装のコードを一定期間残すことができます。
master/mainブランチ上で旧実装を手軽に確認することができます。

リリースタイミングをデプロイ依存にさせない設計にもできる

例えば、「この機能はXX日のXX時ピッタシくらいにリリースしたい」という要望があったとします。
時間厳守の要望においてデプロイによって切り替えを制御するのはあまり賢くありません。

こういう場合は、リリースフラグの部分を関数で書き、「XX時以降は新実装」という分岐を入れます。
XX時までにデプロイさえしておけば、厳密な時間で新実装側を機能させ始めることができます。

本番環境では旧仕様が動き、検証環境では新仕様をデバッグさせることもできる

検証環境では新実装を試したいという要望も多いでしょう。

こういう場合にもリリースフラグの部分を関数で書き、「検証環境では新実装・本番環境では旧実装」という分岐にします。
検証環境でもmaster/mainブランチでデバッグを進めることができ、マージされていないコードを少なくできる、変更のリードタイムを縮めることができるという効果があります^[2]

後書き

400LoC以上のプルリクエストは人類にはレビューできないという話は、多くの実装者が認識すべきことかなと思ってます。
筆者もこれを認識しており、本記事で解説したようなやり方を用いて、プルリクエストの目的をなるべく一つに絞るように意識しています。
新機能の開発であっても、大きいブランチとして溜め込まず、細かくマージ・デプロイしていくスタイルで開発するようにしています。
同僚からは有難いことに「プルリクがレビューしやすい」というような評価を頂けています。
みなさんもレッツスモールプルリクエスト！

脚注

ぶっちゃけこのくらいの変更行ならレビューできる、というツッコミは無しで。あくまで例です。 ↩︎
この場合は実際には本番環境には新機能は反映されていないため、「変更のリードタイムは縮まっていないだろ」という見方もあると思います。 ↩︎

先にまとめ

解説しようとしていること

「変更」を「追加」と「削除」に分ける

「変更」と「リリース」を分ける

実装

プルリクを分けないで「変更」する

追加・リリース・削除でプルリクを分けるやり方

具体例での実装

前提

プルリクを分けないで「変更」する

追加・リリース・削除でプルリクを分けるやり方

副産物

切り戻しが容易になる

旧実装のコードを一旦残すことができる

リリースタイミングをデプロイ依存にさせない設計にもできる

本番環境では旧仕様が動き、検証環境では新仕様をデバッグさせることもできる

後書き

Discussion