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

Davide de Paolis - May 4 '20 - - Dev Community

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.

go on arguing, just wait I get some popcorns

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')
Enter fullscreen mode Exit fullscreen mode

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;
}
Enter fullscreen mode Exit fullscreen mode

?

Fight!

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)
Enter fullscreen mode Exit fullscreen mode

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)
Enter fullscreen mode Exit fullscreen mode

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')
}
Enter fullscreen mode Exit fullscreen mode

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

Toc, toc.  Did you write this code some time ago?

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.

. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
Terabox Video Player