Go Style & Idioms

Review Go code for style and idioms based on the official Go Code Review Comments wiki.

Note on Project-Specific Conventions

This skill covers general Go idioms and standard practices. Some projects use different conventions:

•For DDD architecture patterns, see go-review-ddd skill
•For repository-specific patterns, see go-repository skill
•Some projects use pointer-first approaches that differ from standard Go guidance

1. Formatting & Documentation

gofmt

All Go code must be formatted with gofmt.

Review Checklist

•Issue if code is not gofmt-formatted (indentation, spacing)
•Issue if imports are not grouped and sorted

// Issue: manual formatting
func main(){
	fmt.Println( "hello" )
}

// Correct: gofmt-formatted
func main() {
	fmt.Println("hello")
}

Comment Sentences

Doc comments should be complete sentences.

Review Checklist

•Issue if comments don't start with the name of the element being described
•Issue if comments don't end with a period
•Issue if exported names lack doc comments

// Issue: incomplete comment
// returns the user name
func GetName() string { ... }

// Correct: complete sentence starting with function name
// GetName returns the user's full name.
func GetName() string { ... }

Doc Comments

Every exported name should have a doc comment.

Review Checklist

•Issue if exported functions, types, constants, or variables lack doc comments
•Issue if doc comments don't start with the declared name
•OK for unexported names to have comments but not required

// Issue: no doc comment on exported function
func ProcessUser(user *User) error { ... }

// Correct: doc comment starting with function name
// ProcessUser validates and stores the user in the database.
// It returns an error if validation fails.
func ProcessUser(user *User) error { ... }

// Issue: doc comment doesn't start with type name
// This represents a user
type User struct { ... }

// Correct: starts with type name
// User represents an authenticated user in the system.
type User struct { ... }

Mixed Caps

Use MixedCaps or mixedCaps for multi-word names, not underscores.

Review Checklist

•Issue if names use underscores (e.g., user_count)
•Issue if exported names use wrong casing
•OK for test names to use underscores

// Issue: underscores
var user_count int
func get_user_by_id() { ... }

// Correct: MixedCaps
var userCount int
func GetUserByID() { ... }  // Note: ID not Id

Package Comments

Every package should have a package comment.

Review Checklist

•Issue if package lacks a package comment
•Issue if package comment has blank line between comment and package clause
•Issue if package main comment doesn't describe the binary
•OK for one file in multi-file package to have the comment

// Issue: no package comment
package user

// Issue: blank line between comment and package
// Package user provides user management functionality.

package user

// Correct: package comment adjacent to package clause
// Package user provides user management functionality.
package user

For package main, use a binary description:

// Issue: generic comment
// Package main is the entry point.
package main

// Correct: describes what the binary does
// Binary server runs the HTTP API server for user management.
package main

// Also acceptable prefixes: Command, Program
// Command mycli provides a CLI for managing users.
package main

Line Length

No rigid limit, but avoid uncomfortably long lines.

Review Checklist

•Issue if lines are so long they require horizontal scrolling
•Issue if long lines are broken arbitrarily rather than semantically
•OK for long string literals or generated code
•OK to break lines at natural semantic boundaries

// Issue: arbitrarily broken
result := someFunction(ctx, userID,
    email, name, createdAt, updatedAt)

// Correct: break at semantic boundaries
result := someFunction(
    ctx,
    userID,
    email,
    name,
    createdAt,
    updatedAt,
)

// Also correct: keep short calls on one line
result := someFunction(ctx, userID)

Guidelines:

•Break lines for readability, not to hit a character limit
•Use shorter variable names if it helps
•Reduce nesting or extract helper functions for complex logic

2. Error Handling Idioms

Error Strings

Error strings should not be capitalized or end with punctuation.

Review Checklist

•Issue if error strings start with capital letter
•Issue if error strings end with punctuation (. ! ?)
•OK for proper nouns or acronyms to be capitalized

// Issue: capitalized, punctuation
errors.New("Failed to connect to database.")
fmt.Errorf("Invalid user ID!")

// Correct: lowercase, no punctuation
errors.New("failed to connect to database")
fmt.Errorf("invalid user ID")

// OK: proper noun
errors.New("failed to connect to PostgreSQL")

Handle Errors

Don't discard errors using _.

Review Checklist

•Issue if errors are explicitly discarded with _ = someFunc()
•Issue if errors are ignored without comment
•OK if there's a comment explaining why error is ignored

// Issue: discarding error
_ = file.Close()

