Zenn
⛳

Go初学者へのコードレビューでよくあったコメント20選

2024/11/14に公開
2件
Go
コードレビュー
tech

はじめに

こんにちは、ソーシャルベッティング事業本部 海外ベッティング事業部の山崎です。
本記事では、Effective GoやGoogle のスタイルガイド、Code Review Commentsといった公式資料、Future Architectの記事などを参考に、Go を初めて触る開発者を対象にした汎用的なレビューコメントの 20 選を紹介します。

大きく以下の４つのセクションに分けました

  • 言語仕様に関わる内容
  • 標準パッケージの使い方
  • エラーの扱い方
  • 単体テスト

Linter の活用について

可能な限り lint で自動化して人の手が加わる前に静的解析でできればベターです。
特にこの記事で紹介するような汎用的なコメントについてはいくつか反映できる lint もあると認知しております。
そのような設定の lint config サンプルをまとめようとも思いましたが、実際に運用まで至っておらず信頼性に欠けると思い今回詳しくは記載していません。
CI として取り入れられるとより良いです、良い設定などあればぜひ教えていただきたいです。

言語仕様に関わる内容

1. Slice の空判定は == nil でなくて len() == 0 を使おう

頻出度: ★★★★★ (5)

Go のスライスは、「空のスライス」と「nil のスライス」が異なる状態として存在します。そのため、スライスが空であるかどうかのチェックには、単純な == nil ではなく、len() を使って判定する方がより確実です。

    userIDs := getUserIDs() // ユーザーIDのスライスを取得

    // BAD: nilチェックのみだと空スライスを見逃す
    if userIDs == nil {
        fmt.Println("No user IDs to process.")
        return
    }

    // GOOD: lenチェックにより空スライスも判定可能
    if len(userIDs) == 0 {
        fmt.Println("No user IDs to process.")
        return
    }

「ゼロ値に初期化されたスライス」や「空のスライス」が一貫して判定できる。

参考資料：

2. 事前にスライスの長さが分かる場合は容量を指定しよう

頻出度: ★★★★★ (5)

Go ではスライスを宣言する際に、スライスの「長さ」と「容量」を指定できます。スライスの長さが事前に分かっている場合、容量を設定してスライスを確保すると、効率的にメモリを使用でき、パフォーマンスも向上します。

users := getUsers()
userIDs := make([]string, 0, len(users)) // ユーザーIDリストの容量をあらかじめ指定

for _, user := range users {
    userIDs = append(userIDs, user.ID)
}

スライスの容量不足による再割り当てはメモリ効率を低下させ、大規模なデータを扱うときにボトルネックとなりがちです。

小さなデータを扱う場合は特に気にしなくても問題ありませんが、最適化の観点からこのクセをつけておくと、性能の向上に役立ちます。

実際のメモリアロケーションがどのように行われるかは Go 公式の資料が参考になります →Go Slices: usage and internals

参考資料：

3. Struct のフィールドを利用したロジックは Struct 側に寄せよう

頻出度: ★★★★☆ (4)

Struct に関連するロジックは呼び出し元ではなく、Struct 側にメソッドとして定義することで、コードの可読性・保守性を向上させることができます。

たとえば、User 構造体に「アカウントがアクティブかどうか」を判断するロジックを追加したい場合、以下のように構造体のメソッドとして実装します。

type User struct {
    Name       string
    IsVerified bool
    IsDisabled bool
}

// BAD: 呼び出し元にアクティブ判定のロジックが分散する
func isActiveUser(user User) bool {
    return user.IsVerified && !IsDisabled
}

func greetUser(user User) string {
    if isActiveUser(user) {
        return fmt.Sprintf("Hello, active user: %s", user.Name)
    }
    return fmt.Sprintf("Welcome back! Please verify your account, %s.", u.Name)
}

このように呼び出し元に判定ロジックを持たせると、他の場所でも同様のロジックを書く必要が生じ、コードの分散や冗長性につながります。

User 構造体に IsActive メソッドを追加することで、アクティブ判定のロジックを一箇所に集約できます。

type User struct {
    Name       string
    IsVerified bool
    IsDisabled bool
}

