Go-tools: simple or quickfix: suggest removing unnecessary else after previous block returns

[alexchao26]

Similar to #187

Suggestion to remove unnecessary else/else-if blocks when preceded by a return in the previous block. Helps with minimizing indentation of the happy path. Particularly useful when handling errors:

Example:

func foo(r io.Reader) (string, error) {
    b, err := ioutil.ReadAll(r)
    if err != nil {
        return "", err
    } else {
        return string(b), nil
    }
}

Prefer:

func foo(r io.Reader) (string, error) {
    b, err := ioutil.ReadAll(r)
    if err != nil {
        return "", err
    }
    return string(b), nil
}

Also mentioned here: https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

[mrkanister]

I remember that one of the tools that the Go plugin for Visual Studio Code used (a while ago before the gopls era) would do exactly that kind of check and suggest to remove the else part. There were some false positives when a variable was initialized as part of the if statement that was later also used in the else part, but I am not sure if that was due to a limitation in the analysis library or just an edge case that wasn't explicitly handled.

// The linter (?) would suggest to remove and "out-dent" the else-part,
// but 'value' is scope to the if-block and doesn't exist outside of it.
if value, ok := someMap["key"]; !ok {
    return "fallback"
} else {
    return value
}

I want to get your opinion on a different example, though. I agree that minimizing the indentation of the happy-path improves the readability of the code, but what if there is no happy-path? In this example, would you say that x being greater than y is less of a happy-path than it being smaller?

func Max(x, y int) int {
    if x > y {
        return x
    } else {
        return y
    }
}

The both feel equally "happy" to me and I think I would be surprised if staticcheck flagged this particular case. Maybe the if-block has to contain a comparison with a value of type error, too, in order for this check to trigger?

Just some food for thought.

[dominikh]

In this example, would you say that x being greater than y is less of a happy-path than it being smaller?

People have gotten very eager at using early returns, probably too eager. Your example is a good minimal example of the explicit else branch preserving more of the structure of the program, making its control flow obvious via indentation.

Early returns make sense when you have a happy path and one to many ways the happy path can fail, taking you to the function's exit, to reduce the overall amount of nesting.

In my opinion, explicit if+else is usually the more appropriate choice when you have two paths that are equal in their importance.

(Edit: I realize I've basically repeated your exact opinion in my own words… so in short, I agree with you.)

[dominikh]

The Forgotten Art of Structured Programming - Kevlin Henney [C++ on Sea 2019] touches on this a bit

[alexchao26]

In my opinion, explicit if+else is usually the more appropriate choice when you have two paths that are equal in their importance.

This is a great way to put it and in that context I agree, there are "equally happy paths."

Personally, I like this idea because it seems like a common case:

Maybe the if-block has to contain a comparison with a value of type error, too, in order for this check to trigger?

but I'm also happy to close the Issue if this check is considered too opinionated in most cases.

[dominikh]

but I'm also happy to close the Issue if this check is considered too opinionated in most cases.

Let's leave it open for now and see if we can implement the check without false positives (or few enough false positives to move it into an in-editor optional refactoring)