Hello, I’ve been reading up on the vexed topic of error handling and how to make it as streamlined as possible. In particular, I’ve been trying to get my head around this page https://blog.golang.org/errors-are-values where Dave Cheney talks about how to do this. The piece of code I’ve just started is a prime candidate for tidying up. At the moment it looks like this:
package main
import (
"encoding/json"
"fmt"
"os"
"os/user"
"path"
"github.com/ChimeraCoder/anaconda"
)
// Credentials struct is read from ~/gotwitter/config.json config file
type Credentials struct {
ConsumerKey string `json:"consumerkey"`
ConsumerSecret string `json:"consumersecret"`
AccessToken string `json:"accesstoken"`
AccessSecret string `json:"accesssecret"`
}
func (c *Credentials) String() string {
return fmt.Sprintf("ConsumerKey: '%v'\nConsumerSecret:'%v'\nAccessToken: '%v'\nAccessSecret:'%v'\n",
c.ConsumerKey, c.ConsumerSecret, c.AccessToken, c.AccessSecret)
}
func main() {
// get current user
usr, err := user.Current()
if err != nil {
reportError(err)
}
// get config file location
credFile := path.Join(usr.HomeDir, ".gotwitter/config.json")
file, err := os.Open(credFile)
if err != nil {
reportError(err)
}
// read config.json file
decoder := json.NewDecoder(file)
creds := &Credentials{}
err = decoder.Decode(&creds)
if err != nil {
reportError(err)
}
fmt.Println(creds)
anaconda.SetConsumerKey(creds.ConsumerKey)
anaconda.SetConsumerSecret(creds.ConsumerSecret)
api := anaconda.NewTwitterApi(creds.AccessToken, creds.AccessSecret)
searchResult, _ := api.GetSearch("golang", nil)
for _, tweet := range searchResult.Statuses {
fmt.Println(tweet.Text)
fmt.Println("")
}
}
func reportError(err error) {
fmt.Println("error:", err)
}
For a start, I’m not convinced that my use of a single function reportError is a good idea. More importantly, I cannot figure out how to apply Dave Cheney’s ideas to my code. Even the application of the
if err := scanner.Err(); err != nil {
// process the error
}
idiom doesn’t solve my problems because, as far as I know, that approach cannot be used for functions which return multiple values, such as
I would really like to know how I can cut out a lot of the boilerplate code that I always seem to end up writing. If anyone could advise me I’d be really grateful.
One step I would always recommend no matter what is to use the pkg/errors package. Why?
The point is,
if err != nil {
return err
}
is as meaningless as
catch(e) {
throw(e)
}
in other languages. If you handle errors that way, you neglect the importance of an error as legitimate part of the program flow.
pkg/errors allows annotating an error with a meaningful message that matches the level of the function that handles the error, plus it passes on all lower-level error messages in case you want to see them at the top level of your program, or in the debug log file.
Step 2 - if you really really want to hide the error handling, consider this pattern:
struct myStruct {
// other fields
Err error
}
func (m *myStruct) DoSomething(...) {
If m.Err != nil {
return // Do nothing if a previous method call has caused an error
}
// function body
if a != b {
m.Err = errors.New(...)
return
}
}
// repeat this with other methods of myStruct
Then you can call the methods of myStruct like this:
m := &myStruct{...}
m.DoSomething()
m.DoSomethingElse()
m.DoSomeMore()
if m.Err != nil {
// handle error
}
If any of the method calls errors out, subsequent method calls do nothing, and the error “falls through” the whole method call sequence until handled by the standard if err != nil idiom.
The clear downside of this pattern is that it decouples error handling from the source of the error, which makes error handling (a) less transparent and (b) more complicated, as the single error handling code at the end must handle all possible errors that can occur within any of the methods called previously.
(Disclaimer: The above code is completely untested and shall only illustrate the concept.)
Edit: Typo
Thanks for the reply Christoph. Have you used any of the other error handling packages described on this page https://pocketgophers.com/error-handling-packages/ ? Or would you recommend keeping it simple and sticking to pkg/errors ?
Use the packages that make your code and life better.
While pkg/errors is nice, it doesn’t do everything you might need. For example, it does not consider the case where you might need to return multiple, related errors such as when using defer.
I wrote the descriptions so you could get a pretty good idea if the package solves a problem you are having. If a package doesn’t help in some way, don’t use it. If a package looks like it will help, figure out if it will actually help before using it in your production code. One way of going about this is exploring alternatives with go run.
Hi, Dave, I must apologise for my mistake in attributing the article in question to you rather than Rob Pike. You were good enough not to upbraid me for my mistake so thanks for that. In my defence, I think I had just read https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully which I am reasonably certain is your work (!) at roughly the same time as Rob’s article.
As penance for my transgression, I will go away and read both articles again, in the hope that some of the wisdom contained therein will permeate my thick skull.
I’ve been working on this code and have tried to incorporate some of the things which have been suggested. I would welcome a critique of this latest version:
Just a small observation, in my opinion is better to keep the things simplest. If you work on big programs maybe is a good ideea to have a complex error handling system. On small programs i guess is not really neccesary to complicate things and make the program unreadable.
I take your point, George. However, my plans for this code are actually ambitious, so I am keen to get some good patterns established early in the proceedings, thus avoiding major painful rewrites later on.
I don’t use any external packages, I just use fmt.Errorf to wrap errors with relevant context. e.g.
if err != nil {
return fmt.Errorf("unable to frob foo with ID %s: %v", thingid, err)
}
For a web application, I have a tiny logging wrapper in which log.Errorf works like fmt.Errorf, but also logs the error at the same time. The wrapper adds the usual timestamp, and also the source code file and line number. So I can return the result of log.Errorf, as it’s an error. That will then bubble up the call stack, and may get similarly logged at higher levels. The result is a sort of selective stack trace.
I do this rather than dumping actual stack traces because I’ve never really found them useful — there’s typically only one line of information in the trace that I actually care about.
OK. a radically revised version here. I’m much happier with this version. The only possible downside has been the use of interface{} to provide a pseudo-generic return value mechanism. See what you think. Comments welcome. Thanks!
package main
import (
"encoding/json"
"fmt"
"os"
"os/user"
"path"
"github.com/ChimeraCoder/anaconda"
"github.com/pkg/errors"
)
// Credentials struct is read from ~/gotwitter/config.json config file
type Credentials struct {
ConsumerKey string `json:"consumerkey"`
ConsumerSecret string `json:"consumersecret"`
AccessToken string `json:"accesstoken"`
AccessSecret string `json:"accesssecret"`
}
// the methodWrapper struct provides pre and post function call wrapping
type methodWrapper struct {
Result interface{}
Err error
}
func (mw *methodWrapper) do(f func() (interface{}, error)) {
if mw.Err != nil {
return
}
result, err := f()
if err != nil {
errors.WithStack(err)
}
mw.Err = err
mw.Result = result
}
func main() {
// construct an instance of methodWrapper
m := &methodWrapper{}
// get the current os user
m.do((func() (interface{}, error) {
result, err := user.Current()
return result, err
}))
// get the current users's configuration path for the gotwitter application
m.do((func() (interface{}, error) {
usr := m.Result.(*user.User)
configPath := path.Join(usr.HomeDir, ".gotwitter/config.json")
_, err := os.Stat(configPath)
return configPath, err
}))
// open a file based on the specified config path
m.do((func() (interface{}, error) {
config := m.Result.(string)
file := &os.File{}
file, err := os.Open(config)
return file, err
}))
// read the config json file into the Credentials struct
m.do((func() (interface{}, error) {
file := m.Result.(*os.File)
decoder := json.NewDecoder(file)
creds := &Credentials{}
err := decoder.Decode(&creds)
return creds, err
}))
// get TwitterAPI based on stored credentials
m.do((func() (interface{}, error) {
creds := m.Result.(*Credentials)
anaconda.SetConsumerKey(creds.ConsumerKey)
anaconda.SetConsumerSecret(creds.ConsumerSecret)
api := anaconda.NewTwitterApi(creds.AccessToken, creds.AccessSecret)
if api == nil {
err := errors.New("Error creating TwitterAPI")
return api, err
}
return api, nil
}))
// search current Twitter timeline for golang content
m.do((func() (interface{}, error) {
api := m.Result.(*anaconda.TwitterApi)
searchResult, err := api.GetSearch("golang", nil)
if err == nil {
for _, tweet := range searchResult.Statuses {
fmt.Println(tweet.Text)
fmt.Println("")
}
}
return api, err
}))
// final check on any errors which may have occurred
if m.Err != nil {
fmt.Printf("%+v", m.Err)
}
}
Glad to hear that you found a viable pattern. Have you considered inverting the dependency on the custom error wrapper type?
Instead of having your methods return a result and an error, maybe pass in a pointer to m and let the funnctions update state when necessary. I think it could reduce some of the complexity and help readability. A sample can be found at the link below.
Hi Wilken, thanks very much for your suggestions. With your suggestion about inverting the dependency on the wrapper, I have managed to reduce the code further, although my solution is a compromise between your approach and my approach. Here is my latest version. It’s much cleaner now, thanks to your help!
package main
import (
"encoding/json"
"fmt"
"os"
"os/user"
"path"
"github.com/ChimeraCoder/anaconda"
"github.com/pkg/errors"
)
// Credentials struct is read from ~/gotwitter/config.json config file
type Credentials struct {
ConsumerKey string `json:"consumerkey"`
ConsumerSecret string `json:"consumersecret"`
AccessToken string `json:"accesstoken"`
AccessSecret string `json:"accesssecret"`
}
// methodWrapper struct provides pre and post function call wrapping
type methodWrapper struct {
Result interface{}
Err error
}
func (m *methodWrapper) do(f func(m *methodWrapper)) {
if m.Err != nil {
return
}
f(m)
if m.Err != nil {
errors.WithStack(m.Err)
}
}
func main() {
// construct an instance of methodWrapper
m := &methodWrapper{}
// get the current os user
m.do((func(m *methodWrapper) {
m.Result, m.Err = user.Current()
}))
// get the current users's configuration path for the gotwitter application
m.do((func(m *methodWrapper) {
usr := m.Result.(*user.User)
m.Result = path.Join(usr.HomeDir, ".gotwitter/config.json")
_, m.Err = os.Stat(m.Result.(string))
}))
// open a file based on the specified config path
m.do((func(m *methodWrapper) {
config := m.Result.(string)
m.Result, m.Err = os.Open(config)
}))
// read the config json file into the Credentials struct
m.do((func(m *methodWrapper) {
file := m.Result.(*os.File)
decoder := json.NewDecoder(file)
defer file.Close()
m.Result = &Credentials{}
m.Err = decoder.Decode(m.Result)
}))
// get TwitterAPI based on stored credentials
m.do((func(m *methodWrapper) {
creds := m.Result.(*Credentials)
anaconda.SetConsumerKey(creds.ConsumerKey)
anaconda.SetConsumerSecret(creds.ConsumerSecret)
m.Result = anaconda.NewTwitterApi(creds.AccessToken, creds.AccessSecret)
m.Err = nil
// I'm thinking of adding a context field to the methodWrapper struct...
// err := errors.New("Error creating TwitterAPI")
}))
// search current Twitter timeline for golang content
m.do((func(m *methodWrapper) {
api := m.Result.(*anaconda.TwitterApi)
var tweets anaconda.SearchResponse
tweets, m.Err = api.GetSearch("golang", nil)
if m.Err == nil {
for _, tweet := range tweets.Statuses {
fmt.Println(tweet.Text)
fmt.Println("")
}
}
}))
// final check on any errors which may have occurred
if m.Err != nil {
fmt.Printf("%+v", m.Err)
}
}
To me this is a large step in the wrong direction. You’ve created your own error handling DSL, which introduces noise and sticks out from the norm. Regular if err != nil checks are visually trivial and reads as second nature after getting use to reading Go code.
Having some type that absorbs error checks, “nops” out subsequent operations and then returns the error at the end is useful for specific, contained scenarios - like the common “binary writer” thing where you do something like
w := newWriter(...)
w.writeInt32(42)
w.writeInt64(7)
...
if err := w.flush(); err != nil {
// handle any error that may have happened along the way
}
or whatever. But not as a wrapper around general code.
I got rid of the Result component of the struct - the use of interface{} was bothering me too much and replaced it with dedicated variables. Despite Jacob’s subjective view that regular if err != nil checks are visually trivial, I find that they are really distracting and that the use of a struct with formerly 2 variables, now just one, doesn’t provide the same level of noise. As for DSL, I assume you mean Domain Specific Language - well that’s just an over the top response, in my humble view! Here’s the latest version:
package main
import (
"encoding/json"
"fmt"
"os"
"os/user"
"path"
"github.com/ChimeraCoder/anaconda"
"github.com/pkg/errors"
)
// Credentials struct is read from ~/gotwitter/config.json config file
type Credentials struct {
ConsumerKey string `json:"consumerkey"`
ConsumerSecret string `json:"consumersecret"`
AccessToken string `json:"accesstoken"`
AccessSecret string `json:"accesssecret"`
}
// methodWrapper struct provides pre and post function call wrapping
type methodWrapper struct {
Err error
}
func (m *methodWrapper) do(f func(m *methodWrapper)) {
if m.Err != nil {
return
}
f(m)
if m.Err != nil {
errors.WithStack(m.Err)
}
}
func main() {
// construct an instance of methodWrapper
m := &methodWrapper{}
// get the current os user
var usr *user.User
m.do((func(m *methodWrapper) {
usr, m.Err = user.Current()
}))
// get the current users's configuration path for the gotwitter application
var config string
m.do((func(m *methodWrapper) {
config = path.Join(usr.HomeDir, ".gotwitter/config.json")
_, m.Err = os.Stat(config)
}))
// open a file based on the specified config path
var file *os.File
m.do((func(m *methodWrapper) {
file, m.Err = os.Open(config)
}))
// read the config json file into the Credentials struct
var creds *Credentials
m.do((func(m *methodWrapper) {
decoder := json.NewDecoder(file)
defer file.Close()
m.Err = decoder.Decode(&creds)
}))
// get TwitterAPI based on stored credentials
var api *anaconda.TwitterApi
m.do((func(m *methodWrapper) {
anaconda.SetConsumerKey(creds.ConsumerKey)
anaconda.SetConsumerSecret(creds.ConsumerSecret)
api = anaconda.NewTwitterApi(creds.AccessToken, creds.AccessSecret)
m.Err = nil
// I'm thinking of adding a context field to the methodWrapper struct...
// err := errors.New("Error creating TwitterAPI")
}))
// search current Twitter timeline for golang content
var tweets anaconda.SearchResponse
m.do((func(m *methodWrapper) {
tweets, m.Err = api.GetSearch("golang", nil)
if m.Err == nil {
for _, tweet := range tweets.Statuses {
fmt.Println(tweet.Text)
fmt.Println("")
}
}
}))
// final check on any errors which may have occurred
if m.Err != nil {
fmt.Printf("%+v", m.Err)
}
}