// GOOD: User 構造体にメソッドとして定義
func (u *User) IsActive() bool {
    return u.IsVerified && !IsDisabled
}

func (u *User) Greet() string {
    if u.IsActive() {
        return fmt.Sprintf("Hello, active user: %s", u.Name)
    }
    return fmt.Sprintf("Welcome back! Please verify your account, %s.", u.Name)
}

このようにすることで、User 構造体がアカウントのアクティブ状態を一元的に管理でき、呼び出し元のコードもシンプルになります。判定条件が変更になった場合も、IsActive メソッドを修正するだけで対応できるため、保守性が高まります。

これにより、User 構造体がドメインモデルとして成長し、サービス全体で「アクティブなユーザーの定義」を明確に一元管理できます。

またロジックが構造体のメソッドとして閉じることで、意図しない呼び出し側での重複処理や、複雑な条件の再現による誤用を防げます。

参考資料：

4. Slice や Map を返すときは直接返さず型定義しよう

頻出度: ★★★★☆ (4)

前の項目の具体例にもなる内容です。

例えば、ユーザーの名前を返す関数を考えます。

type User {
     ID string
     Name string
     IsPrivate bool
}

func UserNames(users []User) []string{
     userNames := make([]string, 0, len(users))
     for _, u := range users {
         userNames = append(userNames, u.Name)
     }
     return userNames
}

ここでは UserNames 関数がユーザーを受け取り名前を返しています。この方法でも問題なく実行できますが将来的な拡張性が乏しく、ドメインの知識が分散してしまっています。

そこでスライス型を Users としてラップする新しい型を定義し、Names() という Users 型に寄せた関数を定義します。

type User struct{
     ID string
     Name string
     IsPrivate bool
}

type Users []User

func (us Users) Names() []string {
     userNames := make([]string, 0, len(us))
     for _, u := range us {
         userNames = append(userNames, u.Name)
     }
     return userNames
}

これで Users 型に Names というメソッドを追加して、名前のスライスを取得できるようになります。

コードの意図が明確になること、こちらもまた User モデルがドメインモデルとして成長していくことが良いです。

例えばこのようにすることで便利な関数を集約できたり、ドメインとして必要な仕様書としての役割に成長したりすることが期待できます。

func (us Users) Filter() Users {
      // フィルタリングロジック
}

func (us Users) Sort() {
     // ソートロジック
}

参考資料:

5. 関数呼び出しの返却を直接返そう

頻出度: ★★★★☆ (4)

Go では、関数の戻り値を複数返すことができます。

直接returnを使って返すことでコードがシンプルになります。

// BAD
func bar() (string, error) {
      s, err := foo()
      if err != nil {
        return "", err
      }
      return s, nil
}

// GOOD: 呼び出した関数の戻り値をそのまま返すことで、シンプルで可読性が高い
func bar() (string, error) {
    return foo()
}

error をラップしたりする場合やログを出すなどの用途があるとできません。

参考資料：

6. 複数の同一型の返り値がある場合は返り値に名前をつけよう

頻出度: ★★☆☆☆ (2)

// BAD: 呼び出し側で返り値の意味が不明確
func getUserDetails() (bool, bool)

// GOOD: 返り値の名前をつけることで意図を明確に
func getUserDetails() (isActive bool, isAdmin bool)

複数の同一型を返す関数では、呼び出し側が返り値の意味を誤解する可能性が高くなります。

Go では型に名前はつけられませんが、返り値に名前をつけることはできます。基本的に名前は不要ですが、複数の同一型の返り値を返さなければいけないときに名前をつけることで、意味の曖昧さを解消できます。

参考資料：

7. ネストを深くさせないことを心がけよう

頻出度: ★★☆☆☆ (2)

アーリーリターンできるならそうする。

err := foo()
if err != nil {
    return err
} else {
    return nil
}

// GOOD
err := foo()
if err != nil {
    return err
}
return nil

if inputName != "" {
    u.name = inputName
} else {
    u.name = "名無しさん"
}

// GOOD: アーリーリターンでネストを浅く
u.name = "名無しさん"
if inputName != "" {
    u.name = inputName
}