// Issue: not handling error
someFunc()

// Correct: handle error
if err := file.Close(); err != nil {
    return fmt.Errorf("failed to close file: %w", err)
}

// OK: rare case with comment
_ = file.Close() // closing read-only file, error not relevant

Indent Error Flow

Handle errors first, then normal code.

Review Checklist

•Issue if happy path is indented and error handling is in else clause
•Issue if error checks don't use early returns

// Issue: happy path indented
func doSomething() error {
    if err := step1(); err == nil {
        if err := step2(); err == nil {
            return step3()
        } else {
            return err
        }
    } else {
        return err
    }
}

// Correct: error flow handling with early returns
func doSomething() error {
    if err := step1(); err != nil {
        return err
    }
    if err := step2(); err != nil {
        return err
    }
    return step3()
}

Don't Panic

Don't use panic for normal error handling.

Review Checklist

•Issue if panic is used for expected errors
•Issue if panic is used in library code
•OK for panic in init() or truly unrecoverable situations

// Issue: panic for expected error
func GetUser(id string) *User {
    user, err := db.Find(id)
    if err != nil {
        panic(err)  // Wrong!
    }
    return user
}

// Correct: return error
func GetUser(id string) (*User, error) {
    user, err := db.Find(id)
    if err != nil {
        return nil, fmt.Errorf("failed to get user: %w", err)
    }
    return user, nil
}

In-Band Errors

Avoid in-band error values (special return values indicating errors).

Review Checklist

•Issue if functions return -1, 0, nil, or empty string to indicate errors
•Issue if callers must remember special error values

// Issue: in-band error (-1 means error)
func GetAge() int {
    if err := loadUser(); err != nil {
        return -1  // Caller must check for -1
    }
    return user.Age
}

// Correct: explicit error return
func GetAge() (int, error) {
    if err := loadUser(); err != nil {
        return 0, fmt.Errorf("failed to load user: %w", err)
    }
    return user.Age, nil
}

3. Code Organization

Import Grouping

Group imports into standard library, third-party, and local imports.

Review Checklist

•Issue if imports are not grouped
•Issue if groups are not separated by blank lines
•Issue if imports within groups are not sorted

// Issue: not grouped
import (
    "fmt"
    "github.com/gin-gonic/gin"
    "context"
    "myapp/internal/domain"
    "strings"
)

// Correct: grouped and sorted
import (
    "context"
    "fmt"
    "strings"

    "github.com/gin-gonic/gin"

    "myapp/internal/domain"
)

Import Blank

Use blank imports (import _ "pkg") only for side effects.

Review Checklist

•Issue if blank import used in library code (not main or tests)
•Issue if blank import purpose is not documented with comment
•OK for blank imports in main package or test files
•OK for database drivers, image format registration, etc.

// Issue: blank import in library without explanation
import _ "github.com/lib/pq"

// Correct: blank import with comment explaining why
import (
    "database/sql"

    _ "github.com/lib/pq" // PostgreSQL driver registration
)

// Correct: in main.go for side effects
package main

import (
    _ "image/png" // Register PNG format
    _ "image/jpeg" // Register JPEG format
)

Import Dot

Avoid dot imports (import . "pkg") in production code.

Review Checklist

•Issue if dot import used outside of test files
•Issue if dot import used when not required for circular dependency
•OK for dot imports in _test.go files to break circular dependencies
•OK sparingly in tests for readability (e.g., DSL packages)

// Issue: dot import in production code
import . "github.com/onsi/gomega"

func Process() {
    Expect(result).To(BeTrue())  // Where does Expect come from?
}

// Correct: explicit import
import "github.com/onsi/gomega"

func Process() {
    gomega.Expect(result).To(gomega.BeTrue())
}

// OK: dot import in test file for readability
// foo_test.go
package foo_test

import (
    . "myapp/foo"  // Testing package foo from external test
)

4. Interfaces

Define Interfaces Where Used

Define interfaces in the package that uses them, not in the package that implements them.

Review Checklist

•Issue if package exports interfaces for its own concrete types
•Issue if interfaces are defined in "interfaces.go" alongside implementations
•Issue if interfaces have many methods (prefer small interfaces)
•OK for well-known interfaces like io.Reader, io.Writer

// Issue: interface defined with implementation
// storage/storage.go
package storage

type Storage interface {  // Don't define here!
    Get(key string) ([]byte, error)
    Set(key string, value []byte) error
}

