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.
If a function returns multiple values, do not inline in order to reduce scope. The reduces nesting.
Use var when declaring zero value structs
In order to make declaration of zero value structs obvious, use
Use nil slices as opposed to empty slices.
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.
Refer to the Uber Style Guide section on Specifying Container Capacity for more details.
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.
One exception to this rule is when you want to make the same set of assertions given multiple inputs.
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.
Words such as
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.
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.
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.
- 2024-02-27: Add "Return nil or false for non-existent values"
These do not come up often in code reviews, but I wanted to make note of them:
- Avoid Naked Parameters
- Start Enums at One
- Use time
- Time with external systems
- Exit in main and Exit Once
- Reduce Nesting and Unnecessary Else