if-else のどちらも変数代入する場合は条件一致した場合に上書きするようにしましょう。

Go Code Review Comments でも特にエラーのネストで浅く仕様という言及があるのはこだわりを感じて興味深い内容です。

https://go.dev/wiki/CodeReviewComments#indent-error-flow

参考資料：

標準パッケージの使い方

8. 期間の変数を扱うときはtime.Duration 型を使おう

頻出度: ★★★★☆(4)

Go では、時間や期間を表現するときには time.Duration 型を使用すると、コードがより明確になります。

// BAD: intで秒数を扱うと分かりにくく、誤解の元になる
func Wait(delay int) {
    :
    time.Sleep(time.Duration(delay) * time.Millisecond)
}

Wait(10) // これは「10秒」か、それとも「10ミリ秒」か？不明確

とするよりも。

// GOOD: time.Durationを直接引数として受け取ることで単位が明確に
func Wait(delay time.Duration) {
    time.Sleep(delay)
}

Wait(10 * time.Millisecond) // ミリ秒単位と一目で分かる
Wait(1 * time.Second)       // 秒単位も直感的

このようにすることで、関数の呼び出し側でも単位が明確になり、誤解を防げます。また、コード内での計算や処理も time.Duration によって一貫した表現ができるため、メンテナンス性が高まります。

時間に関連するすべての処理には、まず time パッケージが利用できるかを検討すると良いでしょう。

参考資料：

9. 暗号化用途では math/rand を使わずに crypto/rand を使おう

頻出度: ★★☆☆☆(2)

math/randは暗号論的に安全でない疑似乱数であるため暗号鍵やセキュリティトークンなど、予測不可能な乱数を生成したいときは crypto/randを使用するほうがよいとされています。math/rand 自体が悪というわけではないです。

また最近であれば math/rand を使いたい場合にも math/rand/v2 の利用を検討しても良いでしょう。
math/rand と同じく非暗号化用途に使うものですが、従来の math/rand と比較して新しいジェネレータや自動のシード設定、安全度の強化などがされています。詳しくは以下の記事が参考になります。
https://go.dev/blog/randv2

参考資料：

10. 正規表現のコンパイルはグローバル変数で行おう

頻出度: ★☆☆☆☆(1)

regexp パッケージの利用時。

// BAD: 毎回の呼び出しでコンパイルが行われ、処理が遅くなる
func sanitizeInput(input string) string {
    newlineRE := regexp.MustCompile(`\n`)
    return newlineRE.ReplaceAllString(input, " ")
}

// GOOD: 事前にコンパイルし、再利用することで効率化
var newlineRE = regexp.MustCompile(`\n`)

func sanitizeInput(input string) string {
    return newlineRE.ReplaceAllString(input, " ")
}

regexp における正規表現のコンパイルはコストがかかるため、毎回呼び出すのではなく、事前にコンパイルして再利用するのが効率的です。

特に大規模な処理や複数回の正規表現使用が必要な場合はパフォーマンが向上します。

グローバル変数に保持することで、関数間での共有も簡単になります。

参考:

エラーの扱い方

11. 成功時のエラー返却はnilで返そう

頻出度: ★★★★☆(4)

エラーを返す関数において、処理が成功したことを明示的にしめすためにも error の返り値にnilエラーを返すことが一般的です。

Go においてエラーを伴う関数では、error が nil であるかどうかによって成功/失敗判断がされます。

// BAD: エラーがない場合も返り値にerrが含まれていて成功かどうか曖昧
func GetUsers() (*User, error) {
    user, err := db.GetUserByID(id)
    users.Filter()
    return user, err
}

// GOOD: ↑と結果は同じだが明示的で可読性が高い
func GetUser(id int) (*User, error) {
    user, err := db.GetUserByID(id)
    if err != nil {
        return nil, err
    }
    users.Filter()
    return user, nil
}

このように成功時にエラーとしてnilを返すことで、関数の意図がより明確になり、コードの可読性も向上します。
説明の都合上、取得とリターンまでの中間処理として Filter() などの例の関数を入れました。
もし中間処理がない場合は 5.関数呼び出しの返却を直接返そう の方を優先してシンプルに返すことを推奨します。

