✏️

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

2025/10/17に公開

株式会社ナレッジワークでバックエンドエンジニアをしている nasum です。
先日開催された Go Conference 2025 で、ナレッジワークのブース展示として ENABLE THIS CODE by your REVIEWという企画を実施しました。

これは、Go で書かれたサンプルコードを皆さんにレビューしてもらうという企画で、合計 4 問作成しました。

2 問目 (1日目の午後) は私が出題を担当しました。
自分はGo歴が長い方ではなく、コードを書くうえでミスをすることが多々ありました。今回はそんな中で自分が躓いてきた中から印象的だったものを選び出題コードを作成しました。楽しんでもらえていたら幸いです。

 出題コードtype Config struct {
	Settings map[string]string
	Timeout  *int             
}

type User struct {
	ID, Name, Email string
}

func isShortDisplayName(name string) bool { return len(name) <= 8 }

var rawUsers = `[
	{"id":"u1","name":"Alice","email":"alice@example.com"},
	{"id":"u2","name":"Bob","email":"bob@example.com"},
	{"id":"u3","name":"こんにちは","email":"konnichiha@example.com"}
]`

func applyDefaults(cfg *Config) {
	if os.Getenv("APP_ENV") == "" {
		cfg.Settings["env"] = "dev"
	}
	if v := os.Getenv("INCLUDE_HEADER"); v != "" {
		cfg.Settings["include_header"] = v 
	}
	if v := os.Getenv("TIMEOUT"); v != "" {
		if n, _ := strconv.Atoi(v); n > 0 {
			cfg.Timeout = &n
		}
	}
}

func buildReport(users []User, cfg *Config) string {
	index := map[string]User{}
	for _, u := range users { index[u.ID] = u }

	var lines []string
	if cfg == nil || cfg.Settings["include_header"] != "false" {
		lines = append(lines, "ID,Name,Email,ShortDisplay")
	}
	for id := range index {
		u := index[id]
		flag := "NG"
		if isShortDisplayName(u.Name) { flag = "OK" }
		lines = append(lines, fmt.Sprintf("%s,%s,%s,%s", id, u.Name, u.Email, flag))
	}
	return strings.Join(lines, "\n")
}

func main() {
	var users []User
	if err := json.Unmarshal([]byte(rawUsers), &users); err != nil {
		fmt.Println("failed to load users:", err); return
	}

	var cfg Config
	applyDefaults(&cfg)

	for _, u := range users {
		if !isShortDisplayName(u.Name) && cfg.Settings["env"] != "prod" {
			fmt.Println("[warn] display name too long:", u.Name)
		}
	}

	if cfg.Timeout == nil {
		fmt.Println("[info] timeout not set; using default")
	} else {
		time.Sleep(time.Duration(*cfg.Timeout) * time.Millisecond)
	}

	report := buildReport(users, &cfg)
	fmt.Println("--- report ---")
	fmt.Println(report)
}
出題したコードはユーザのレポートを出力するというものでした。出力の前に設定の読込や適用を行ったり、バリデーションを行ったりするコードになります。

 想定レビューポイントの解説パッと見ただけでも問題のあるコードだとわかりますが、一応自分の想定していたレビューポイントがあります。それぞれ解説していきます。

 L1-4 Settingsがゼロ値のままなことSettings がゼロ値のままになっています。書き込み時に panic が発生する原因になります。NewConfig()のような関数を作って初期化したほうが安全です。
func NewConfig() *Config {
    return &Config{Settings: make(map[string]string)}
}
また、構造体が使われる applyDefaults でnilチェックをするとなお良いです。

 L10 isShortDisplayNameでlenを使って文字列をカウントしていることlenはバイト数をカウントしているのでマルチバイト文字で意図通りの結果になりません。ASCIIでは偶然正しく見えるような挙動をするので間違えやすいポイントです。
utf8.RuneCountInString 等を検討すると良いです。
func isShortDisplayName(name string) bool { return utf8.RuneCountInString(name) <= 8 }

 L12-16 JSONのコードを直書きJSONのコードを文字列で直書きしています。
Userの定義や、外部ファイルだったら //go:embed を使うなどしたほうが良いです。
//go:embed users.json
var rawUsers string

 L19-21 cfg.Settings['env']初期化していない Settings にアクセスしてしまっているのでここで panic になります。
APP_ENV が空だった場合にのみ "dev" を代入していますが、処理の意図が少し分かりづらいです。

