Favorite Anti-Pattern

GOG

Seriously, though, that's almost always feasible. At least, as long as there isn't something after the if/else, like "if (condition) DoStuff() else None; DoOtherStuff();"

I kinda missed this the first time 'round, but what does None actually do? (Pinging @Gąska while I'm at it.)

With throw, control returns to the caller, so you don't care about anything that might come after (in this code path). However, from what you wrote it looks like None does not do that, so what's it for?

Gąska

@GOG said in Favorite Anti-Pattern:

@abarker said in Favorite Anti-Pattern:

Seriously, though, that's almost always feasible. At least, as long as there isn't something after the if/else, like "if (condition) DoStuff() else None; DoOtherStuff();"

I kinda missed this the first time 'round, but what does None actually do? (Pinging @Gąska while I'm at it.)

It's like, when you want an early return without an actual value, but don't want to call it an error. Like, say, when you have a function that returns the last element of a list - for an empty list there's nothing to return, but it's not exactly an abnormal case, so you return None.

Specifically, None is the idiomatic name for the empty variant of an optional type, the non-empty variant being Some(x) where x is the actual value you care about. Returning None implies that the function's value type is an optional (e.g. in Scala that would be Option[T]). When the unusual case is actually an error, other types are used (such as Scala's Try[T] or Rust's Result<T, E>).

Edit: also, in functional languages, if/else is an expression, not a statement, so you don't have to write return in all branches - just write it once before if. Also also, the last expression in a function is implicitly returned, so you can omit the explicit keyword altogether.

Edit 2: to better illustrate my point - what I'm saying is that

if (!condition) {
    lets();
    do();
    something()
} else {
    None
}

looks better than

if (condition) {
    None
} else {
    lets();
    do();
    something()
}

especially when you can avoid the !.

GOG

@Gąska said in Favorite Anti-Pattern:

@GOG said in Favorite Anti-Pattern:

@abarker said in Favorite Anti-Pattern:

Seriously, though, that's almost always feasible. At least, as long as there isn't something after the if/else, like "if (condition) DoStuff() else None; DoOtherStuff();"

I kinda missed this the first time 'round, but what does None actually do? (Pinging @Gąska while I'm at it.)

It's like, when you want an early return without an actual value, but don't want to call it an error. Like, say, when you have a function that returns the last element of a list - for an empty list there's nothing to return, but it's not exactly an abnormal case, so you return None.

Specifically, None is the idiomatic name for the empty variant of an optional type, the non-empty variant being Some(x) where x is the actual value you care about. Returning None implies that the function's value type is an optional (e.g. in Scala that would be Option[T]). When the unusual case is actually an error, other types are used (such as Scala's Try[T] or Rust's Result<T, E>).

Edit: also, in functional languages, if/else is an expression, not a statement, so you don't have to write return in all branches - just write it once before if. Also also, the last expression in a function is implicitly returned, so you can omit the explicit keyword altogether.

Okay, so None is a return to the caller? This being the case, "if (condition) DoStuff() else None; DoOtherStuff()" is equivalent to "if (condition) { DoStuff(); DoOtherStuff(); } else None;" (braces to indicate scope)? Will "None; DoOtherStuff()" actually cause other stuff to be done?

Gąska

@GOG see edit 2. None isn't quite return to the caller - it's just an empty value - but it's almost always used to return to the caller without actual result.

GOG

@Gąska The edit doesn't really address my main question, namely if you have:

None
DoSomething();

Will something be done, or will the function return None.

Gąska

@GOG as I said - there's nothing special about None itself (it's just a regular constant much like EXIT_SUCCESS). But there's a certain convention built around it that makes it roughly equivalent to early throw on precondition failure.

GOG

@Gąska That still doesn't answer my question. I want to know whether once your code path encounters None does it return this value or does it continue executing. I don't know how I can make it any clearer than this.

It is an honest question: I really don't know.

Zecc

@Gąska said in Favorite Anti-Pattern:

@GOG said in Favorite Anti-Pattern:

@abarker said in Favorite Anti-Pattern:

Seriously, though, that's almost always feasible. At least, as long as there isn't something after the if/else, like "if (condition) DoStuff() else None; DoOtherStuff();"

I kinda missed this the first time 'round, but what does None actually do? (Pinging @Gąska while I'm at it.)

It's like, when you want an early return without an actual value, but don't want to call it an error. Like, say, when you have a function that returns the last element of a list - for an empty list there's nothing to return, but it's not exactly an abnormal case, so you return None.

