Common Golang Code Review Comments

We use linters at work all the time. As good as they are, they do not catch everything. Here are some common code review comments I have seen that linters miss.

This is not meant to be a comprehensive list. This is simply to provide answers for when these comments occur. I will update this document when needed.

Inline error checking

In order to limit the scope of variables, prefer to inline error checking.

var person Person
if err := json.Unmarshal(personJSON, &person); err != nil {
   logger.WithError(err).Error("Could not parse the person JSON from the people service")
}

If a function returns multiple values, do not inline in order to reduce scope. The reduces nesting.

personID, err := uuid.NewRandom()
if err != nil {
  logger.WithError(err).Error("Unable to generate a new id for a person")
}

Use var when declaring zero value structs

In order to make declaration of zero value structs obvious, use var.

var person Person

// NOT
person := Person{}

Use nil-slices

Use nil slices as opposed to empty slices.

var people []Person

// NOT
people := []Person{}

This prevents preallocation of the underlying array. You can read about why this is preferred in the blog post The Difference between Nil and Empty Slices in Go.

Preallocate slices and maps when possible

If you know how large a map or slice will be, preallocate the memory using capacity hints.

people := make([]People, len(peopleFiles))

peopleMap := make(map[string]People, len(peopleFiles))

Refer to the Uber Style Guide section on Specifying Container Capacity for more details.

Test tables

Ideally, a unit-test should be a simple list of steps. Abstractions, loops, and if statements should be avoided. This makes test code more readible and maintainable.

host, port, err := net.SplitHostPort("192.0.2.0:8000")
require.NoError(t, err)
assert.Equal(t, "192.0.2.0", host)
assert.Equal(t, "8000", port)

One exception to this rule is when you want to make the same set of assertions given multiple inputs.

host, port, err := net.SplitHostPort("192.0.2.0:8000")
require.NoError(t, err)
assert.Equal(t, "192.0.2.0", host)
assert.Equal(t, "8000", port)

host, port, err = net.SplitHostPort("192.0.2.0:http")
require.NoError(t, err)
assert.Equal(t, "192.0.2.0", host)
assert.Equal(t, "http", port)

host, port, err = net.SplitHostPort(":8000")
require.NoError(t, err)
assert.Equal(t, "", host)
assert.Equal(t, "8000", port)

host, port, err = net.SplitHostPort("1:8")
require.NoError(t, err)
assert.Equal(t, "1", host)
assert.Equal(t, "8", port)

In this case, the practice of test tables are encouraged. Follow the Test Tables pattern established by the Uber style guide.

Try to air on the side of simplicity. Follow their guidelines for avoiding complexity in table tests.

Initialisms

Words such as URL and ID should have the same casing pattern. For instance, the variable name URLPatterns should be URLPatterns when exported and urlPatterns when private. UrlPatterns is incorrect.

See the Go Style guide initialisms section for more details.

Return nil or false for non-existent values

For data fetchers, avoid returning a special "Not Found" error value that a caller needs to switch on. It is more ergonomic to return nil or false.

Fetchers that fetch into a pointer

For functions that fetch data and update a pointer, return false in position #0 when the data is not found.

// FetchPerson fetches a person into the value referenced by `into`
// Return values:
//
// happy path  `(true, nil)`
// error case  `(_, not-nil)`  Callers should ignore position #0
// not found   `(false, nil)`
func FetchPerson(id uuid.UUID, into *Person) (bool, error)

Fetcher that return a pointer

For functions that return a pointer to a the fetched values, return nil in position #0 when the data is not found.

// FetchPerson fetches a person and returns a pointer to its value
// Return values:
//
// happy path  `(&Person{...}, nil)`
// error case  `(_, not-nil)`         Callers should ignore position #0
// not found   `(nil, nil)`
func FetchPerson(id uuid.UUID) (*Person, error)

Fetchers that return a value

Functions that return a value often return a zero value that is expensive to check if it is empty. In this case, follow the pattern established by the map API and return an "ok" boolean.

// FetchPerson fetches a person and returns its value with an "ok" boolean
// Return values:
//
// happy path  `(Person{...}, true, nil)`
// error case  `(_, _, not-nil)`           Callers should ignore positions #0 and #1
// not found   `(_, false, nil)`           Callers should ignore position #0
func FetchPerson(id uuid.UUID) (Person, bool, error)