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.

const adapter = context.animal === 'dog' ? require('./animal-adapters/dog') : context.animal === 'cat' ? require('./animal-adapters/cat') : require('./animal-adapters/fish')
VS
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.

Originally published at https://dev.to on May 4, 2020.