It's working, why change it? - Code Review Chronicles
Tags: codequality, beginners, codereview, javascript
Published: 2020-05-04 08:03:20

---
title: It's working, why change it? - Code Review Chronicles
published: true
cover_image: https://dev-to-uploads.s3.amazonaws.com/i/dp0r3k5i4ebj1vjik20m.png
description: what do you do when you disagree with a comment during a code review?
tags: #codequality, #beginners, #codereview, #javascript
---

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](https://media.giphy.com/media/t3dLl0TGHCxTG/giphy.gif)


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;
}
```
?

![Fight!](https://dev-to-uploads.s3.amazonaws.com/i/z7dn5arl0w4wfv0b69bl.gif)


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

![Toc, toc.  Did you write this code some time ago?](https://media.giphy.com/media/k3ysanIoI6Yx2/giphy.gif)

> 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.