参考:

12. エラーの等価判定には errors.Is を使おう

頻出度: ★★☆☆☆(2)

Go ではエラーを下流から上流にラップする場合があるので、error の比較をするときはerrors.Is を使うことでラップされたエラーも含めて等価判定が行えます。

func openFile(filename string) error {
    _, err := os.Open(filename)
    // BAD: 単純な等価判定ではエラーがラップされている場合に失敗する
    if err == os.ErrNotExist {
        return fmt.Errorf("file does not exist: %w", err)
    }

    // GOOD: errors.Is を使ってラップされたエラーも正しく判定
    if errors.Is(err, os.ErrNotExist) {
        return fmt.Errorf("file does not exist: %w", err)
    }

    return err
}

errors.Isはラップされたエラーの判定にも対応しているため、特定のエラーの意図を明確にすることでエラー処理が堅牢になります。

Go の公式ドキュメントでも推奨されており、errors.Is のドキュメントで詳細が確認できます。公式の FAQ でもエラーのハンドリング方法として記載されている →How should I change my error-handling code to work with the new features

また golangci-lint などでも制限できます:

参考資料:

13. エラーのスコープを小さくしよう

頻出度: ★★☆☆☆(2)

エラースコープを狭めることで、意図しないエラーの再利用を防ぎ、コードの可読性と保守性を向上させることができます。

特に Go ではエラーを都度処理する必要性があるためエラーのスコープを小さくすることはじわじわと保守性があがります。

// BAD: 同じエラーメッセージを再利用するとコピペミスが発生しやすい
func performTasks() error {
    err := taskA()
    if err != nil {
        return err
    }

    err = taskB()
    if err != nil {
        return err // taskA のエラーが再利用される可能性
    }
    return nil
}

// GOOD: 独立したスコープ内でエラーを管理
func performTasks() error {
    if err := taskA(); err != nil {
        return err
    }
    if err := taskB(); err != nil {
        return err
    }
  }

このようにすることで、エラーがそれぞれのスコープで完結するため、後続の処理でエラーにアクセスできなくなります。

ただし、スコープを狭める手法は、条件によって処理がネストしすぎる可能性もあるため、コードの複雑さを避ける工夫も必要です。

参考:

14. エラーの適切な伝搬をしよう

頻出度: ★★★☆☆(2)

エラーを伝搬する際には、エラーのラップを適切に行い、エラーの発生元や詳細を付加することが重要です。

%wを使うことでエラーをラップし、発生元の情報を含めながらも、上位でエラーハンドリングがしやすくなります。これにより、エラーメッセージが明確になり、デバッグが容易になります。

またこれは Go の標準ツールであるgo vet で検知できます。

// BAD1: err を伝搬せずに本来のエラー内容が失われる
func readFile(filename string) error {
    data, err := os.ReadFile(filename)
    if err != nil {
        return fmt.Errorf("failed to read file")
    }
    return nil
}
// BAD2: %v を使用するとエラーが単なる文字列となり、伝搬されない
func readFile(filename string) error {
    data, err := os.ReadFile(filename)
    if err != nil {
        return fmt.Errorf("failed to read file: %v", err)
    }
    _ = data
    return nil
}

// GOOD: エラーをラップし、詳細な情報を保持
func readFile(filename string) error {
    data, err := os.ReadFile(filename)
    if err != nil {
        return fmt.Errorf("failed to read file %s: %w", filename, err)
    }
    return nil
}

参考資料:

https://github.com/knsh14/uber-style-guide-ja/blob/master/guide.md#unnecessary-else

Reduce Scope of Variables

https://github.com/knsh14/uber-style-guide-ja/blob/master/guide.md#unnecessary-else

15. エラー の文言は小文字で始めよう

頻出度: ★★☆☆☆(2)

// BAD
fmt.Errorf("Something went wrong")

// GOOD
fmt.Errorf("something went wrong")

他の文脈と組み合わせた際に不要な大文字や句読点が生じないようにするため。

例えば、fmt.Errorf("something bad") のように書き、ログで log.Printf("Reading %s: %v", filename, err) としても違和感がないようにするため。

参考:

単体テスト

