3

So I've read through Short circuit evaluation, is it bad practice?, Is it bad practice to use short-circuit evaluation instead of an if clause? and some other related questions, and I get that it is generally considered bad practice to use short-circuit evaluation for flow control.

I'm trying to write warning-free, ROC (Really Obvious Code), and I'm looking for the best way to do only the first of a number of actions that returns a truthy value.

The code I am refactoring from was originally like so:

this.initSingleTag(tags.open_at === '7', CombinedSearch.ITEMS.avail_morning, 'search-functions-open', 'morning') ||
this.initSingleTag(tags.open_at === '12', CombinedSearch.ITEMS.avail_midday, 'search-functions-open', 'midday') ||
this.initSingleTag(tags.open_at === '18', CombinedSearch.ITEMS.avail_evening, 'search-functions-open', 'evening') ||
this.initSingleTag(tags.open_at === '22', CombinedSearch.ITEMS.avail_late, 'search-functions-open', 'late');

Trying to get it free of warnings in JSHint, my first-run refactor looks like:

if (!this.initSingleTag(tags.open_at === '7', CombinedSearch.ITEMS.avail_morning, 'search-functions-open', 'morning')) {
   if (!this.initSingleTag(tags.open_at === '12', CombinedSearch.ITEMS.avail_midday, 'search-functions-open', 'midday')) {
     if (!this.initSingleTag(tags.open_at === '18', CombinedSearch.ITEMS.avail_evening, 'search-functions-open', 'evening')) {
       this.initSingleTag(tags.open_at === '22', CombinedSearch.ITEMS.avail_late, 'search-functions-open', 'late');
     }
   }
 }

Which honestly looks no more readable to me. Is there a common better practice for this kind of situation?

Edit: Since posting the question and headscratching a bit, I had come up with the following:

CombinedSearch.prototype.initFirstTruthyTag = function(tags) {
    var that = this;
    $.each(tags, function(i, parameters) {
        return !(that.initSingleTag.apply(that, parameters));
    });
};

...Later...

this.initFirstTruthyTag([
    [tags.open_at === '7', CombinedSearch.ITEMS.avail_morning, 'search-functions-open', 'morning'],
    [tags.open_at === '12', CombinedSearch.ITEMS.avail_midday, 'search-functions-open', 'midday'],
    [tags.open_at === '18', CombinedSearch.ITEMS.avail_evening, 'search-functions-open', 'evening'],
    [tags.open_at === '22', CombinedSearch.ITEMS.avail_late, 'search-functions-open', 'late']
]);

Although I am painfully aware of the fact that the third parameter is repeated multiple times, it's kind of a sideline to the original question (plus I am not sure that dogged compliance with DRY outweighs the consistency with the original function call, which is still used throughout the rest of the code around this call)

Community
  • 1
Chris O'Kelly
  • 161

3 Answers3

6

What about something along the lines of

var tagCombiList = [['7',CombinedSearch.ITEMS.avail_morning, 'morning'],
                    ['12',CombinedSearch.ITEMS.avail_midday, 'midday'],
                    ...]

