[Golang] code review - error handling

Error handling is one of the most controversial features of Go. This blog will not discuss the merits of the design.

This blog discusses what I look for when I review Go code with respect to error handling and what change would I suggest to the author.

The "just return error"

DON'T just pass an error along - handle it appropriately

Consider this code below

func validate(s string) error {
        err := isValid(s)
        if err != nil {
               return err
        }
        return nil
    }

This code might seem fine at first - if there’s an error, return the error. What’s so bad about it?

The fundamental problem with this code is that there is no way to tell where the error is coming from. If every piece of code in a system just does this, eventually someone will run into a log line sql: error connection closed and has no idea where that's coming from. When this happens, somebody will be stuck in a long and tedious session tracing down code looking for where might sql errors be coming from.

Another common thing is developers adding logging into error check but still returning an unannotated error

func validate(productName string) error {
    err := isValid(productName)
    if err != nil {
           logger.Errorf("product name is not valid: %v", err)
           return err
    }
    return nil
}

In my opinion, this is worse. Not only does it not communicate the issue clearly up to the caller, it is also writing logs (which is something its caller probably also does), making logs noisier.

The better, and more responsible, thing to do is to ALWAYS add context to the error before returning

func validate(productName string) error {
    err := isValid(productName)
    if err != nil {
           return fmt.Errorf("product name is not valid: %w", err)
    }
    return nil
}

Now things are much clearer to the caller and when the errors inevitably show up in the logs. Wrapping errors properly allows the caller to take appropriate actions whether it is to log the error or add their own context and pass it along. Enforcing this rule in the code base will make chasing down issues much easier when these errors inevitable show up in the logs.

product name is not valid: sql: error connection closed - a much more searchable error which expresses exactly where things go wrong.

---

Using == to compare errors

There are times where checking for specific kind of errors are necessary. For example, when handling SQL transaction

err := tx.Rollback()
if err != nil {
    if err != sql.ErrTxDone {
        // Unexpected error
    }
    // Tx already finished - moving on...
}

This method of comparing error types is not compatible with adding context to errors mentioned above. Instead, use the errors package that recursively unwraps errors to compare for target error

err := fmt.Errorf("failed some important transaction: %w", sql.ErrTxDone)

err == sql.ErrTxDone // false

errors.Is(err, sql.ErrTxDone) // true

This advice goes hand-in-hand with the earlier advice about wrapping your error. If == is used to compare errors, wrapping errors becomes a breaking change; therefore, ALWAYS flag down if err == <someErrType> and convert that to errors.Is(err, <someErrType>

---

Use a linter

I recommend using golangci-lint. Visit the official documentation HERE for more details.

There are issues that are not obvious to human reviewers. This section discusses some examples of those issues that I rely on using a linter to find and fix them.

Ignore errors implicitly

Sometimes it is difficult to tell during a code review if a method returns an error, or not. Consider this code

func thing() {
    ...
    f.Close()
    ...
}

When that shows up in a diff, without knowting exactly what type f is and the signature of Close(), it is impossible to tell if an error is being ignored. A linter can flag any and all ignored errors down and fails the build - forcing the code to be changed to

func thing() {
    ...
    _ = f.Close()
    ...
}

It might seem inconsequential but this communicates to the readers/reviewers that the author of this code has made a conscious choice to ignore this error. It may or may not matter but in my experience, forcing errors to be ignored explicitly pulls more attention to the decisions and generates great conversations around it.

Assigning errors variable without checking them

This issue gets flagged by golangci-lint as ineffectual assignment. This issues are something I run into mostly in test code. Although this issue is not specific to errors, since it is pretty normal in Go code to shadow the err variable, it ends up becoming a common mistake to make.

sut, err := setupTest()
result, err := sut.DoThing()
if err != nil {
    t.Errorf("fail to do thing: %v", err)
}

As a reviewer of the code above, there is no way to tell if the author intended to ignore the err returned from setupTest() or just simply made a mistake. Use golangci-lint to catch and report this issue and either ignore the error explicitly, or checking the returned error.