When you start reviewing code for other Go developers, you'll notice that "working code" isn't the goal—"idiomatic code" is. Go is a language of opinions. It prizes clarity over cleverness and simplicity over abstraction. If you're coming from Java or Python, your instinct will be to build hierarchies and complex abstractions. In Go, that's usually a red flag.

Let's look at this through a real-world scenario. Imagine a teammate submitted a PR for a simple UserRepository. It works, it passes the tests, but it feels... heavy. Let's walk through how I'd review this and refactor it into something that looks like it was written by a seasoned Gopher.

// The original PR submission
type User struct {
    ID   int
    Name string
}

type UserStore interface {
    FetchUser(id int) (*User, error)
}

type MySQLStore struct {
    connection string
}

func (m *MySQLStore) FetchUser(id int) (*User, error) {
    if id != 0 {
        user, err := m.doLookup(id)
        if err == nil {
            return user, nil
        } else {
            return nil, err
        }
    } else {
        return nil, errors.New("id cannot be zero")
    }
}

Flattening the Happy Path

The first thing that jumps out at me is the "Pyramid of Doom" in FetchUser. The logic is nested inside if/else blocks, pushing the "happy path" (the successful result) deep into the right side of the screen. In Go, we do the opposite: we handle errors first and return early. This keeps the successful flow aligned to the left margin.

func (m *MySQLStore) FetchUser(id int) (*User, error) {
    if id == 0 {
        return nil, errors.New("id cannot be zero")
    }

    return m.doLookup(id) // No need for the extra if/else wrapper
}

Avoiding Interface Pollution

Now look at the UserStore interface. A common mistake for newcomers is to define an interface before they have more than one implementation. This is "Interface Pollution." In Go, interfaces are satisfied implicitly. You don't need to declare that MySQLStore implements UserStore; it just does.

My rule of thumb during reviews: Accept interfaces, return structs. The MySQLStore should be returned as a concrete pointer, and the consumer of the store should define the interface they actually need. I'd tell the developer to delete the UserStore interface entirely unless we're already implementing a PostgresStore or a MockStore.

Wait, I messed up the slice

Let's add a method to fetch multiple users. I'll write this quickly to show you a mistake I actually made in a PR last week. I wanted to pre-allocate a slice for performance because I knew I was expecting about 10 users.

func (m *MySQLStore) FetchActiveUsers() ([]*User, error) {
    users := make([]*User, 10) // My mistake here
    
    // Imagine a loop that appends results
    for i := 0; i < 10; i++ {
        u, _ := m.doLookup(i)
        users = append(users, u)
    }
    return users, nil
}

I caught this during my own self-review. By calling make([]*User, 10), I created a slice that already contains 10 nil elements. When I call append, Go adds the new users after those 10 nils. The resulting slice would have a length of 20. To fix this, I need to set the length to 0 and the capacity to 10.

// The correction
users := make([]*User, 0, 10) 

Cleaning up the Nomenclature

Finally, I'd look at the naming. Go prefers short, punchy names when the context is clear. FetchUser is okay, but inside a UserRepository, Get is often sufficient. Also, if a variable's type is User, naming the variable userObj or userStruct is redundant. Just use u or user.

Here is the final, idiomatic version of our little service:

type User struct {
    ID   int
    Name string
}

type MySQLStore struct {
    conn string
}

func NewMySQLStore(conn string) *MySQLStore {
    return &MySQLStore{conn: conn}
}

func (s *MySQLStore) Get(id int) (*User, error) {
    if id == 0 {
        return nil, errors.New("id cannot be zero")
    }
    return s.doLookup(id)
}

📋 Practical Task

Refactoring the Legacy Order Processor

You are reviewing a PR for an order processing system. The current code is functional but non-idiomatic. Refactor the following ProcessOrder function to follow Go idioms:

1. Eliminate the nested "Pyramid of Doom" by using early returns.
2. Remove redundant else blocks.
3. Fix the slice initialization mistake (it currently creates unnecessary empty elements).
4. Simplify the naming.

func ProcessOrder(orderID int, items []string) ([]string, error) {
    var processedItems []string
    if orderID != 0 {
        if len(items) > 0 {
            // Mistake: This pre-allocates length instead of capacity
            processedItems = make([]string, len(items)) 
            
            for _, item := range items {
                upperItem := strings.ToUpper(item)
                processedItems = append(processedItems, upperItem)
            }
            return processedItems, nil
        } else {
            return nil, errors.New("no items provided")
        }
    } else {
        return nil, errors.New("invalid order id")
    }
}