type RedisStorage struct { ... }
func (r *RedisStorage) Get(key string) ([]byte, error) { ... }
func (r *RedisStorage) Set(key string, value []byte) error { ... }

// Correct: interface defined where it's used
// cache/cache.go
package cache

// Storage is the interface for cache backends.
type Storage interface {
    Get(key string) ([]byte, error)
    Set(key string, value []byte) error
}

type Cache struct {
    storage Storage  // Accept interface
}

func New(s Storage) *Cache {
    return &Cache{storage: s}
}

Keep Interfaces Small

Prefer small interfaces with 1-3 methods.

Review Checklist

•Issue if interface has more than 5 methods
•Issue if you're creating interfaces just for mocking
•OK to compose small interfaces into larger ones when needed

// Issue: large interface
type UserService interface {
    Create(ctx context.Context, user *User) error
    Get(ctx context.Context, id string) (*User, error)
    Update(ctx context.Context, user *User) error
    Delete(ctx context.Context, id string) error
    List(ctx context.Context) ([]*User, error)
    Search(ctx context.Context, query string) ([]*User, error)
    // ... 10 more methods
}

// Correct: small, focused interfaces
type UserReader interface {
    Get(ctx context.Context, id string) (*User, error)
}

type UserWriter interface {
    Create(ctx context.Context, user *User) error
    Update(ctx context.Context, user *User) error
}

// Compose when needed
type UserReadWriter interface {
    UserReader
    UserWriter
}

Don't Export Interfaces for Mocking

Design APIs that are testable without mocking interfaces.

Review Checklist

•Issue if interfaces exist solely for testing/mocking
•Issue if production code only has one implementation of an interface
•OK for interfaces with multiple real implementations

// Issue: interface just for mocking
type TimeProvider interface {
    Now() time.Time
}

type realTime struct{}
func (realTime) Now() time.Time { return time.Now() }

// Correct: accept a function or use dependency injection
type Service struct {
    now func() time.Time
}

func New() *Service {
    return &Service{now: time.Now}
}

// In tests:
svc := &Service{now: func() time.Time { return fixedTime }}

5. Functions & Methods

Named Result Parameters

Use named results sparingly, only when they improve clarity.

Review Checklist

•Issue if named results are used just to enable naked returns
•Issue if named results don't improve documentation
•OK for named results in defer-heavy functions or long functions

// Issue: named result just for naked return
func GetUser(id string) (user *User, err error) {
    user, err = db.Find(id)
    return  // Naked return obscures what's being returned
}

// Correct: explicit return
func GetUser(id string) (*User, error) {
    user, err := db.Find(id)
    return user, err
}

// OK: named results for documentation in complex function
func Divide(dividend, divisor float64) (quotient, remainder float64, err error) {
    if divisor == 0 {
        return 0, 0, errors.New("division by zero")
    }
    quotient = dividend / divisor
    remainder = math.Mod(dividend, divisor)
    return quotient, remainder, nil
}

Naked Returns

Avoid naked returns except in very short functions.

Review Checklist

•Issue if naked returns used in functions longer than 5-10 lines
•Issue if naked return makes it unclear what's being returned

// Issue: naked return in long function
func Process(data string) (result string, err error) {
    // ... 50 lines of code ...
    return  // What values are being returned?
}

// Correct: explicit return
func Process(data string) (string, error) {
    // ... processing ...
    return result, err
}

Receiver Names

Use short, consistent receiver names (1-2 letters).

Review Checklist

•Issue if receiver name is "this" or "self"
•Issue if receiver name is overly long
•Issue if receiver names are inconsistent across methods of same type
•Issue if receiver name shadows package or type name

// Issue: "this" or "self"
func (this User) GetName() string { ... }
func (self User) GetEmail() string { ... }

// Issue: inconsistent names
func (u User) GetName() string { ... }
func (user User) GetEmail() string { ... }

// Correct: short, consistent
func (u *User) GetName() string { ... }
func (u *User) GetEmail() string { ... }

Receiver Type

Use pointer receivers unless you have a good reason not to.

Review Checklist

•Issue if methods on same type have inconsistent receiver types (some pointer, some value)
•OK for small immutable structs to use value receivers
•OK for value receivers on methods that don't modify receiver

Note: Some projects use pointer-first conventions. This guidance is for general Go code.

// Issue: inconsistent receiver types
func (u *User) SetName(name string) { u.Name = name }  // Pointer
func (u User) GetName() string { return u.Name }       // Value - inconsistent!

