DEV Community

Cover image for Code Smell 145 - Short Circuit Hack
Maxi Contieri
Maxi Contieri

Posted on • Updated on • Originally published at maximilianocontieri.com

Code Smell 145 - Short Circuit Hack

Don't use boolean evaluation as a readability shortcut

TL;DR: Don't use Boolean comparison for side effect functions.

Problems

  • Readability

  • Side Effects

Solutions

  1. Convert short circuit into IFs

Context

Smart programmers like to write hacky and obscure code even when there is no strong evidence for this improvement.

Premature optimization always hurts readability.

Sample Code

Wrong

userIsValid() && logUserIn();

// this expression is short circuit
// Does not value second statament
// Unless the first one is true

functionDefinedOrNot && functionDefinedOrNot();

// in some languages undefined works as a false
// If functionDefinedOrNot is not defined does
// not raise an erron and neither runs
Enter fullscreen mode Exit fullscreen mode

Right

if (userIsValid()) {
    logUserIn();
}

if(typeof functionDefinedOrNot == 'function') {  
    functionDefinedOrNot();
}
// Checking for a type is another code smell
Enter fullscreen mode Exit fullscreen mode

Detection

[X] Semi-Automatic

We can check if the functions are impure and change the short circuit to an IF.

Some actual linters warn us of this problem

Tags

  • Premature Optimizacion

Conclusion

Don't try to look smart.

We are not in the 50s anymore.

Be a team developer.

Relations

Credits

Photo by Michael Dziedzic on Unsplash


A computer is a stupid machine with the ability to do incredibly smart things, while computer programmers are smart people with the ability to do incredibly stupid things. They are, in short, a perfect match.

Bill Bryson


This article is part of the CodeSmell Series.

Discussion (12)

Collapse
ingosteinke profile image
Ingo Steinke

What about optional chaining? Which of course only works with functions designed to be chained, like user?.isValid()?.logUserIn();

Collapse
moopet profile image
Ben Sinclair

I'd use that for finding a deep value but not performing an action:

if (user?.foo?.bar) {
  user.login();
  // or...
  logUserIn(user);
  // or whatever
}
Enter fullscreen mode Exit fullscreen mode

Also, the implication in your example is that isValid() returns a user object when that's not obvious - it looks like it should be a boolean.

Collapse
feliciousity profile image
Felicia Ann Kelley

It has to be a boolean, boolean is how the data is first made.

Collapse
mcsee profile image
Maxi Contieri Author • Edited on

Null is the worst code smell

And optional chaining, IMHO hids this antipattern.
I've written a smell on optional chaining. I will publish it in the following weeks

hackernoon.com/null-the-billion-do...

And optional chaining is another code smell

Collapse
larsejaas profile image
Lars Ejaas

But we are not always writing the data or code with null ourselves.

One example: Auto-generated types made with GraphQL Code Generator.
We still need to be able to handle null.
Null does indeed make sense if you do not know if data will return empty.

Thread Thread
mcsee profile image
Maxi Contieri Author

Of course
In case null is beyond your own control you need to deal with it

That is why it is the billion dollar mistake

We must be mature enough to avoid creating NEW nulls

Thread Thread
larsejaas profile image
Lars Ejaas

Yeah, that makes sense 😄

Collapse
lukeshiru profile image
Luke Shiru

While I agree with your point about null, that doesn't make optional chaining bad. Optional chaining also works with undefined, and is amazing. If we pretend that the type of functionDefinedOrNot is (() => void) | undefined, then instead of doing this:

if (functionDefinedOrNot !== undefined) {  
    functionDefinedOrNot();
}
Enter fullscreen mode Exit fullscreen mode

We can simply:

functionDefinedOrNot?.();
Enter fullscreen mode Exit fullscreen mode

Which at least from my point of view is far better.

Collapse
bwca profile image
Volodymyr Yepishev

In my humble opinion user?.isValid()?.logUserIn() reads like a human sentence that makes sense, compared to multiline typeof blocks. Looks Smells good to me :)

Collapse
rgails profile image
Ryan Gails

My big issue with this is debugging it. Did we not log in because we have no user, because the user isn't valid, or because isValid returned null (which seems like something you'd want to handle in a particular way IMO)? A single line getting the job done is great until you're ripping through 20 lines of chained methods trying to figure out where things went wrong.

I agree with Ben that it's fine to get deep values with though. Especially if you really just care about whether or not you could get to the value you were looking for.

Collapse
eljayadobe profile image
Eljay-Adobe

When I've asked about optional chaining in C++, the push back I've gotten has been that it's a code smell because optional chaining means high coupling and intimate knowledge of the private parts.

So, for better or worse, in C++ we'll still have this...

if (user && user->isValid())
    user->isValid()->logUserIn();
Enter fullscreen mode Exit fullscreen mode
Collapse
omarbenmegdoul profile image
Omar Benmegdoul • Edited on

Why is this a problem specifically before a side-effect?

Something like myFunc?.() will force people to pause. On the other hand short-circuits are a common pattern when setting default values or optionally rendering React components. People are used to reading them in other contexts, often with function calls on the right, so why do they become less readable in this one?