Questions about "if err != nil"- Critique of new version requested

Good work in reducing the code.

I had some time to look at your code and to be honest I don’t understand the goal of the methodWrapper.

If you are trying to reduce the number of times you write if err != nil, then I have to agree with @calmh on this one.

The pattern you have implemented seems a bit much and as @dfc pointed out doesn’t fit the pattern you mentioned in your initial question.

Errors are values in Go and should be handled, not deferred, to ensure the integrity of the code. Yes, there are cases where deferring makes sense but this is not that case.

With that being said, here are my initial thoughts on the code as it stands.

The use of a custom error-like type for formatting is more than necessary. Especially, as a pointer - I know, I suggested it but I didn’t really understand your intent.

The use of anonymous functions, while flexible, introduces another level of complexity which is harder to read than the standard if err check. As you look through this code ask yourself: do I really need to wrap these statements and are type assertions really needed.

Lastly, at no point do you exit if any call to Do returns an error. Nor do you have any code in place to recover from that said error. Which means you are passing around invalid data and running a bunch of noops for no reason. For example, the call to get a users configuration file should never run if user.Current() returns with an error. There are some good posts on error handling and code correctness. Maybe @dfc or @goinggodotnet can provide insight.

Here is some sample code, using a pattern for handling errors that I’ve come across and used in the past. For the record, the intent was to terminate immediately after printing each error.

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"`
}

func terminateOnError(err error) {
	if err != nil {
		fmt.Printf("%+v", errors.WithStack(err))
		os.Exit(1) // or anything else ...
	}
}

func main() {

	// get the current os user
	usr, err := user.Current()
	terminateOnError(err)

	// get the current users's configuration path for the gotwitter application
	config := path.Join(usr.HomeDir, ".gotwitter/config.json")
	_, err := os.Stat(config)
	terminateOnError(err)

	// open a file based on the specified config path
	file, err := os.Open(config)
	terminateOnErr(err)
	defer file.Close()

	// read the config json file into the Credentials struct
	decoder := json.NewDecoder(file)
	err = decoder.Decode(&creds)
	terminateOnErr(err)

	// get TwitterAPI based on stored credentials
	var api *anaconda.TwitterApi
	anaconda.SetConsumerKey(creds.ConsumerKey)
	anaconda.SetConsumerSecret(creds.ConsumerSecret)
	api = anaconda.NewTwitterApi(creds.AccessToken, creds.AccessSecret)
	// 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
	tweets, err := api.GetSearch("golang", nil)
	terminateOnErr(err)

	for _, tweet := range tweets.Statuses {
		fmt.Println(tweet.Text)
		fmt.Println("")
	}
}

Please note that this sample code is untested and is a copy/paste of your code, with some tweaks, so there might be typos or errors. But it should be enough to get the gist.

I hope the comments shed a little light and help you get to your end goal.

Hey Wilken, thanks so much for your efforts with this. To be honest I had considered a simple error checking function before but I think I had read that this would upset the stack handling. The difference here of course is that that is taken care of by the pkg/errors package.

I like the simplicity of your suggested code (I haven’t tried it yet, but I’m sure it’s near enough ok!) - I’ve no wish to write “more complex than thou” code.

Thanks again,

Cheers,
Carl

Hello again Wilken, as it turned out, I did have to tweak the code. Largely because using := in the error assignments was causing build errors. Also, I had to reintroduce the var statements for the various variables, again because of problems related to the := operator’. See what you think:

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"`
}

func terminateOnError(err error) {
	if err != nil {
		fmt.Printf("%+v", errors.WithStack(err))
		os.Exit(1) // or anything else ...
	}
}

func main() {

	var err error

	// get the current os user
	usr, err := user.Current()
	terminateOnError(err)

	// get the current users's configuration path for the gotwitter application

	var config string
	config = path.Join(usr.HomeDir, ".gotwitter/config.json")
	_, err = os.Stat(config)
	terminateOnError(err)

	// open a file based on the specified config path
	var file *os.File
	file, err = os.Open(config)
	terminateOnError(err)
	defer file.Close()

	// read the config json file into the Credentials struct
	var creds *Credentials
	decoder := json.NewDecoder(file)
	err = decoder.Decode(&creds)
	terminateOnError(err)

	// get TwitterAPI based on stored credentials
	var api *anaconda.TwitterApi
	anaconda.SetConsumerKey(creds.ConsumerKey)
	anaconda.SetConsumerSecret(creds.ConsumerSecret)
	api = anaconda.NewTwitterApi(creds.AccessToken, creds.AccessSecret)
	// 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
	tweets, err := api.GetSearch("golang", nil)
	terminateOnError(err)

	for _, tweet := range tweets.Statuses {
		fmt.Println(tweet.Text)
		fmt.Println("")
	}
}

In my opinion, the very first version of your code is the simplest and most readable. When you come back to that code, those features will make it easier to make changes.

The only thing you need to do is to handle the errors explicitly instead of using reportError. For example:

credFile := path.Join(usr.HomeDir, ".gotwitter/config.json")
file, err := os.Open(credFile)
if err != nil {
	log.Printf("unable to open config file: %v", err)
    return
}

A good and descriptive error message, makes it very obvious what the error is and where it has occurred.

If you want to avoid the extra returns, you can always use a different function to act as your main that returns an error. For example:

func main() {
    if err := run(); err != nil {
    	fmt.Fprintf(os.Stderr, "Error: %v\n", err) // or log.Printf
    	os.Exit(1)
    }
}

func run() error {
    credFile := path.Join(usr.HomeDir, ".gotwitter/config.json")
    file, err := os.Open(credFile)
    if err != nil {
        return fmt.Errorf("unable to open config file: %v", err)
    }
    // ...
}

This will allow you to use your main function the same way you would use the rest of your functions and return errors that contain descriptive messages.

You may also use a helper function to group together related operations. For example, I would put user.Current(), os.Open and decoder.Decode in a new function because they all have to do with reading the application’s configuration. This would simplify your main much more as it’s basically three operations:

  1. Read configuration
  2. Construct Twitter client
  3. Do Twitter request.

If reading the app configuration becomes more complex, you could even write a package for that like showcased in the Upspin project.

1 Like

I updated https://pocketgophers.com/error-handling-packages/ with 10 more packages.

Aaaarrrgh! More choice! Head explodes

2 Likes

Here are some defaults you can use to glue your head back together:

  1. Embrace if err != nil. See Why You Should Not Use checkErr.
  2. Bubble errors up when they can’t immediately be handled, but don’t use naked returns
  3. Add context to errors with github.com/pkg/errors
  4. Deal with multiple errors (e.g., when Handling Errors in defer) with github.com/hashicorp/go-multierror
2 Likes

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.