Specifically, None is the idiomatic name for the empty variant of an optional type, the non-empty variant being Some(x) where x is the actual value you care about. Returning None implies that the function's value type is an optional (e.g. in Scala that would be Option[T]). When the unusual case is actually an error, other types are used (such as Scala's Try[T] or Rust's Result<T, E>).

Edit: also, in functional languages, if/else is an expression, not a statement, so you don't have to write return in all branches - just write it once before if. Also also, the last expression in a function is implicitly returned, so you can omit the explicit keyword altogether.

Edit 2: to better illustrate my point - what I'm saying is that
if (!condition) {
    lets();
    do();
    something()
} else {
    None
}
looks better than
if (condition) {
    None
} else {
    lets();
    do();
    something()
}
especially when you can avoid the !.

None of those look better, to my eyes, than

if (condition) {
    return None;
}

// Look ma! Less indenting on the main logic
lets();
do();
something()

Benjamin Hall

@GOG My @Gąska to english translation service is saying "no, it's just convention." Meaning you have to ensure that the None is the terminal path for that code flow manually--the return is implicit in being the last statement of the execution flow. Which seems to be quite a pain, but I'm no functional programming expert.

Gąska

@GOG uh...

def foo(): Int {
  if (something) {
    return 1;
  } else {
    return 2;
  }
}

...is equivalent to (because if/else is expression)...

def foo(): Int {
  return if (something) {
    1
  } else {
    2
  };
}

...is equivalent to (because implicit return of last expression)...

def foo(): Int {
  if (something) {
    1
  } else {
    2
  }
}

...and the idiom is that...

def bar(): Option[Foo] {
  if (canDoWork) {
    Some(doWork())
  } else {
    None
  }
}

Zecc

@GOG said in Favorite Anti-Pattern:

@Gąska The edit doesn't really address my main question, namely if you have:
None
DoSomething();
Will something be done, or will the function return None.

I think that's a syntax error, because you have would-be statement which is just an expression without a semicolon, while not being the last statement in the flow of instructions.

Gąska

@Zecc said in Favorite Anti-Pattern:

None of those look better, to my eyes, than

if (condition) {
    return None;
}

// Look ma! Less indenting on the main logic
lets();
do();
something()

There have been a couple times where I've missed an early return when reading a function and thought that the code runs unconditionally when in fact it's only run if some condition is met. If it's only 1 level, if/else is better than if/(implicit second branch at top level). Things get more complicated with more than 1 level.

Also, when you read a lot of match-heavy functional code, if/else starts looking more readable due to analogy.

GOG

@Gąska said in Favorite Anti-Pattern:

@GOG uh...
def foo(): Int {
  if (something) {
    return 1;
  } else {
    return 2;
  }
}
...is equivalent to (because if/else is expression)...
def foo(): Int {
 return if (something) {
   1
 } else {
   2
 };
}
...is equivalent to (because implicit return of last expression)...
def foo(): Int {
 if (something) {
   1
 } else {
   2
 }
}

Okay, so:

@abarker said in Favorite Anti-Pattern:

Seriously, though, that's almost always feasible. At least, as long as there isn't something after the if/else, like "if (condition) DoStuff() else None; DoOtherStuff();"

Which I understand to be:

if(condition)
{
    DoStuff();
}
else
{
   None
}

DoOtherStuff();  // Does this execute if condition is false? And why isn't this a proper comment?

From what you write, if condition is false, DoOtherStuff() will not execute, because the function returned None.

If this is indeed the case, the code is equivalent to:

if(condition)
{
    DoStuff();
    DoOtherStuff();
}
else
{
    None
}

What am I missing?

ETA: If None isn't a function return, that would make sense, but in that case why isn't it implicit for if statements?

JBert

Is there any difference between None and null?

Filed under: Only been diagonally reading the previous discussion.

EDIT: Ah, I realized there might be. Languages like C# do some chicanery (most likely at compilation time) to make them the same whereas Java implemented "optional" types as a framework feature rather than a language / runtime feature, in which case the optional type can have either have value "empty" or null, the latter being a mistake.

abarker

@GOG said in Favorite Anti-Pattern:

@abarker said in Favorite Anti-Pattern:

Install something like Notepad++. It's a decent text editor, and you can fold code blocks in it.

I know. That's why (among other things) I used the word "editor" rather than "IDE".

