Keep if clauses side-effect free
8 points by gavinmorrow
8 points by gavinmorrow
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.
Says who?
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.
IMO user_callback obviously 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.
Counterpoint, I like putting side effects in my... let blocks:
(let [path "test.db"
_ (when (os/stat path) (os/rm path))
db (sql/open path)]
(defer (os/rm path)
(defer (sql/close db)
(sql/eval db `CREATE TABLE people(name TEXT, age INTEGER, bool INTEGER);`)
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. ok and entry only exist in the action clauses.
boolean isNewCategory = categorySeen.add(categoryID);
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).
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.
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