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.)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 likeNone
does not do that, so what's it for?
-
@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 beingSome(x)
wherex
is the actual value you care about. ReturningNone
implies that the function's value type is an optional (e.g. in Scala that would beOption[T]
). When the unusual case is actually an error, other types are used (such as Scala'sTry[T]
or Rust'sResult<T, E>
).Edit: also, in functional languages,
if/else
is an expression, not a statement, so you don't have to writereturn
in all branches - just write it once beforeif
. 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
!
.
-
@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 beingSome(x)
wherex
is the actual value you care about. ReturningNone
implies that the function's value type is an optional (e.g. in Scala that would beOption[T]
). When the unusual case is actually an error, other types are used (such as Scala'sTry[T]
or Rust'sResult<T, E>
).Edit: also, in functional languages,
if/else
is an expression, not a statement, so you don't have to writereturn
in all branches - just write it once beforeif
. 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?
-
@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.
-
@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
.
-
@GOG as I said - there's nothing special about
None
itself (it's just a regular constant much likeEXIT_SUCCESS
). But there's a certain convention built around it that makes it roughly equivalent to early throw on precondition failure.
-
@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.
-
@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 beingSome(x)
wherex
is the actual value you care about. ReturningNone
implies that the function's value type is an optional (e.g. in Scala that would beOption[T]
). When the unusual case is actually an error, other types are used (such as Scala'sTry[T]
or Rust'sResult<T, E>
).Edit: also, in functional languages,
if/else
is an expression, not a statement, so you don't have to writereturn
in all branches - just write it once beforeif
. 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()
-
@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.
-
@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 } }
-
@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.
-
@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.
-
@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 returnedNone
.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 forif
statements?
-
Is there any difference between
None
andnull
?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.
-
@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.
-
@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.
-
@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 returnedNone
.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();
-
@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 returnedNone
.If you have something after the if/else (like your
DoOtherStuff()
), then theif/else
isn't returned anymore because it's not the last expression. Instead, the result odDoOtherStuff()
is implicitly returned.However, functional style code usually has only one expression per function, so there's rarely any confusion.
-
@abarker I think
if
is an expression with a value (soelse
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.
-
@PleegWat said in Favorite Anti-Pattern:
@abarker I think
if
is an expression with a value (soelse
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 said in Favorite Anti-Pattern:
@abarker I think
if
is an expression with a value (soelse
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 discouragedNot 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).
-
@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 areJust x
(for somex
; whenx
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 returnsx
and possibly throws exceptions into a function that returns eitherJust x
orExcept 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.
-
@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 toDoSomeOtherThing()
, 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.)
-
@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.
-
@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 toDoSomeOtherThing()
, 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.
-
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.
-
@GOG I think the functional way to handle that case would be to take a
Maybe<Client>
and anAddress
and return anotherMaybe<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.
-
@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'sTry[T]
with variantsSuccess(T)
andFailure(Throwable)
. For Rust, it'sResult<T, E>
with variantsOk(T)
andErr(E)
. They're used much likeNone
, 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?
-
@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).
-
@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.
-
@Gąska Also, there is a special hell for people who won't use
else if
(orelseif
orelsif
orelif
, depending how the language spells it) and instead insist on usingelse { if … }
-
@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.