I think at least the first "success" example is unnecessarily verbose. I assume it's usually obvious that if you're testing a function in an if statement, it'll be related to whether it was successful.
The only function of an if statement is to test whether a condition is true. It’s not for executing code as a side-effect of the test.
Yeah, this blog was just weird, and should generally be ignored/avoided. To the author of the blog: If you want to do the compiler's work for it, like creating temporaries, then knock yourself out, as long as you're not working for or with me. Start by considering changing your APIs (names, contracts) if you can't tell what a function does by its name.
Readability is important. It's often of critical importance. But the blog showed examples of how to decrease readability in the name of improving readability.
I think that example does read weirdly, but because of how it uses booleans and not so much the side effect in the if statement. imo it would be fine (or at least a little better) if it was instead if (enqueueMessage().is_ok()) { … }.
However, since it makes sense to name side-effectful functions for what they do rather than the status they return, I think they will almost always read weirdly. Assigning to a variable allows a name to be given to the boolean, so it can read naturally.
This is one of the things that I've thought about the most while developing my programming style and I think I've landed on the side of "side effects are fine, actually, you just have to indicate that they're happening". So something like if(handle_event(x) == EV_OK) having side effects is fine, but if(user_callback(x) == EV_OK) having side effects is not. A comment is usually OK, but fixing the names of the functions is better.
If statements being able to have side effects is just too useful in terms of structure/formatting/etc, so programmers have to be mindful that they might happen there. But there's still a bias in the brain towards interpreting if statements as purely logical, so programmers should also take steps to plant reminders that they actually aren't in the shape/flavor of their code.
I’d say the problem is more about add returning a bool. If it were instead “added” or “already in set”, things would be clear.
Rust’s HashSet::insert returns bool in much the same way. I wish it instead defined an enum, but the ergonomics of such things aren’t great due to needing extra imports for things like == Insert::Inserted; you’d end up defining methods on the enum to convert it to bool, e.g. .is_inserted(). Something like == _::Inserted could be nicer for referring to the variant in such cases.
Its HashMap::insert, on the other hand, returns Option<V>, the value replaced, if any. That’s not ambiguous.
This is where I like .Net's TryAdd family of functions. It reads well, especially inside if statements. And the ones with out parameters to get the reference when true. Combined with nullable annotations, the IDE knows which branches are null safe too (and binding patterns is even better still, I think rust does similar here too).
I believe the point of Go's short forms is to limit the lexical scope of variables, thus reducing cognitive load. ok and entry only exist in the action clauses.
A similar rule I like is that lines of code should generally try to have a single way to fail that line. I.e. in general for structured error handling (exceptions / panics / etc.), avoid having it alongside conditionals / loops, or having multiple exceptions that can be thrown in a single chain.
There's lots of little other things that tend to come up like that that are language dependant. But my rationale is runtime tracing and the effect on logs and post crash diagnosis (and many years of hitting this sort of thing in a variety of systems across many languages).
Your sad path shouldn't look like the amazon delta
Really liked this post and agree with the point made! In a similar vein, this is also the reason I dislike the "Walrus Operator". You save a single line or two, for no performance benefits and harder to read code.
I've vacillated between this style for C specifically, but after writing plenty of Rust and Lisp where it's very common to do patterns like while let (val1, val2) = iterator.next() { ... } (also if let and such in Rust) or (let ((val (get-line *standard-input*))) ...) I realized that it doesn't really actually impact readability in most cases.
Especially with C's error handling conventions where defining a billion different temporaries with various different return types just sucks versus just putting the function in the conditional unless it'd really be unclear.
prayerie | 12 hours ago
I think at least the first "
success" example is unnecessarily verbose. I assume it's usually obvious that if you're testing a function in an if statement, it'll be related to whether it was successful.Says who?
cpurdy | 6 hours ago
Yeah, this blog was just weird, and should generally be ignored/avoided. To the author of the blog: If you want to do the compiler's work for it, like creating temporaries, then knock yourself out, as long as you're not working for or with me. Start by considering changing your APIs (names, contracts) if you can't tell what a function does by its name.
Readability is important. It's often of critical importance. But the blog showed examples of how to decrease readability in the name of improving readability.
[OP] gavinmorrow | 6 hours ago
I think that example does read weirdly, but because of how it uses booleans and not so much the side effect in the if statement. imo it would be fine (or at least a little better) if it was instead
if (enqueueMessage().is_ok()) { … }.However, since it makes sense to name side-effectful functions for what they do rather than the status they return, I think they will almost always read weirdly. Assigning to a variable allows a name to be given to the boolean, so it can read naturally.
wareya | 13 hours ago
This is one of the things that I've thought about the most while developing my programming style and I think I've landed on the side of "side effects are fine, actually, you just have to indicate that they're happening". So something like
if(handle_event(x) == EV_OK)having side effects is fine, butif(user_callback(x) == EV_OK)having side effects is not. A comment is usually OK, but fixing the names of the functions is better.If statements being able to have side effects is just too useful in terms of structure/formatting/etc, so programmers have to be mindful that they might happen there. But there's still a bias in the brain towards interpreting if statements as purely logical, so programmers should also take steps to plant reminders that they actually aren't in the shape/flavor of their code.
Ambroisie | 9 hours ago
IMO
user_callbackobviously must be thought of as having side effects: your code doesn't control it.Edit: I don't disagree with your point, just the specific example. And I guess that's where we fall back to coding style issues.
veqq | 11 hours ago
Counterpoint, I like putting side effects in my... let blocks:
chrismorgan | 13 hours ago
I’d say the problem is more about
addreturning a bool. If it were instead “added” or “already in set”, things would be clear.Rust’s
HashSet::insertreturnsboolin much the same way. I wish it instead defined an enum, but the ergonomics of such things aren’t great due to needing extra imports for things like== Insert::Inserted; you’d end up defining methods on the enum to convert it to bool, e.g..is_inserted(). Something like== _::Insertedcould be nicer for referring to the variant in such cases.Its
HashMap::insert, on the other hand, returnsOption<V>, the value replaced, if any. That’s not ambiguous.jessicah | 11 hours ago
This is where I like .Net's TryAdd family of functions. It reads well, especially inside if statements. And the ones with out parameters to get the reference when true. Combined with nullable annotations, the IDE knows which branches are null safe too (and binding patterns is even better still, I think rust does similar here too).
bediger4000 | 8 hours ago
How about Go's "short form" if statements?
if entry, ok := dosomething(key); ok {I believe the point of Go's short forms is to limit the lexical scope of variables, thus reducing cognitive load.
okandentryonly exist in the action clauses.joshka | 11 hours ago
A similar rule I like is that lines of code should generally try to have a single way to fail that line. I.e. in general for structured error handling (exceptions / panics / etc.), avoid having it alongside conditionals / loops, or having multiple exceptions that can be thrown in a single chain.
There's lots of little other things that tend to come up like that that are language dependant. But my rationale is runtime tracing and the effect on logs and post crash diagnosis (and many years of hitting this sort of thing in a variety of systems across many languages).
Your sad path shouldn't look like the amazon delta
conor | 9 hours ago
Really liked this post and agree with the point made! In a similar vein, this is also the reason I dislike the "Walrus Operator". You save a single line or two, for no performance benefits and harder to read code.
dpedu | 8 hours ago
Reminds me of the The Linux Backdoor Attempt of 2003. Side-effects indeed.
nytpu | an hour ago
I've vacillated between this style for C specifically, but after writing plenty of Rust and Lisp where it's very common to do patterns like
while let (val1, val2) = iterator.next() { ... }(alsoif letand such in Rust) or(let ((val (get-line *standard-input*))) ...)I realized that it doesn't really actually impact readability in most cases.Especially with C's error handling conventions where defining a billion different temporaries with various different return types just sucks versus just putting the function in the conditional unless it'd really be unclear.