Short circuit evaluation to do only the first of a number of actions

Question

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)

Yes, put the conditions and their corresponding actions into a data structure, and write a *loop* over them rather than repeating the control flow constructs `n` times. But how well this can be done depends on the language used. — Kilian Foth, Feb 22 '17 at 07:01
I don't know how others feel, but I dislike using short circuiting on methods that have side effects. Personal preference perhaps. — RubberDuck, Feb 22 '17 at 21:56
I like the way your third example's going, if you can make it work (the logic looks wrong, but the idea is sound). — Robert Harvey, Feb 22 '17 at 23:29
oh - so far it appears to work for me? Now I'm less confident. What was the issue you saw with the logic? — Chris O'Kelly, Feb 22 '17 at 23:36
It will always return the first iteration of the loop, unless the function that you hand to `.each` is a predicate (returns a boolean). The documentation says that function is a callback. Yeah, I guess I don't understand how that works. — Robert Harvey, Feb 22 '17 at 23:48
Oh right, so yeah, on the one hand I feel like me having to explain it to another developer means that it's not exactly ROC, but on the other hand any developers working on this should be experienced jQuery developers and so this should make more sense to them. Essentially returning false from the callback to `$.each` can be treated as a `break`, returning non-false can be treated as `continue`. — Chris O'Kelly, Feb 23 '17 at 00:34
OK, that makes sense. It wasn't clear to me from the jQuery documentation that that's how it worked, though admittedly I didn't read the whole thing. — Robert Harvey, Feb 23 '17 at 01:17

Doc Brown · Accepted Answer · 2017-02-22T21:13:46.243

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.

This is actually pretty close to what I have done at this point, except abstracted into a function (added to the question). Thanks! — Chris O'Kelly, Feb 22 '17 at 23:08

score 4 · Answer 2 · answered Feb 22 '17 at 08:46

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(...);

Note that the language is JavaScript (OP mentions using JSHint, which is a JavaScript linting tool), so the functional operations on arrays you suggest and first-class functions are both available and idiomatic. — Jules, Feb 22 '17 at 12:15
Haven't looked at `any` or `map` yet (maps are something I've used in PHP though), will have to do some further learning. The early return strategy had occurred to me, but the function in which these calls are made sets some other tags too and I was loathe to break it down any further into once-used functions with ridiculously long names. — Chris O'Kelly, Feb 22 '17 at 23:18

score 2 · Answer 3 · answered Feb 22 '17 at 07:17

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.

Essentially the issue to solve here is that initSingleTag is somewhat expensive, even just the part of it that checks validity. As a result, for tags like this where only one in the group will ever be valid I am trying to prevent checking the validity of every one of them if I already know that they can't be valid. — Chris O'Kelly, Feb 22 '17 at 23:22

Short circuit evaluation to do only the first of a number of actions

3 Answers3