Refactoring a Function in Go

The following is a walk-through of how I refactored a function.

Recently, I needed to refactor a function that looked like

func soSomething(r *foo.Foo, a string, b string, c string) {
    err := r.Bar(a)
    if err != nil {
        panic(err.Error())
    }
    err = r.Bar(b)
    if err != nil {
        panic(err.Error())
    }
    err = r.Bar(c)
    if err != nil {
        panic(err.Error())
    }
}

In this function is only called in a few places. The Bar function is performing some computation on the strings and storing the results in an array in r. It will only return an error if the string passed to Bar had been previously passed to Bar. This is documented above the Bar function.

Some housekeeping: the Bar is only ever called in this function. doSomething is never called on the same instance of foo.Foo.

First, we can get closer to idiomatic Go by inlining some of the error handling.

func doSomething(r *foo.Foo, a string, b string, c string) {
    if err := r.Bar(a); err != nil {
        panic(err.Error())
    }
    if err := r.Bar(b); err != nil {
        panic(err.Error())
    }
    if err := r.Bar(c); err != nil {
        panic(err.Error())
    }
}

But what about these panics? Since doSomething is only called in a few places and we know a, b, and c are always different, the panics are designed to terminate the application if there is corruption. If Bar ever returns an error, then one of a few issues is going one:

Bar returns an error even if a, b, and c are unique.
The machine is corrupt. If the CPU corrupts the Bar computation, or RAM is not correctly storing values, then Bar could return an error. In that case, Bar is acting as a canary for a bad machine.
a, b, and c are not unique.

In case 1, the in line documentation for Bar is incorrect. This is totally possible, any if that were the case, we should open a bug with the foo library or submit a patch to edit the documentation. At a higher policy level, it may be a good idea to consider using the another library. However, I wrote the library and I pinky promise the documentation and implementation is correct. Case 1 should never occur.

If we could be sure that the error is a result of case 3, the program should definitely panic and report the error in the panic message. If the program were to leave some intermediate state in some other system, e.g. a database, then the program may want to clean up that state first. However, if you know the machine to be corrupt, you may want to panic, and leave the cleanup for another instance of the program running on another machine. However, we should not treat this function as a corruption detector, and therefore we should not panic. Checkout When to Panic

Case 3 should never occur either. There are only a few places where doSomething is called, and all of the arguments passed to it are unique string literals. We can slightly improve the implementation by adding a comment that the function will panic if the arguments are not unique.

// doSomething will panic if a, b, or c are not unique.
// It may panic if doSomething is called more than once on r.
func doSomething(r *foo.Foo, a string, b string, c string) {
    // ...
}

This comment may be discouraged in idiomatic Go since this is a private function, and it is only called in a few places. However, this module is pretty large, and the implementation of this function is pretty far from some if its uses.

So are we done with refactoring? No. The panic calls cannot stay. We are essentially using them as asserts about the state of r. However, we need to follow our policy of only ever panicking from the main function. (Also, we do not need to panic in main since we can just return early.) Therefore we need to return an error.

func doSomething(r *foo.Foo, a string, b string, c string) error {
    if err := r.Bar(a); err != nil {
        return err
    }
    if err := r.Bar(b); err != nil {
        return err
    }
    return r.Bar(c)
}

It is then the responsibility of the callers to doSomething to clean up and return early with an error if they get an error from doSomething. However, some of the callers of doSomething already return errors themselves, we may want some way of noting that this is a fatalError and that the computation should not continue a. la.

type fatalError {
    err error
}

func (err *fatalError) Error() string {
    return err.Error()
}

func doSomething(r *foo.Foo, a string, b string, c string) error {
    if err := r.Bar(a); err != nil {
        return fatalError{err: err}
    }
    // ...
}

However, this is not great design. Since doSomething would only ever return a fatalError or nil, wrapping the error in a fatalError does not add any information. If the callers of doSomething already return errors, then perhaps adding a wrapper there would be useful, but the implementation of doSomething is already finished.