// Correct: consistent pointer receivers
func (u *User) SetName(name string) { u.Name = name }
func (u *User) GetName() string { return u.Name }

// OK: small immutable type with value receivers
type Point struct{ X, Y int }
func (p Point) Add(q Point) Point { return Point{p.X + q.X, p.Y + q.Y} }

Context as First Parameter

Context should be the first parameter in functions.

Review Checklist

•Issue if context is not the first parameter
•Issue if context is stored in a struct (use parameter instead)

// Issue: context not first
func GetUser(id string, ctx context.Context) (*User, error) { ... }

// Correct: context first
func GetUser(ctx context.Context, id string) (*User, error) { ... }

6. Concurrency

Goroutine Lifetimes

Document goroutine lifetimes, especially in concurrent code.

Review Checklist

•Issue if goroutines are launched without clear termination
•Issue if no comment explains when goroutine exits
•Issue if goroutine might leak

// Issue: unclear goroutine lifetime
func StartProcessor() {
    go process()  // When does this stop?
}

// Correct: documented lifetime and cleanup
// StartProcessor launches a background goroutine that processes
// items until ctx is canceled.
func StartProcessor(ctx context.Context) {
    go func() {
        for {
            select {
            case <-ctx.Done():
                return  // Goroutine exits when context canceled
            case item := <-queue:
                process(item)
            }
        }
    }()
}

Synchronous Functions

Prefer synchronous functions; let caller decide on concurrency.

Review Checklist

•Issue if function launches goroutine internally without clear need
•OK for goroutines in long-running services or servers

// Issue: forces async on caller
func ProcessData(data []byte) {
    go func() {
        // process data
    }()
}

// Correct: synchronous, caller controls concurrency
func ProcessData(data []byte) error {
    // process data synchronously
    return nil
}

// Caller can make it async if needed:
go func() {
    if err := ProcessData(data); err != nil {
        log.Println(err)
    }
}()

7. Memory & Performance

Pass Values vs Pointers

For small structs, consider passing by value.

Review Checklist

•Issue if large structs (many fields) passed by value
•OK for small structs (2-3 primitive fields) to be passed by value
•OK for time.Time, bytes.Buffer to be passed by value

Note: Some projects use pointer-first approaches. This is standard Go guidance for optimal performance.

// Small immutable struct - value is fine
type Point struct {
    X, Y int
}

func Distance(p1, p2 Point) float64 {
    dx := p1.X - p2.X
    dy := p1.Y - p2.Y
    return math.Sqrt(float64(dx*dx + dy*dy))
}

// Large struct or mutable - use pointer
type User struct {
    ID        string
    Email     string
    Name      string
    CreatedAt time.Time
    // ... many more fields
}

func UpdateUser(u *User) error { ... }

Copying

Be careful when copying structs to avoid unexpected aliasing.

Review Checklist

•Issue if copying a struct whose methods use pointer receivers
•Issue if copying a struct containing a mutex, channel, or other sync primitives
•Issue if copying causes unintended sharing of pointer fields
•OK for small value types designed to be copied

// Issue: copying struct with pointer receiver methods
type Counter struct {
    count int
}

func (c *Counter) Increment() { c.count++ }
func (c *Counter) Value() int { return c.count }

func main() {
    c1 := Counter{}
    c2 := c1           // Copied! c2 is independent
    c1.Increment()
    fmt.Println(c2.Value())  // Prints 0, not 1 - may be unexpected
}

// Issue: copying struct with mutex
type SafeMap struct {
    mu sync.Mutex
    m  map[string]int
}

func main() {
    sm1 := SafeMap{m: make(map[string]int)}
    sm2 := sm1  // BUG: mutex copied, map shared!
}

// Issue: copying struct with pointer field causes aliasing
type User struct {
    Name    string
    Friends []*User  // Pointer slice
}

func main() {
    u1 := User{Name: "Alice", Friends: []*User{...}}
    u2 := u1  // u2.Friends points to same slice!
    u2.Friends[0].Name = "Bob"  // Modifies u1's friend too!
}

// Correct: if you need to copy, implement Clone()
func (u *User) Clone() *User {
    clone := *u
    clone.Friends = make([]*User, len(u.Friends))
    copy(clone.Friends, u.Friends)
    return &clone
}

Rule of thumb: If a type's methods have pointer receivers, don't copy values of that type.

Declaring Empty Slices

Use var s []T for empty slices, not s := []T{}.

Review Checklist