実際には「環境変数が設定されていなければ "dev" をデフォルトとして使う」という意味なので、そのことをコード上ではっきり書くと読みやすくなります。
たとえば次のように書くと、「環境変数 > デフォルト」という優先関係が明確になります。
env := os.Getenv("APP_ENV")
if env == "" {
    env = "dev" // デフォルト値
}
cfg.Settings["env"] = env

 L22-24 os.Getenv("INCLUDE_HEADER")os.Getenv("INCLUDE_HEADER") で取得している値を文字列として扱い比較している。
strconv.ParseBool を使いboolにしたほうが扱いやすいです。

 L25-29 TIMEOUTの値の取扱TIMEOUT は Atoi を使ってintに変換していますが、エラーを無視しています。また、TIMEOUT に入力される値がミリ秒なのかマイクロ秒なのか変数名だけでわからないのも問題です。
エラー処理はもちろん必要ですが、変数名に単位を含めると意図が明確になります。

たとえば TIMEOUT_SEC や TIMEOUT_MS のようにしておくと、コードを読むだけで単位が分かります。
v := os.Getenv("TIMEOUT_MS")
d, err := time.ParseDuration(v + "ms")
if err == nil {
    timeout := int(d.Milliseconds())
    cfg.Timeout = &timeout
}

 L40-45 mapの順序不定index という変数はmapですが、順序が不定にも関わらず for id := range index で処理しています。ここはレポートを作成する処理ですが、順序が不定だとレポートとしてよくありません。

 L44 素朴なCSVの作成CSVを素朴に文字列で作成している。 u.Name や u.Email にカンマが入ると容易に壊れてしまいます。
encoding/csv を使用したほうが安全です。
var buf bytes.Buffer
w := csv.NewWriter(&buf)
for id := range index {
    u := index[id]
    w.Write([]string{u.ID, u.Name, u.Email, flag})
}
w.Flush()
fmt.Println(buf.String())

 付箋で頂いた想定回答以外のレビュー出題者としてはレビューポイントを事前に考えていましたが、レビューをしていただくと自分の想定していた回答ではなく、それでいてなるほど確かにと思うようなレビューをしていただけました。

 L10 マジックナンバーを使っているisShortDisplayName内でマジックナンバーを使っている
定数を以下のように定義するとよいです。
const MaxDisplayNameRunes = 8
マジックナンバーについては当たり前に指摘されるところですが、どういうわけか自分は気づかずに埋め込んでしまいました。普段の仕事でも気をつけないといけませんね。

 L33 indexという名前のmapindex という変数名でmapを作っていますが、index という名前からはmapであることが想定できない。

usersByIDなどのわかりやすい変数名のほうが望ましいです。
これも意図していませんでしたが、確かにおかしいです。普段の仕事では意識して名前を付けていますが、こうして問題用に書いたコードに無意識の癖が出るのは面白い発見でした。

 L59 意味のわからない条件L59でif文に条件を書いていますが、条件の意図がわからない。

条件に意味をつけるためにメソッドにするなどして適切な名前付けが必要だと思う。
確かに言われてみればこのif文の条件は意図が伝わりづらいです。何らかの名前をつけることで読みやすくなるというのはいい指摘だと思いました。

 感想企画のために問題のあるコードを作るのは難しくもあり面白くもありました。普段問題のないコードを作ろうと頑張るところを逆のベクトルで考えるのは新鮮でした。
ちょっと無理やり作った感も出てしまったのでどれぐらい楽しんでもらえるのか不安でしたが、蓋を開けてみれば多くの人が付箋を貼ってくれてレビューを楽しんでくれたのが良かったです。
次回は、「一見すると隙がないようで、よく見るとまずいところが潜んでいる」そんな骨のある問題を作ってみたいと思います。

株式会社ナレッジワーク

株式会社ナレッジワークは「LIFE WITH ENABLEMENT できる喜びが巡る日々を届ける」をミッションに、セールスイネーブルメントAI「ナレッジワーク」を開発・提供しています。本ブログでは、ナレッジワークの技術情報を発信しています。

出題コード

想定レビューポイントの解説

L1-4 Settingsがゼロ値のままなこと

L10 isShortDisplayNameでlenを使って文字列をカウントしていること

L12-16 JSONのコードを直書き

L19-21 cfg.Settings['env']

L22-24 os.Getenv("INCLUDE_HEADER")

L25-29 TIMEOUTの値の取扱

L40-45 mapの順序不定

L44 素朴なCSVの作成

付箋で頂いた想定回答以外のレビュー

L10 マジックナンバーを使っている

L33 indexという名前のmap

L59 意味のわからない条件

感想

Discussion