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)

Improve this question
7
  • 2
    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. Commented Feb 22, 2017 at 7:01
  • I don't know how others feel, but I dislike using short circuiting on methods that have side effects. Personal preference perhaps. Commented Feb 22, 2017 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). Commented Feb 22, 2017 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? Commented Feb 22, 2017 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. Commented Feb 22, 2017 at 23:48

3 Answers 3

Reset to default
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.

Improve this answer
1
  • This is actually pretty close to what I have done at this point, except abstracted into a function (added to the question). Thanks! Commented Feb 22, 2017 at 23:08
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(...);
Improve this answer
2
  • 2
    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. Commented Feb 22, 2017 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. Commented Feb 22, 2017 at 23:18
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.

Improve this answer
1
  • 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. Commented Feb 22, 2017 at 23:22

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.