Go Conference 2025 「ENABLE THIS CODE by your REVIEW」3 問目解説

こんにちは、ナレッジワークのソフトウェアエンジニアの石川宗寿 (munetoshi) です。

先日開催された Go Conference 2025 で、ナレッジワークのブース展示として ENABLE THIS CODE by your REVIEW という企画を実施しました。
これは、Go で書かれたサンプルコードを皆さんにレビューしてもらうという企画で、合計 4 問作成しました。
この記事では、私が担当した 3 問目 (2 日目の午前) の解説をします。

私が一番慣れている言語は Kotlin で、実は Go 歴 2 ヶ月という初心者です。そのため、問題の作成にあたっては、Go 固有の引っかかりポイントではなく、他の言語でもありそうな問題点を中心に考えました。例えば、goroutine の使い方の間違いに関しては、Kotlin の coroutine や他言語の Thread でもありがちな問題を考え、それを goroutine に変えて作成しています。

出題コードと想定レビューポイント

以下のコードでは、Processor の Run でユーザの情報を onMemoryUserRepo に蓄積し、さらに analytics.HeavyCalculation で分析を行い、その結果を analyticsClient に蓄積しています。我ながら「各部分では単純そうに見えるのに、全体では何がやりたいのかよく分からない」コードがかけたと思います。

type UserProfile struct{ details [1024]byte }
type User struct {
	id      int
	name    string
	profile UserProfile
}

func (u *User) GetID() int          { return u.id }
func (u *User) SetName(name string) { u.name = name }

type onMemoryUserRepo struct {
	db map[int]User
	mu sync.Mutex
}

func (r *onMemoryUserRepo) Save(u User, isLegacy bool, legacyID string) error {
	r.mu.Lock()
	uid := u.GetID()
	if isLegacy {
		uid = idconverter.ToNewID(legacyID)
	}
	if uid <= 0 {
		return errors.New("invalid user ID")
	}
	r.db[uid] = u
	r.mu.Unlock()
	return nil
}

type analyticsClient struct{ counts map[string]int }
func (c *analyticsClient) Increment(key string) { c.counts[key]++ }

type Processor struct {
	repo      *onMemoryUserRepo
	analytics *analyticsClient
}
type ProcessingError struct{}

func (e *ProcessingError) Error() string { return "processing error" }

func (p *Processor) Run(userIDs []int, userNames []string, out *[]User) error {
	for i, id := range userIDs {
		if id > 0 {
			if name := userNames[i]; len(name) > 0 {
				user := User{id: id}
				user.SetName(name)
				p.repo.Save(user, id%2 == 0, "")
				*out = append(*out, user)
			}
		}
	}

	var wg sync.WaitGroup
	for _, u := range *out {
		wg.Add(1)
		go func(key string) { defer wg.Done(); p.analytics.Increment(key) }(analytics.HeavyCalculation(u))
	}
	wg.Wait()

	return apperrors.NewProcessingError()
}

レビューポイントとしては、名前やネストといった表面的なものから、設計に関わるものまで多角的になるようにしました。以下が想定しているレビューポイントです。

• l.1, l.16 巨大な struct とその値渡し: UserProfile が構造化されておらず、巨大な struct になっている。また、Save 関数でそれが値渡しされているため、関数呼び出しのたびに巨大なデータのコピーが必要となる。

• l.16 他の引数の値に依存して使われたり使われなかったりする値: Save 関数の引数 legacyID や、User.GetID の値が使われるかどうかは isLegacy の値に依存している。isLegacy が false だと legacyID に何を与えても無視されてしまう。これは誤解を招いてしまうので、関数を分けたり、引数を struct などで構造化したりする必要がある。

• l.23 early return での mutex の unlock 忘れ: Save では Mutex を使っており、最後に Unlock している。しかし、途中の early return で脱出した場合はアンロックされない。 Unlock は defer でするべき。

• l.25 非直感的な key と value の対応付け: isLegacy が true のときは、db map[int]User のキーに legacyID が使われるが、その value User の持つ id は、キーとは別の値となるため、混乱しやすい。legacyID と User.id を混ぜてキーにするのではなく、スライスを２つに分けたり、ID のペアの struct を定義して整理する必要がある。