16. Table Driven Test を積極的に利用しよう

頻出度: ★★★★★(5)

Table Driven Test(TDT)は、テストのエントリーに入力値と期待値を含めることができ可読性も高く条件を把握しやすいテストです。

Go では公式からも推奨されている書き方で標準ライブラリやその他パッケージでも広く採用されているテスト手法です。

VSCode でも標準の Go 拡張にgotests の機能組み込まれておりポチポチするだけでファイルや関数からテストテンプレートを自動生成してくれます。

// BAD: 個別にテストケースを書くと冗長になりやすい
func TestIsAdult(t *testing.T) {
    u := User{Age: 19}
    if u.IsAdult() != false {
        t.Error("Expected false, got true")
    }
    u = User{Age: 20}
    if u.IsAdult() != true {
        t.Error("Expected true, got false")
    }
}

// GOOD: Table Driven Test で複数のテストケースを一括で記述
func TestIsAdult(t *testing.T) {
    tests := []struct {
        age      int
        expected bool
    }{
        {age: 19, expected: false},
        {age: 20, expected: true},
        {age: 25, expected: true},
    }

    for _, tt := range tests {
        u := User{Age: tt.age}
        if got := u.IsAdult(); got != tt.expected {
            t.Errorf("IsAdult(%d) = %v; want %v", tt.age, got, tt.expected)
        }
    }
}

ただし実際のプロダクトコードでの E2E テストや複雑なシナリオでは、単一の TDT では表現が難しい場合もあります。
このような場合では別途 E2E テストを用意したりなど柔軟な対応が必要になります。

参考資料。

17. [testify] assert と require を使い分けよう

頻出度: ★★★☆☆(3)

requireとassertの使い分けは、テストの実行フローを制御する上でキモになります。

assert と require はインターフェースは同じですが以下の特徴があります。

  • require は後続の処理をしない (失敗時にt.FailNow() される)
  • assert は後続の処理を続ける

require を適切に使うことでテストコードでの意図しないエラーやパニックを防げます。テストケースの見通しが良くなり、実行フローに一貫性が保たれます。

got, err := getUsers()
require.NoError(t, err) // エラーがないのは前提条件。後続の処理が成り立たないのでrequire
require.Len(t, got, 1) // １つ取得できる期待も前提条件。ここがだめなら後続でnil panic が起きる可能性がある
assert.Equal(t, got[0].name, "Bob")  // Len の判定をassert でやっているとここで nil panic が起きる
assert.Equal(t, got[0].age, 20)

参考資料:

18. [testify] 適切な関数で評価しよう

頻出度: ★★★★★(5)

got, err := getUsers()
// BAD
require.Equal(t, nil, err)
require.Equal(t, 1, len(got))
assert.Equal(t, false, got[0].IsDisabled)
assert.Equal(t, nil, got[0].Point)
assert.Equal(t, []string{}, got[0].Keywords)

// BETTER
require.NoError(t, err)
require.Len(t, got, 1)
assert.False(t, got[0].IsDisabled)
assert.Nil(t, got[0].Point)
assert.Empty(t, got[0].Keywords)

適切な関数を使うことで、コードレビューやデバッグ時に意図が明確になりやすいことに加えて型安全が保証され予期せぬ不一致を回避できます。

  • 型安全についての補足
    たとえばrequire.Equal(t, nil, err) の例でいうと、 nil は型が指定されていないため、Go では error 型と互換性のない nil が与えられた場合に型不一致が発生し、意図したエラーチェックが行われない可能性があります。
    require.NoError(t, err) は err が error 型であることを前提にしているため、型不一致によるエラーが発生する心配がありません。

    func getError() error {
    var err *MyCustomError = nil // *MyCustomError型のnil
    return err                   // errorインターフェースにキャスト
}

func TestEquality(t *testing.T) {
    err := getError()

    // BAD: 型が一致していない可能性があるため、意図した動作にならない
    require.Equal(t, nil, err) // *MyCustomError型とerror型で型不一致

    // GOOD: require.NoErrorはerror型のnilかどうかを正確に判定
    require.NoError(t, err)
}

    sample:
    https://go.dev/play/p/J151vHBEDw7

