It’s working, why change it? — Code Review Chronicles

Image for post
Image for post

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.

Image for post
Image for post

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')VSlet 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

Image for post
Image for post

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.

Written by

Sport addicted, productivity obsessed, avid learner, travel enthusiast, expat, 2 kids. Technical Lead (NodeJs Serverless)

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store