Nevertheless, you're still assuming I am free to install Notepad++ wherever it is I need it. What if this assumption turns out not to hold?

That's when you throw your hands up in defeat and cry.

abarker

@GOG said in Favorite Anti-Pattern:

@abarker said in Favorite Anti-Pattern:

Seriously, though, that's almost always feasible. At least, as long as there isn't something after the if/else, like "if (condition) DoStuff() else None; DoOtherStuff();"

I kinda missed this the first time 'round, but what does None actually do? (Pinging @Gąska while I'm at it.)

I think it's a placeholder that varies with the context. If the check is in a loop it could be continue;. Otherwise, it's going to be a throw or return of some sort. Or, it could just be not doing anything.

abarker

@GOG said in Favorite Anti-Pattern:

@Gąska said in Favorite Anti-Pattern:
@GOG uh...
def foo(): Int {
  if (something) {
    return 1;
  } else {
    return 2;
  }
}
...is equivalent to (because if/else is expression)...
def foo(): Int {
 return if (something) {
   1
 } else {
   2
 };
}
...is equivalent to (because implicit return of last expression)...
def foo(): Int {
 if (something) {
   1
 } else {
   2
 }
}
Okay, so:

@abarker said in Favorite Anti-Pattern:

Seriously, though, that's almost always feasible. At least, as long as there isn't something after the if/else, like "if (condition) DoStuff() else None; DoOtherStuff();"

Which I understand to be:
if(condition)
{
    DoStuff();
}
else
{
   None
}

DoOtherStuff();  // Does this execute if condition is false? And why isn't this a proper comment?
From what you write, if condition is false, DoOtherStuff() will not execute, because the function returned None.

If this is indeed the case, the code is equivalent to:
if(condition)
{
    DoStuff();
    DoOtherStuff();
}
else
{
    None
}

It's also possible (in my mind) that None is a no-op, in which case the simplified code would actually be:

if (condition)
{
    DoStuff();
}
DoOtherStuff();

Gąska

@GOG said in Favorite Anti-Pattern:

@Gąska said in Favorite Anti-Pattern:
@GOG uh...
def foo(): Int {
  if (something) {
    return 1;
  } else {
    return 2;
  }
}
...is equivalent to (because if/else is expression)...
def foo(): Int {
 return if (something) {
   1
 } else {
   2
 };
}
...is equivalent to (because implicit return of last expression)...
def foo(): Int {
 if (something) {
   1
 } else {
   2
 }
}
Okay, so:

@abarker said in Favorite Anti-Pattern:

Seriously, though, that's almost always feasible. At least, as long as there isn't something after the if/else, like "if (condition) DoStuff() else None; DoOtherStuff();"

Which I understand to be:
if(condition)
{
    DoStuff();
}
else
{
   None
}

DoOtherStuff();  // Does this execute if condition is false? And why isn't this a proper comment?
From what you write, if condition is false, DoOtherStuff() will not execute, because the function returned None.

If you have something after the if/else (like your DoOtherStuff()), then the if/else isn't returned anymore because it's not the last expression. Instead, the result od DoOtherStuff() is implicitly returned.

However, functional style code usually has only one expression per function, so there's rarely any confusion.

PleegWat

@abarker I think if is an expression with a value (so else is mandatory). return exists but is strongly discouraged; the value of the last statement is returned automatically and that is preferred. IE, early return prohibited, which most others in this thread seem to have indicated to be undesirable.

abarker

@PleegWat said in Favorite Anti-Pattern:

@abarker I think if is an expression with a value (so else is mandatory). return exists but is strongly discouraged; the value of the last statement is returned automatically and that is preferred. IE, early return prohibited, which most others in this thread seem to have indicated to be undesirable.

That is going to depend on the language. I thought we were (at this point) talking about no language in particular. What you're saying, on the other hand, is language specific. Though I am unclear about which language you are referring to.

PleegWat

@abarker Sorry, I mean what @gaska was talking about.

Gąska

@PleegWat said in Favorite Anti-Pattern:

@abarker I think if is an expression with a value (so else is mandatory).

Not mandatory. If the "then" part returns unit, then you can omit else and it behaves like a C programmer would expect.

return exists but is strongly discouraged