for(t of tagCombiList)
{
    if(this.initSingleTag(tags.open_at === t[0],t[1],'search-functions-open', t[3]) 
       break;
}

The advantage of this code is not only it avoids the need for short circuits. It is also much more DRY - in both of your versions, there was almost the same line of code repeated four times. Moreover, by separating the tabular data combination of your input data from the code which actually uses the data, you have a better separation of concerns, which as a "side effect" needs less columns, which also improves readability.

Doc Brown
  • 218,378
4

The short-circuiting version might not be the most beautiful piece of code but it certainly isn't terrible either. I strongly recommend that you format it differently, though.

Looking at this code, it is not immediately clear what is going on.

this.initSingleTag(tags.open_at === '7', CombinedSearch.ITEMS.avail_morning, 'search-functions-open', 'morning') ||
this.initSingleTag(tags.open_at === '12', CombinedSearch.ITEMS.avail_midday, 'search-functions-open', 'midday') ||
this.initSingleTag(tags.open_at === '18', CombinedSearch.ITEMS.avail_evening, 'search-functions-open', 'evening') ||
this.initSingleTag(tags.open_at === '22', CombinedSearch.ITEMS.avail_late, 'search-functions-open', 'late');

This version – which is identical except for white-space – is much clearer.

this.initSingleTag(tags.open_at === '7', CombinedSearch.ITEMS.avail_morning, 'search-functions-open', 'morning')
    || this.initSingleTag(tags.open_at === '12', CombinedSearch.ITEMS.avail_midday, 'search-functions-open', 'midday')
    || this.initSingleTag(tags.open_at === '18', CombinedSearch.ITEMS.avail_evening, 'search-functions-open', 'evening')
    || this.initSingleTag(tags.open_at === '22', CombinedSearch.ITEMS.avail_late, 'search-functions-open', 'late');

I generally believe that usage of white-space is mostly a matter of taste but not so in this case.

The other two answers have already pointed out that your code is actually an iteration that stops at the first element for which initSingleTag returns true so I agree that a loop (or, preferably, a lazy combination of any and map if your technology supports it) should be used here.

If the logic cannot be formulated as a loop – for example, because you're calling different functions every time and functions are not first-class citizens in your language – then note that there is another alternative to short-circuiting operators and the arrow-style nested if cascade.

if (firstStrategy(...)) {
} else if (secondStrategy(...)) {
} else if (thirdStrategy(...)) {
} else {
    this.isNotYourLuckyDay(...);
}

Technically, this is still a nested if but much nicer to look at. Using early returns would be yet another alternative.

if (firstStrategy(...)) {
    return;
}
if (secondStrategy(...)) {
    return;
}
if (thirdStrategy(...)) {
    return;
}
this.isNotYourLuckyDay(...);
5gon12eder
  • 7,236
2

If you want to create a more readable code you can:

a.- Create a dictionary with the key/value pairs for CombinedSearchItems.

b.- Create another method/function to call initSingleTag removing duplicated code.

c.- Split the if conditions.

CombinedSearchItemsDict = {
  'morning': CombinedSearch.ITEMS.avail_morning,
  'midday': CombinedSearch.ITEMS.avail_midday, 
  'evening': CombinedSearch.ITEMS.avail_evening, 
  'late': CombinedSearch.ITEMS.avail_late 
}

cond1 = magicFunction(7, 'morning')
cond2 = !cond1 && magicFunction(12, 'midday')
cond3 = !cond1 && !cond2 && magicFunction(18, 'evening')
if (!cond3) {
       magicFunction(22, 'late')
}

function magicFunction(time, day_time)
{
  timeStr = str(time)
  combinedSearch = CombinedSearchItemsDict[day_time] 

  return this.initSingleTag(tags.open_at === time, combinedSearch, 'search-functions-open', day_time)
}

... but as you can see this is still hard to understand. The key is create code that it will be easy to read for someone in the future... (That someone can be you)..

I mean, if someone reads 'magicFunction' there is nothing on that name that tells what is going on!

So, instead of using a enigmatic name like magicFunction you should use a name that describes what are you doing inside that function. The same for cond1, cond2 and cond3, another name like morningTagSuccess, middayTagSuccess and eveningTagSuccess can give more info about what is going on,, but still...

Maybe more important, you have to ask yourself, What I'm trying to do? and Why? Are you trying to do something by brute-force? Why are you trying to see "If I have no success then, try this another thing?" Why not check first what case you have to execute based on your parameters/system status? Something like; 

if (isTagValid('morning'))
{
    applySingleTag(7, 'morning')
}
else if (isTagValid('midday'))
{
    applySingleTag(12, 'midday')
}
else if (isTagValid('evening'))
{
    applySingleTag(18, 'evening')
}
else
{
    applySingleTag(22, 'late')
}

Yep, we are not using short circuit, but maybe this is not the place to do it.

mayo
  • 123
  • 6