testify の関数はやることがシンプルなので Interface の関数名を一通り眺めてみるのがいいでしょう。
基本的には Error, NoError, Equal, True/False, Len, Nil, NotNil あたりがよく利用される印象です。

https://pkg.go.dev/github.com/stretchr/testify@v1.9.0/require

19. [testify] アサーション関数ごとに適切な引数順を意識しよう

頻出度: ★★★★☆(4)

got, err := getUser()
require.NoError(t, err)

//BAD
require.Equal(t, got.Name, "Bob")

// GOOD
require.Equal(t, "Bob", got.Name)

testify のアサーション関数では各関数ごとに適切な引数順を意識することが重要です。
require.Equal や assert.Equal などのテストアサーションにおいて、expected, actual の順で引数を渡しましょう。

// BAD: 引数の順序が逆
require.EqualError(t, "file not found", err)

// GOOD: actual, expected の順
require.EqualError(t, err, "file not found")

require.EqualError や assert.Contains：actual, expected の順で引数を渡します。このように、アサーション関数によって引数の順が異なるため、公式ドキュメントやインターフェースを参照して確認することが大切です。

順番を守らないと、テスト失敗時のエラーメッセージがあべこべになり、期待値と実際の値が何なのかが分かりにくくなります。

どちらに入れても成功/失敗自体は同じですがエラーメッセージの表示が紛らわしくなり、認識負荷が高まります。

参考資料:

20. [testify] 評価に関わるコメントは arg として渡そう

頻出度: ★★★☆☆(3)

// 削除関数のテスト
err := deleteUser()
require.NoError(t, err)

got, err := getUser("user_id")

// BAD:
require.NoError(t, err) // 論理削除のためレコードは残っておりエラーが起きない

// GOOD: require.NoError にコメントを渡し、意図を明確にする
require.NoError(t, err, "論理削除のためレコードは残っておりエラーが起きない")

testify の assert や require には msgAndArgs というオプショナルの引数があり、これを使うことでテストケースの詳細な意図や背景をコメントとしてメッセージに加えられます。

特に条件が複雑なテストや、エラーメッセージが一目では理解しづらいテストの場合、追加コメントがあることで、デバッグ時に失敗の原因や期待される動作をすぐに把握できます。

テストが失敗した場合にも詳細なデバッグ情報が得られます。

積極的に使ってテストコードの可読性と実用性を高めていきましょう。

まとめ

主観的ではありますが、よく指摘するコメントをまとめた記事でした。

これまでのコードレビューでは、Effective Go、Google のスタイルガイド、Code Review Commentsといった公式資料や、Future Architectの技術記事などを頻繁に参考にしてきました。

この記事もまた誰かのコードレビューや Go スキルの向上に役立てば幸いです。
ここまで読んでいただきありがとうございました！

👉 MIXI ではコミュニケーションを一緒につくる仲間を募集しています！
Go の案件も多く幅広いプロダクトに携わる機会があります。ぜひお気軽にご連絡ください！

https://mixigroup-recruit.mixi.co.jp/

参考資料

GitHubで編集を提案

Discussion

やんやんやんやん

よくまとまった記事でとても参考になりました！！

  1. Slice や Map を返すときは直接返さず型定義しよう

を読んでいて、 タイトルからは「関数がsliceやmapを返すときにその返り値の型をちゃんと定義しよう」という風に解釈していたのですが、それ以降に書かれている例を見ると「sliceやmapに対して操作をするような関数を定義する際は操作されるsliceやmapを型として定義しよう」ということが伝えたいことなのかなと感じました。

NAKKA-KNAKKA-K

まとめ記事、ありがとうございました！

タイトルからは「関数がsliceやmapを返すときにその返り値の型をちゃんと定義しよう」という風に解釈していたのですが、

私も同じように誤解したのですが、
これはValueObject的な話で、

それ以降に書かれている例を見ると「sliceやmapに対して操作をするような関数を定義する際は操作されるsliceやmapを型として定義しよう」ということが伝えたいことなのかな

これがファーストクラスコレクション的な話ですよね。

何かしら名称を含めてあげると意図が語弊なく伝わるかもしれません。