• ll.33-36 Processor が repo と analytics を状態として持つ設計: 現在の設計では、repo と analytics に結果を書き込んでいるが、struct を見ただけではその意図がわからない。また、複数回 Run を呼び出したときの挙動も不明確。Run の結果を analytics に書き込むのではなく、戻り値として返すことで analytics をフィールドとして持つ必要がなくなり、データの流れも明確になる。

• l.42 インデックスによる 2 つのスライスの要素の暗黙的な関連付け: Run の引数 userIDs と userNames はインデックスで関連付けられ、「同じインデックスなら同じユーザ」として処理されている。しかし、それはドキュメンテーションコメントにも書かれておらず、コードの中身を読まないと理解できない。そのため、間違った順のスライスを渡してしまうというバグを招きかねない。ID と Name のペアとなる struct を作り、そのスライスを引数に取るべき。

• l.42 戻り値で十分な out parameter: Run の結果のスライスは out parameter out で返却されているが、これは戻り値で十分。out parameter にすると、結果が何であるかが関数のシグネチャから読み取りにくくなったり、引数として渡されたスライスがすでに要素を持つ場合にどうなるかといったケースを考慮する必要があったりと、問題が発生しやすい。

• l.56 analytics.Increment による競合状態: analytics.Increment は goroutine の中で実行されているため、排他制御が必要となる。しかしこの関数内では、そのまま ++ が実行されるため競合が発生する。Mutex などを使って、Increment がアトミックに実行されるようにするべき。

• l.56 意味の薄い goroutine: この goroutine では、Increment だけが並行に実行される。本来、並行させたかったと思われる analytics.HeavyCalculation は、関数の引数として与えられるているため、goroutine 外で実行されてしまう。無名関数の引数としては u を渡し、analytics.HeavyCalculation を関数内で実行されるようにするべき。

• その他、様々な指摘点: 不適切な命名、ll.42-51 の不要なネスト、不要な Getter/Setter (GetID/SetName)、常にエラーを返す Run など、他にも様々な指摘点がある。

頂いたレビューと議論

まず、問題作成時に想定していなかったレビューポイントとして、大きく 2 つありました。

• sync.WaitGroup の Go() を使う: 8 月にリリースされた Go 1.25 では、新たに Go() が追加されています。これを使うことで、Add/Done/Wait の定型句を書く必要がなく、かつ、それを忘れることによるバグを回避できます。
• len(name) がマルチバイト文字で不正確になる: len はバイト数を返すため、マルチバイト文字を扱う場合に文字数と一致せず、意図しない挙動になる可能性があります。代わりに utf8.RuneCountInString を使うべきです。実はこれと同じポイントは、2 問目で想定されたレビューポイントとして埋め込まれていました。

また、様々な意見が聞け、議論になったポイントとしては以下の 3 つがあります。各意見でそれぞれメリットとデメリットがあり、何を優先するかによっても戦略が変わるトピックになりました。

• user ID をどう取り扱うか: UUID を使うべき、非負であるなら uint にするべき、ID を明示的に別の type として定義するべき、legacy ID と統合した構造体を作ることで legacy ID を隠すべき...など。
• Save での legacy ID をどう取り扱うか: Save を legacy ID 用と通常用で分けてしまうのか、逆に user ID の構造体を作ることで legacy かどうかを意識しないようにするのか。また、どちらの場合でも onMemoryUserRepo ではどのようにデータを持つようにするのか。
• 設計としてどう責任を分割するか: 現在の Processor が何でもするという状況は分割したいが、それをどのように分けるのか (レイヤで切るのか「ユーザの登録」と「集計」で分けるのかなど)。

企画を実施しての感想

まだ Go には慣れていないのですが、他の言語での知見を応用することで、必ずしも「正解」が決まるわけではないような、議論の余地がある問題が作れたのではないかなと思います。そして実際に参加者と楽しくかつ新鮮な議論ができました。ご参加いただいた皆様、ありがとうございました。