Not discouraged as much as mostly useless because there are alternatives (remember that aside from if/else there's also match expression that can fulfill your multi-error needs).

dkf

@GOG said in Favorite Anti-Pattern:

but what does None actually do?

Conventionally it is either the sole member of the type with only one value (so it actually requires zero bits to express) or it is part of a tagged union Option typeclass where the other values are Just x (for some x; when x is of a non-nullable reference type, it's like making a type that adds nullability except you can't get the proper reference out of it without handling the possibility that the reference might not be there). In some languages, you transform a function that returns x and possibly throws exceptions into a function that returns either Just x or Except theException and then the caller undoes the transform on the other side. Which sounds a lot more complex than it actually is at the level the user works.

Digressing, there are generally two possible ways to handle exceptions in language implementations. They do not have to be done in the way that C++ does it, and when you've got the case where an exception isn't actually travelling very far, the C++ “zero cost” approach is actually a lot more expensive than the jump threading approach, which is what all the messing about with option types ends up getting built into once you get serious with the inlining. Part of the problem with the C++ approach (as I've seen it in the LLVM code at least) is that the generating of the exception is not part of the IR, and remains actually an unanalysed event; the compiler can't find a way to make exceptions less costly even in the cases where it would otherwise be able to detect that the failure handler would otherwise be something that the failure handler would otherwise be possible to jump to directly (threading in any scope cleanup code needed along the way). It's a really strange omission in my view, and means that error handling in C++ as implemented by LLVM is considerably slower than it might otherwise be.

GOG

@Gąska Yeah, when I try to consider this code from a functional perspective (which most definitely isn't my usual thing), it doesn't really parse. I would expect there to be some kind of expression to connect the if clause to DoSomeOtherThing(), so it all resolves to a single return.

But while we're at it: what would happen to the result of this if clause, if it isn't the last expression in the function? Does it disappear into the void?

(Just to clarify, I get the idioms, but I'm not clear on the formal side of the language.)

Watson

@GOG said in Favorite Anti-Pattern:

But while we're at it: what would happen to the result of this if clause, if it isn't the last expression in the function? Does it disappear into the void?

In this context (functional programming à la Scala or Haskell) a function is a single expression; there isn't a "sequence of statements" as such (though it is perfectly feasible to implement the concept and most FP languages provide syntactic sugar to make doing so palatable).

In functional programming, functions are much more like functions than like algorithms. Hence the name.

Gąska

@GOG said in Favorite Anti-Pattern:

@Gąska Yeah, when I try to consider this code from a functional perspective (which most definitely isn't my usual thing), it doesn't really parse. I would expect there to be some kind of expression to connect the if clause to DoSomeOtherThing(), so it all resolves to a single return.

Technically there is. The ; is an infix left-associative operator takes two arguments and returns the value on the right. Some languages also have a suffix ; that takes one argument and returns unit (as in, explicitly ignores the value).

But I find it easier to think about ; as a statement terminator that "disables auto-return" and lets you add another expression to a function.

Some functional languages, such as Scala, have Javascriptesque automatic semicolon insertion, so you get the same effect by making a line break.

But while we're at it: what would happen to the result of this if clause, if it isn't the last expression in the function? Does it disappear into the void?

Yes, and depending on language and compiler options, you might get a warning about that.

GOG

Thanks @Watson and @Gąska for clearing things up for me.

Now that I've had time to digest everything, @Gąska, I realize that the confusion about whether it's sensible to test preconditions early stemmed from the fact that what you're talking about is something different than what I'm talking about.

Let's assume we have a Client type, which is nullable, and one of its properties is a Representative - of type Person, also nullable.

Say, we want to write a function that gets the representative of the client we pass into it:

Person GetClientRepresentative(Client client)
{
    // We cannot naively dereference client, because we might get a null reference, so...

    if (client != null)
    {
        return client.Representative;
    }
    else
    {
        return null;
    }
}

This is what you're talking about and here it absolutely makes sense to have the "failure" clause (we passed a null client) come last.

Here's what I'm talking about - suppose we want to set the client's address:

void SetClientAddress(Client client, Address address)
{
    // We can't simply set client.Address, because of a possible null reference, so...

    // Guard clause to ensure assumptions are met.
    if (client == null)
    {
        // Don't try to pass null clients, dummy.
        throw new ArgumentNullException(nameof(client));
    }

    // If we got this far, we know client is not null, so we can safely assign.
    // We're not checking whether address is null (if nullable), because a null address is a valid option.
    client.Address = address;
}

We could write this as:

void SetClientAddress(Client client, Address address)
{
    if (client != null)
    {
        client.Address = address;
    }
    else
    {
        throw new ArgumentNullException(nameof(client));
    }
}

but if we do it like this, we'll have all the problems reading and reasoning about the code I wrote of earlier, as soon as we're dealing with a non-trivial valid case.

Conceptually, what you're talking about is substituting a default value when actual value cannot be determined. What I'm talking about is a no-go situation, where you can't do anything sensible, other than bug out and complain to the caller.

TwelveBaud

@GOG I think the functional way to handle that case would be to take a Maybe<Client> and an Address and return another Maybe<Client> that, if there is actually a client in there, has the new address in there, but if there's nothing in there let the nothingness propagate. There's probably some sort of sugar for Maybe-"setting" (i.e. return a copy of the possibly-extant object but with that part mutated) but I don't work enough with functional to know.

Gąska

@GOG None is just an easy example. As I mentioned, for actual error conditions there are other types to use - each language has its own. In Scala, for example, it's Try[T] with variants Success(T) and Failure(Throwable). For Rust, it's Result<T, E> with variants Ok(T) and Err(E). They're used much like None, except they have space to describe the actual error that occured.

As I mentioned, the ordering is just a matter of personal preference to me - I like having the "ok" case at the top because it matches how I read functions. "This function is called such and such. It has such and such arguments. It needs to know this and this, and if everything is okay, it does this and this. Otherwise it reports this error, or this one, or this one."

The "forgetting" argument really doesn't sound convincing to me. If the function is so large you can forget what was in the condition of an if/else, it ought to be split up into smaller ones anyway.

If there are a lot of preconditions that all report a different error, then yes, it does become unreadable to put the "happy" path in "then" branch. But that has nothing to do with wanting to know error conditions first, and everything to do with excessive nesting and there-and-back-again nature of consecutive elses (they appear in the reverse order from the conditions checked).

If there was no nesting problem, I would still put the "happy" case first. And in functional languages, there is a way out - pattern matching.

Consider the following class:

class Foo {
  int bar;
  Baz baz; // enum
  Qux qux; // another class
}

And the following function:

int DoSomething(Foo foo)
{
  if (x.bar < 2) {
    throw new Exception("bar less than 2");
  }

  if (x.baz != Baz.Inga) {
    throw new Exception("Bazinga!");
  }

  if (x.qux is null) {
    throw new Exception("no qux");
  }

  return x.bar + x.qux.bar;
}

It can be written in Scala like this (assuming Foo is rewritten as case class):

def DoSomething(foo: Foo): Try[Int] {
  foo match {
    case Foo(bar, Baz.Inga, Some(qux)) if bar >= 2 =>
      Success(bar + qux.bar)
    case Foo(bar, _, _) if bar < 2 =>
      Failure(new Exception("bar less than 2"))
    case Foo(_, baz, _) if baz != Baz.Inga =>
      Failure(new Exception("Bazinga!"))
    case Foo(_, _, None) =>
      Failure(new Exception("no qux"))
  }
}

You have multiple error cases with different messages, but no nesting. Is there still any readability to be gained from moving the first case to the end, after all the error cases?

dkf

@Gąska said in Favorite Anti-Pattern:

I like having the "ok" case at the top because it matches how I read functions.

Interesting. I strongly prefer having the things that work like preconditions up front so that they effectively document the fact that these are checked assumptions. I also like to keep the nesting level of the function core as low as possible (because that's often quite high anyway).

Gąska

@dkf said in Favorite Anti-Pattern:

@Gąska said in Favorite Anti-Pattern:

I like having the "ok" case at the top because it matches how I read functions.

Interesting. I strongly prefer having the things that work like preconditions up front so that they effectively document the fact that these are checked assumptions.

Well, the assumptions are being checked beforehand anyway - I mean, how else do you prevent running the "meat" if they're not met? It's about whether you want the list of all possible error messages at the top or at the bottom.

I also like to keep the nesting level of the function core as low as possible (because that's often quite high anyway).

Me too. See previous post.

dkf

@Gąska Also, there is a special hell for people who won't use else if (or elseif or elsif or elif, depending how the language spells it) and instead insist on using else { if … }

GOG

@Gąska Honestly, I find the Scala pattern-matching version to be a pain to read whatever the order. I feel we're talking past each other at this point and FWIW I fully agree with you that "happy case first" is a good choice for the kind of examples you've posted.