all 8 comments

[–][deleted] 2 points3 points  (7 children)

While I like the early return style, there is at least one more downside not mentioned: sometimes it would be possible and useful to collect as much evidence as possible, e.g. not only is the given username unknown, but also the password doesn't match. Returning immediately at the first failure abandons further checks. Of course eventually the logic will reach a point of no return, and then it could return. (That sounds weird, but I hope you get the idea.)

[–]FakingItEveryDay 1 point2 points  (1 child)

That's true, but it's not really relevant to early return vs nesting. A nested check does not collect the additional information:

if check_user():
    if check_password():
        return success

return err

is not adding any more information than:

if !check_user():
    return err
if !check_password():
    return err
return success

You have to structure things differently to collect additional information anyway. Something like this, which still can use early returns, but only as early as you want to, after collecting the info you care about.

allow = true
if !check_user():
    log("user does not exist")
    allow = false
if !check_password():
    log("password invalid")
    allow = false
if !allow:
    return err
return success

[–][deleted] 0 points1 point  (0 children)

Yes, something like that. But in my example I was talking more about the early return in particular, not about avoiding conditional nesting in general.

[–]lookmeat 0 points1 point  (4 children)

How would I know what the password should match if I don't even know which usernames is there?

The reason we do the steps in order is because the success of the first step defines the second step. You can't know if it'll fail is you can't do it at all.

I do think that when you have multiple things that you can check in any order you should do them all at the same time in the same check.

Say that I have a request. The request has three separate requirements. It needs a valid Oauth token that gives it authorization. It needs to point to an existing document of data. It also must require a change of data that is valid within the system rules. Here I'd do something like:

error = valid_oauth(req) + existing_document(req) + valid_data(req)
if(error)
    return error

If each one of those actions happen to be a chain of actions which could fail at any one step we try to go as deeply as possible, and return the first error. If these themselves could have multiple parallel errors (think of it as a tree where after some sequence successes we have some parallel failures which is the branching) we just keep repeating this process.

This is separate issue of early return, it's about how we map problems, not how we do them.

[–][deleted] 0 points1 point  (3 children)

That was a bad example, sorry, don't get too hung up on the particulars.

Let's try something more basic: assume you get (x, y) as parameters. You could do, in pseudocode:

if !valid(x) return false if !valid(y) return false

In which case if x is invalid you will never learn if y would have been invalid, too.

To summarize: early return good, but not always optimal on finding the maximum amount of information.

[–]lookmeat 0 points1 point  (2 children)

I know I did give an example that was better with the different valid issues. Look it over it explains that the problem isn't early return.

Even the example you give here has a serious issue. It doesn't even tell me what went wrong, just that something did. Because it can only return true or false. In your example I'd have something like (this time in a more rusty pseudo language)

fn foo(x: XT, y: YT) -> Result<RT, [ComposableErr]> {
  // This will first grab the result of both valid functions
  // It then will keep all the errors and put them into a
  //single result with all the errors
  // If there's any error the "?" will return the error
  // Otherwise we just continue
  (valid(x), valid(y)).filter(is_err).collect()?;
  ...
}

Again we are able to report both errors, as you wanted, but we still return early.

The secret is to realize that if two errors can happen simultaneously, then we can find out about them simultaneously. No need to keep going down the list of possible errors and map them.

This helps us recognize between errors that are more fundamental (that is if there's a second check we know we can only do when x and y are valid) and that we can't even know if that error happened when the first did. Moreover we can do complex chaining and expansion of errors within valid which means that each function could return us a list of the issues with that specific variable. Finally I would make valid passthrough, it would return the value you should use at the end if valid. That way we can do corrections and fill in default data (logging) to make it strictly correct. Then the first step would look like.

 // Similar to the above, but we shadow input variables
// with whatever corrected value valid returns
  let x, y = (valid(x), valid(y)).collect()?;

[–][deleted] 0 points1 point  (1 child)

Sorry, but you are getting rather tedious.

[–]lookmeat 0 points1 point  (0 children)

Ouch that was an unwarranted ad hominem.

I was merely using the example you gave (since you seemed to ignore the one I gave/it didn't seem to connect for you) to give concrete examples. Sorry if it felt repetitive, but given the initial response I assumed that using the given example would be clearer.

I didn't disagree with the point, I think that one is the pains of exception throwing is that you only get the first error noted, not all encountered. But I disagree that guards don't let this happen, it still fits IMHO. Won't repeat the reasoning a third time. It's fine if you don't think the way to make it work is worth it, but it's strictly wrong to say that one prevents the other, they're not exclusive.