DEV Community

Which would you prefer and why?

Omar Lozada on August 25, 2018

I recently wrote a small function. And thought of different ways of implementing it. Basically the function accepts a parameter and do string man...

Read full post

Adrian B.G. • Aug 25 '18

Either way I'll not use if for more than 2 options, and because the devs are usually lazy and do not refactor and just add a new option, I tend to never use it.

The answer depends a lot on the size of the do stuff code blocks.

1-2 liners I suggest using a switch
If there are more I would suggest using a map, similar to your 2nd version but it can be replaced with other functions. This is how actually some compilers transform the switch.

const handlers = {};
handlers[`option1`] = function(){ //do a lot of stuff }
handlers[`option2`] = function(){ //do a lot of stuff }
function (arg) {
  //if arg doesnt exists in handlers ..
  return handlers[arg]();
}

Because of the size of the function (now in the x * 10s) I would transform this function to a module, and declare the option functions outside of the main function.

The code can grow independently, the functions will not be "compiled" until they are called for the first time and they can be tested independently, if needed.

3 . 2 can be future extended, if needed to one of Plugin/Services/Inversion of control pattern, mainly to inject new functionalities in the map as more modules are active in your project.

Sergiu Mureşan • Aug 25 '18 • Edited

This is a great improvement over the second version.

The only issue I find is that the code for the function is no longer inside the function itself but outside. Maybe this will solve the issues:

let handlers;
function (arg) {
  if (!handlers) {
    handlers[`option1`] = function() { //do a lot of stuff }
    handlers[`option2`] = function() { //do a lot of stuff }
  }
  return handlers[arg];
}

Adrian B.G. • Aug 25 '18

Functions in functions I would say is smelly code, anyway if there are 5 options each with 7+ LOC then the main function will become too big, that is why I said to make a distinct object/module, with a size of 50+ LOC would probably required its own module. Then the functions being outside is ok, because they are encapsulated by the module.

Sergey Kislyakov • Aug 25 '18

The second example looks a lot nicer to me.

Note 1: you can use const instead of let in your second example because you do not transform the object anywhere in your function.

Note 2: second example will return undefined instead of '' if the key in the object does not exist. I think you'd handle that as well.

Shalvah • Aug 26 '18

*because you do not reassign the reference.

You can transform an object variable declared with const.

Omar Lozada • Aug 25 '18

Thanks for pointing out the difference between let and const, I'll keep that in mind.

Sergiu Mureşan • Aug 25 '18 • Edited

The first option is best here for 3 reasons:

It has better performance (only processes the option it lands on, not all of them)
More intuitive in the long run (if the function becomes much larger then it is hard to understand that the object optionResult is actually a switch or if else
You are returning an empty string if arg has an unexpected value, whereas in the second version you can't do that unless you check if optionResult doesn't have the property which further adds to the complexity of the function.

You can also straight up return the optionResult inside the if statements if no post-processing is needed.

Omar Lozada • Aug 25 '18

Thanks for the detailed response,
I agree that in the code above I haven't considered doing some validation.

Brett Stevenson • Aug 25 '18

I agree with the other responses that the second function looks much cleaner in this example, though it could also become less intuitive as the complexity of the logic that replaces // do stuff for option(n) increases.

simo • Aug 25 '18

Ternary Operator:

const foo = (arg) =>
  arg === 'option1'
  ? 'foo' // do stuff for option1

  : arg === 'option2'
  ? 'bar' // do stuff for option2

  : arg === 'option3'
  ? 'baz' // do stuff for option3

  : 'noop'

misbeliever • Aug 26 '18

That looks ugly

simo • Aug 26 '18

I had to say why do I prefer it and why, right?

Well, that's the most elegant way to do it, without creating an object just to return the value in the end.
It's better than using switch, no fall-through footgun here.
It's obviously more elegant than if-else-return.
Implicitly returns a value - essential for functional programming (that can be achieved with the object approach too).

While it might not be the prettiest code to see - it's what we currently have in the language, also it's pretty straightforward to understand.

dallgoot • Aug 26 '18

Let's split on the principles that describe the needs:

function checks 'arg' value among known values (implies constants)
function then process a treatment according to that value
if the value is unknown/invalid : fallback or error

function onOption(arg) {
    switch (arg) {
        case OPTION1 : return onOption1();
        case OPTION2 : return onOption2();
        case OPTION3 : return onOption3();
        default:
            return onUnknownOption()
    }
}

Results:

the accepted values are used through constants: consistency through code and a single place for modifications if need to be.
The function do a simple thing : branching against 'arg' value according to a set of known values (unless invalid).
Every specific process is encapsulated in a specific function: if a treatment is shared between option : another function can encapsulate it and make it reusable, where if it were inside the switch it would mean copy/pasting (or worse trying to factorize using IF). If "onOptionX" methods are pure, this is a pure function also.

I see often people trying to replace a switch with an object.
On the eye it's pleasant but in reality it's not using the simplest language construct to do the job, it's allocating memory for an object only to perform a string comparison.
Moreover many people don't take in account the "key not found" scenario leading to functions that errors or return undefined.

Richard Kichenama • Aug 27 '18

Using an object, where the keys are unique and translate to an action or value, is preferred because it reduces the complexity to a single path instead of multiple with switch or if. Can be externalized into a constant and have the function be

const Mutations = {
  'option1': ...,
};

function (arg) {
  const result = Mutations[arg];
  return (result && result) || `unknown option: ${arg}`;
}

Michael Salaverry • Aug 26 '18

If only one of the string manipulation functions should be run, then I would recommend ramdajs.com/docs/#cond which will only run the first condition which matches. The switch statement could run multiple times if a break or return is forgotten.

misbeliever • Aug 26 '18

2 option is much better, but the function still not the best in the best form , optionalResult is created everytime function is used, if its used multiple times, that is a waste of time, so it should be initialized out of scope, and best passed as an argument to the function to avoid sideefects.

Chris James • Aug 25 '18

Why not just switch?

developer.mozilla.org/en-US/docs/W...

Omar Lozada • Aug 25 '18

Yeah, been thinking of using switch statement.

It's just that for me it looked much harder to read at first compared to using if statement.

marcel0ll • Aug 25 '18

It depends on your goals with this code.
If there will be only a few options using if else or switch are in my opinion the best options, as they do the job without adding extra complexity, but as soon as you have more options and those functions start to grow I would refactor the code to use an extensible approach similar to your version 2.

pranay rauthu • Aug 27 '18 • Edited

This is one of the common choice which should be made by developer. It depends on the number of options, stuff you want to do for each option and more importantly how it effects code complexity. Check my comment here

Sadick • Aug 26 '18 • Edited

I use a general rule that if the options am supposed to handle exceeds three i use a switch otherwise i use an if statement. Switch was specifically made for such situations.

ComputerSmiths • Aug 25 '18

Rather than worry about the implementation, document it properly and it doesn’t matter how you implement it, no-one has to look at the code to determine what it does and make use of it. And the maintainers can write to the spec...

Haritian Pinio • Aug 25 '18

code readability sometimes is a necessity for teams with several members. I think the second is more readable. I had similar issues with 9-11 strings and I went with second option.

John Alcher • Aug 25 '18

The second one requires less thinking to be understood IMO, so I'd go with that route.

stereobooster • Aug 25 '18

I would use switch-case and exhaustive check