•Issue if empty slice declared with make([]T, 0) or []T{}
•OK for var s []T (nil slice)

// Issue: unnecessary allocation
s := []string{}
s := make([]string, 0)

// Correct: nil slice (preferred)
var s []string

// Also OK when size is known
s := make([]string, 0, 10)  // Preallocate capacity

Note: nil slices and empty slices behave identically for append, len, and range.

8. Security & Testing

Crypto Rand

Use crypto/rand for keys and secrets, not math/rand.

Review Checklist

•Issue if math/rand used for cryptographic purposes
•Issue if math/rand used for tokens, session IDs, or keys

// Issue: insecure for tokens
import "math/rand"
token := rand.Int63()

// Correct: crypto/rand for security
import "crypto/rand"
b := make([]byte, 32)
if _, err := rand.Read(b); err != nil {
    return err
}
token := base64.URLEncoding.EncodeToString(b)

Examples

Use testable examples in _test.go files.

Review Checklist

•Issue if examples don't follow Example* naming convention
•Issue if examples lack // Output: comment
•OK for examples without output if they demonstrate usage

// Correct: testable example
func ExampleUser_GetFullName() {
    u := &User{FirstName: "John", LastName: "Doe"}
    fmt.Println(u.GetFullName())
    // Output: John Doe
}

Useful Test Failures

Test failures should clearly indicate what went wrong.

Review Checklist

•Issue if test errors lack context (which test, what input, what expected)
•Issue if using t.Fatal when t.Error would suffice

// Issue: uninformative failure
if got != want {
    t.Fatal("wrong result")
}

// Correct: clear failure message
if got != want {
    t.Errorf("GetName(%q) = %q; want %q", input, got, want)
}

9. Naming Conventions

Initialisms

Use correct casing for initialisms (URL, ID, HTTP, etc.).

Review Checklist

•Issue if initialisms use wrong casing: Url, Id, Http, Json
•Correct: URL, ID, HTTP, JSON (all caps or all lowercase)

// Issue: wrong casing
type User struct {
    UserId   string
    ImageUrl string
    HttpCode int
}

// Correct: proper initialisms
type User struct {
    UserID   string  // ID not Id
    ImageURL string  // URL not Url
    HTTPCode int     // HTTP not Http
}

// Also correct when not exported
userID := "123"
imageURL := "https://..."

Common Initialisms

Wrong	Correct
`Id`	`ID`
`Url`	`URL`
`Http`	`HTTP`
`Json`	`JSON`
`Xml`	`XML`
`Html`	`HTML`
`Api`	`API`
`Db`	`DB`
`Sql`	`SQL`
`Ui`	`UI`

Package Names

Package names should be short, lowercase, no underscores or mixedCaps.

Review Checklist

•Issue if package names use underscores or mixed caps
•Issue if package name is plural (prefer singular)
•Issue if package name is overly long

// Issue: wrong package names
package user_service
package UserService
package users

// Correct: short, lowercase, singular
package user
package service
package http

Variable Names

Use short names for short scope, descriptive names for longer scope.

Review Checklist

•Issue if loop variables use long names (index, element)
•Issue if package-level variables use single-letter names
•OK for i, j, k in loops
•OK for short receiver names

// Issue: overly verbose loop vars
for index, element := range items {
    fmt.Println(index, element)
}

// Correct: short names for short scope
for i, item := range items {
    fmt.Println(i, item)
}

// Issue: single letter for package var
var u *User  // Package level - too short

// Correct: descriptive for longer scope
var DefaultUser *User

Output Format

When reviewing Go code, report findings as:

code

**Go Style Review**

Issues found:
- `user.go:12` - Function `GetUser` missing doc comment
- `user.go:23` - Error string "Failed to connect" should be lowercase: "failed to connect"
- `user.go:45` - Receiver name "this" should be short (e.g., "u")
- `user.go:67` - Initialism "UserId" should be "UserID"
- `auth.go:34` - Imports not grouped (stdlib, third-party, local)
- `handler.go:89` - Error discarded with `_ = file.Close()` without comment

Recommendations:
- Add doc comments to all exported functions
- Use consistent receiver names across User methods
- Group imports: stdlib, third-party, then local

No issues:
- All code is gofmt-formatted
- Error handling uses early returns
- Context passed as first parameter

When to Run

This skill auto-invokes when reviewing all Go files (.go extension).

For more focused reviews:

•DDD architecture: Use go-review-ddd skill
•Repository patterns: Use go-repository skill
•Gin framework: Use gin-go skill