It's working, why change it? - Code Review Chronicles

I am pretty sure it happened to you many times to not agree on a comment you received in your Merge/Pull Request.

Cmon, this comment is just about style. The code is working. why should I change it?
...and by the way. I like my style best.

When something like that happens, and we try and manage to let it happen very rarely (over time we all grew similar coding habit and styles - and many time we don't consider code style so important to block a ticket from being blocked by minor details), we normally stop the discussion and start a quick poll on slack.

Which snippet do you like the most?

Some of you might think of two kids arguing for a toy and calling for Mama - with the winner grinning full of pride afterwards, but it is actually a very democratical process that always spawns interesting things. ( and never lasts more than 5 minutes - while a discussion could go on over and over).

Recently - I received a Poll in Slack with this snippet, about loading modules based on a specific value.

A)

const adapter = context.animal === 'dog'
            ? require('./animal-adapters/dog')
            : context.animal === 'cat'
            ? require('./animal-adapters/cat')
            : require('./animal-adapters/fish')

vs
B)

let adapter
switch(context.animal) {
    case 'dog': adapter = require('./animal-adapters/dog'); break;
    case 'cat': adapter = require('./animal-adapters/cat'); break;
    case 'fish': adapter = require('./animal-adapters/fish'); break;
}

?

I usually like ternary operators because they are very useful for one-liners, but never use them when they are nested because I find them hard to read, on the other hand... I wasn't that happy with the switch either.

A separate function would allow more flexibility and return immediately without assigning the value - and also provide a default.

const getAdapter = (animal) =>{
    switch (animal ) {
        case 'dog':return  require('./animal-adapters/dog');
        case 'cat': return require('./animal-adapters/cat'); 
        case 'fish': return require('./animal-adapters/fish'); 
        default : return null;
    }
}
const adapter = getAdapter(context.animal)

Still, the main doubt was about the switch. And in fact, a colleague submitted immediately his solution:

I always wondered, why use switch if you can use a Map

const animals = new Map([
['cat', './animal-adapters/cat'], 
['dog', './animal-adapters/dog']
])
const module = animals.get(context.animal)
const adapter = require(module)

In the end, we all agreed that the ternary was not readable enough, the switch could be replaced by a Map, but to simplify even more it would have been enough an object:

const adapters = {
    dog: require('./animal-adapters/dog'),
    cat: require('./animal-adapters/cat'),
    fish: require('./animal-adapters/fish')
}

of course wrapped to some check for null properties and returning a default.

Was it worth it?

Sure, it was amazing to see how in less than 2 minutes we had 5 slightly different versions and it was funny seeing how everyone was iterating on the previous solutions to make it cleaner and more readable.

The original question was still open though:

It is valid and working code, why change it?

Well, unless performance requires super performant code, I prefer readability over everything else ( and ease of composition aside, this is also why I prefer chaining map, filter, reduce, etc rather than doing everything in one single big for-loop)

And to support my stance I always bring up the following famous quotes:

Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live.
John F. Woods 1991

Any fool can write code that a computer can understand. Good programmers write code that humans can understand.
Martin Fowler, 2008.

I believe that disagreements are something inherent with Code reviews. But we should always try - of course without getting lost on details or have holy wars on useless things - to be open for other people critics. - of course when they are respectful. We might or might not change our minds, but still, we can learn and grow out of the experience.

Comments

[Michael "lampe" Lazarski]

Code readability should always come before one-liners.

In general, one-liners are something that developers think is cool.
And yes to show-off they are but for maintainability, they are usually the worst kind of code.
Why?
1) Hard to debug
2) You need to much time to understand if after not seeing if for 2 weeks
3) They usually don't add any performance
4) Writing tests can be hard.

I don't get why devs hate switches?
So it is better to write a bunch of if's?

Languages are like a toolbox. Use the right tool for the right job!

[Davide de Paolis]

agree.
switches are not bad. and are better then ifs
but it's better to have maps/objects than switches because they can grow more organically without much changes in the code, avoid issues like fallthrough and missing returns/break, better performance (you access directly the value by name/prop not by comparing to all the cases) and overall readability.

[Davide de Paolis]

love the lazy evaluation with the arrow function!!

[Davide de Paolis]

omitted breaks are exactly why a map/object was the agreed choice here (against the switch). more readability and more safe.

but thanks for sharing your opinion :-)