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.