1

I'm using JavaScript but the question can be generalized to all the languages.

In a nutshell - I'm checking if a browser connecting to my site is a microwave and cater for that accordingly.

What would be the best way to structure my code?

Best way is in "most readable", "most maintainable" (insert your own metric here)...

Option 1.0

var iammicrowave = /(microwave)/.test(navigator.userAgent);
if (iammicrowave) {
    var settings = { blah : 42 };
    magicFunction(settings);
} else {
    magicFunction();
}

Option 1.1

var iammicrowave = /(microwave)/.test(navigator.userAgent);
if (iammicrowave) {
    magicFunction({ blah : 42 });
} else {
    magicFunction();
}

Option 2.0

var iammicrowave = /(microwave)/.test(navigator.userAgent);
var settings;
if (iammicrowave) {
    settings = { blah : 42 };
}
magicFunction(settings);

Option 2.1

var iammicrowave = /(microwave)/.test(navigator.userAgent);
var settings = imamicrowave ? { blah : 42 } : undefined;
magicFunction(settings);

Option 2.1.1

var iammicrowave = /(microwave)/.test(navigator.userAgent);
var settings = imamicrowave ? { blah : 42 } : {};
magicFunction(settings);

Option 2.2

var iammicrowave = /(microwave)/.test(navigator.userAgent);
magicFunction(imamicrowave ? { blah : 42 } : {});

Option 2.3

magicFunction(/(microwave)/.test(navigator.userAgent) ? { blah : 42 } : {});

Many thanks!

closed as primarily opinion-based by TMN, durron597, user40980, user53019, Ixrec Oct 16 '15 at 18:53

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • Note to others: Related post on Code Review meta: meta.codereview.stackexchange.com/q/6080/31562 – Simon Forsberg Oct 15 '15 at 10:20
  • Thank you... Should have mentioned that. I've made an attempt - "is code review the right place to ask" (the answer is NO) - so I ask here... – Mars Robertson Oct 15 '15 at 10:22
  • I think this is a highly subjective area. Personally, I like 2.2 the best, but that's just a personal preference; it certainly isn't a definitive answer. – David Arno Oct 15 '15 at 10:23
  • @GlenPeterson - I believe your comment qualifies as an answer. And don't get too attached to the function name, in real-life it is called _initialLoad, see screenshot: i.imgur.com/yOTq0Br.png – Mars Robertson Oct 15 '15 at 14:05
  • 1
    StackExchange isn't really the place for opinion polls like this. None of these are objectively better than the others (and my own preference is for something that's not even on this list!). – Ixrec Oct 16 '15 at 18:53
3

For something like this where there isn't much meat to the code, the most readable way is the way that is most consistent with the idioms of your language and consistent with your codebase (probably in that priority). The bugs will come when people read the code assuming it does one thing, but your code is different. The readability issues come when people read the code assuming it is one thing, but your code is different.

In a vacuum, I like none of these. Instead:

if(/(microwave)/.test(navigator.userAgent)) {
  magicFunction({blah: 42});
} else {
  magicFunction();
}

It allows you to break in the conditional. It doesn't make local vars which make the reader worried that the value is used later. It makes each branch explicit, which makes it easier to change in the future.

For conditionals that are less clear, I would split them into their own well named function rather than using a local variable for that since less clear logic tends to need reusing and/or testing in isolation.

1

You should not repeat the function call since it is always executed anyway. So 1.0 and 1.1 is out.

The distinction between 2.1 and 2.1.1 is really a question of the contract of the called function. The function might interpret undefined and {} differently, and we can't tell what is correct without knowing the contract of the function. I find option 2.1.1 clearer than option 2.0 and 2.1, but is is also semantically different so hard to say what is correct.

Should you have settings and iammicrowave as variables or just have the expressions inline as in 2.3? I find 2.3 nearly unreadable, so I would argue you should. Especially the variable name iammicrowave makes it a lot clearer what is going on.

  • Thank you for the answer. I also like iammicrowave as a separate variable but for completeness decided to include. In the magicFunction I'm using something like settings = $.extend(settings, {}); so either undefined or {} (empty object) is the same. – Mars Robertson Oct 15 '15 at 11:35
  • In some languages (the equivalent of) magicFunction({blah: 42}) and magicFunction() actually refer to different, overloaded functions, and in that case you can't necessarily consolidate the two calls in one. – Jacob Sparre Andersen Oct 15 '15 at 17:27
1

I think giving magicFunction() a better name would do more help readability than any of the other options you present. Or make a well named wrapper function that constructs the settings map and passes it to magicFunction(). I don't think the iammmicrowave variable is strictly necessary or adds that much to readability.

In a language with types you could also name your settings data type, formalize its allowable keys, and write a description of it in comments where it is defined. That might help too, but I still think a more descriptive function name would help the most.

Not the answer you're looking for? Browse other questions tagged or ask your own question.