Refactoring IF, a real exercise

Introduction

Hello, this article is based on a real refactor situation of a javascript software argentine team.

At work we found a code aka "legacy", this made the team of developers go crazy 😲 😱.

There was only one way to solve this, a refactoring competition 😎💪.

The challenge

Five developers were the competitors:

• @eddovj
• @pabsource
• @adelysalberto
• @cris_198422
• @pascualmj

and many other spectators.

The goal was to refactor this evil code:

const get_data_type = code => {

    if (
        code === 702 ||
        code === 1082 ||
        code === 1083 ||
        code === 1114 ||
        code === 1184 ||
        code === 1266 ||
        code === 12403
    ) {
        return "DATE";
    } else if (
        code === 20 ||
        code === 21 ||
        code === 23 ||
        code === 24 ||
        code === 26 ||
        code === 700 ||
        code === 701 ||
        code === 790 ||
        code === 1700 ||
        code === 2202 ||
        code === 2203 ||
        code === 2204 ||
        code === 2205 ||
        code === 2206 ||
        code === 3734 ||
        code === 3769 ||
        code === 12396
    ) {
        return "NUMERIC";
    } else {
        return "STRING";
    }
};

Evaluation points:

• semantics
• performance

The contest prize

None just the HONOR and respect from all and and the opportunity to participate in this article of dev.to, because we don't have a budget for these things hahaha.

3, 2, 1, go!

These were the proposals presented by the participants:

@pabsource - Pablo:

const codeDate = [20, 21, 23, 24, 26, 700, 701, 790, 1700, 2202, 2203, 2204, 2205, 2206, 3734, 3769, 12396];
const codeNumeric = [702, 1082, 1083, 1114, 1184, 1266, 12403];
const codeInput = 505;

const get_data_type = (codeInput)=>{
  const codeResponse = [codeInput].reduce((acc, item) => {
    const date = codeDate.includes(item) && 'DATE'
    const numeric = codeNumeric.includes(item) && 'NUMERIC'
    return date ? date : numeric || acc
  }, 'STRING');
}

Analysis:
He creates an array and a reducer and includes to get the value of the
type and then a ternary to return a default.

Semantic:
It has a complicated semantics implies mastering the use of reduces and includes.

TIME:
get_data_type(20); - 0.068115234375 ms
get_data_type(702);- 0.008056640625 ms
get_data_type(0); - 0.0068359375 ms

@adelysalberto - Adelys:

const codeNumeric = [702, 1082, 1083, 1114, 1184, 1266, 12403];
const codeDate = [20, 21, 23, 24, 26, 700, 701, 790, 1700, 2202, 2203, 2204, 2205, 2206, 3734, 3769, 12396];
const get_data_type = (code) => {
  const codeNum = codeNumeric.includes(code) && 'NUMERIC';
  const codeD = !codeNum && codeDate.includes(code) && 'DATE';
  return codeNum ? codeNum : codeD || "STRING"
}

Analysis:
He use two includes to get the data type, and a single ternary to return the default.

Semantic:
It has a simpler semantics, it uses only the same operator and a ternary, the default is understood at first glance.

TIME:
get_data_type(20); - 0.041015625 ms
get_data_type(702);- 0.00390625 ms
get_data_type(0); - 0.00390625 ms

@eddovj - Edu:

const codeNumeric = [702, 1082, 1083, 1114, 1184, 1266, 12403];
const codeDate = [20, 21, 23, 24, 26, 700, 701, 790, 1700, 2202, 2203, 2204, 2205, 2206, 3734, 3769, 12396];
const get_data_type = (code) => codeDate.includes(code)?(
        'Date' 
        ): (
            codeNumeric.includes(code) ? 'Numeric' : 'String'
        );

Analysis:
He two ternarys one as main validator and other to return the default.

Semantic:
It implies knowing includes, it makes a more intensive user of the ternary generates confusion if it is not mastered.

TIME:
get_data_type(20); - 0.034912109375 ms
get_data_type(702);- 0.001953125 ms
get_data_type(0); - 0.003173828125 ms

@cris_198422 - Cris:

const dates = [20, 21, 23, 24, 26, 700, 701, 790, 1700, 2202, 2203, 2204, 2205, 2206, 3734, 3769, 12396];
const numerics = [702, 1082, 1083, 1114, 1184, 1266, 12403];
const get_data_type = (code) =>
  dates.includes(code)
    ? 'DATE'
    : numerics.includes(code)
    ? 'NUMERIC'
    : 'STRING';

Analysis:
He use a firts ternary and another ternary to return the default.

Semantich:
Solution in a line is read as a sentence, it implies mastering concatenated ternaries, it is a little more semantic than the previous one.

TIME:
get_data_type(20); - 0.03369140625 ms
get_data_type(702);- 0.0048828125 ms
get_data_type(0); - 0.032958984375 ms

@pascualmj - Manu:
He did'nt present a code but communicate an idea, the code was written by me, as an exception it is accepted for this article.

const NUMBER='NUMERIC';
const DATE='DATE';
const dataValues = {
  20:DATE,21:DATE,23:DATE,24:DATE,26:DATE,700:DATE,701:DATE,790:DATE,
  1700:DATE,2202:DATE,2203:DATE,2204:DATE,2205:DATE,2206:DATE,3734:DATE,
  3769:DATE,12396:DATE,702:NUMBER,1082:NUMBER,1083:NUMBER,1114:NUMBER,
  1184:NUMBER,1266:NUMBER,12403:NUMBER
};

const get_data_type = (code)=>dataValues[code]||'STRING';

Analysis:
Well this is a very different solution, I have decided not to use includes instead it creates a map to avoid doing array operations and the default is when a value is not found in the map.

Semantic:
Easy to understand solution does not imply knowing array prototypes, only the operator in the default.

TIME:
get_data_type(20); - 0.02587890625 ms
get_data_type(702);- 0.0029296875 ms
get_data_type(0); - 0.002197265625 ms

Conclusion:

Comparing all the cases, the more performance codes:

Code	Author	Time
get_data_type(20);	manu	0.02587890625 ms
get_data_type(702);	edu	0.001953125 ms
get_data_type(0);	manu	002197265625 ms

The most efficient solution is the one that uses a map to obtain the results created by @pascualmj with a good semantic.

The solution created by @cris_198422 and @eddovj they have a good performance too.

The solution created by @adelysalberto have a good performance for date values but suffer in the default and the numeric scenario, but I think that not having used ternary concatenation and a simple default like the includes gives you the best semantic.

Learnings:

• Use a object map or the JS MAP for this type of scenario.
• Avoid using unnecessary array operations.
• Avoiding the if / else with two ternarys is efficient but we lose semantics.
• Concatenating ternaries can be dangerous if you do not master their use.

Comments

[Ben Sinclair]

Refactoring isn't about improving performance. I suspect that differences on the order of microseconds will not affect anything in anyone's Javascript. Unless they're crunching numbers for hours (why would someone do that in Javascript?) nobody will ever notice, even if you have a thousand calls and refactor a hundred functions.

Let me have a go at reviewing the refactors :)

const codeDate = [20, 21, 23, 24, 26, 700, 701, 790, 1700, 2202, 2203, 2204, 2205, 2206, 3734, 3769, 12396];
const codeNumeric = [702, 1082, 1083, 1114, 1184, 1266, 12403];
const codeInput = 505;

const get_data_type = (codeInput)=>{
  const codeResponse = [codeInput].reduce((acc, item) => {
    const date = codeDate.includes(item) && 'DATE'
    const numeric = codeNumeric.includes(item) && 'NUMERIC'
    return date ? date : numeric || acc
  }, 'STRING');
}

This is way less readable to me than the original. The purpose of refactoring is to make something easier to maintain, and while this does get that partially (the arrays at the top of the function mean it's easier to scan through the numbers) the reduce takes brainwork to decipher.

For people coming from another language, the omission of semicolons in a callback function that uses ternaries and short-circuits makes it even harder to read.

Next up:

const codeNumeric = [702, 1082, 1083, 1114, 1184, 1266, 12403];
const codeDate = [20, 21, 23, 24, 26, 700, 701, 790, 1700, 2202, 2203, 2204, 2205, 2206, 3734, 3769, 12396];
const get_data_type = (code) => {
  const codeNum = codeNumeric.includes(code) && 'NUMERIC';
  const codeD = !codeNum && codeDate.includes(code) && 'DATE';
  return codeNum ? codeNum : codeD || "STRING"
}

I'm going to immediately call out the variable names. CodeD? What is that? Is it the same as CodeDate or perhaps it means, "decimal"? If I don't know the codebase, I'm left picking this apart to find out what it does.

Next:

const codeNumeric = [702, 1082, 1083, 1114, 1184, 1266, 12403];
const codeDate = [20, 21, 23, 24, 26, 700, 701, 790, 1700, 2202, 2203, 2204, 2205, 2206, 3734, 3769, 12396];
const get_data_type = (code) => codeDate.includes(code)?(
        'Date' 
        ): (
            codeNumeric.includes(code) ? 'Numeric' : 'String'
        );

This is a strangely-formatted ternary. The choice of whitespace makes it difficult to read, especially around the first ? which is different to the second, and having ):( on its own line is awkward. I don't like double-ternaries because there's always a moment of "is this going to return the first or second one?" that you have to go through. If you think it's obvious, remember it's not you who wrote it and might be 10 years old!

Oh, and it'll also fail any tests since it returns values in title case rather than all caps...

Next:

const dates = [20, 21, 23, 24, 26, 700, 701, 790, 1700, 2202, 2203, 2204, 2205, 2206, 3734, 3769, 12396];
const numerics = [702, 1082, 1083, 1114, 1184, 1266, 12403];
const get_data_type = (code) =>
  dates.includes(code)
    ? 'DATE'
    : numerics.includes(code)
    ? 'NUMERIC'
    : 'STRING';

This is the same as the previous solution except it has better formatting and is immediately easier to read even if it's still not quite as simple as the original.

Lastly:

const NUMBER='NUMERIC';
const DATE='DATE';
const dataValues = {
  20:DATE,21:DATE,23:DATE,24:DATE,26:DATE,700:DATE,701:DATE,790:DATE,
  1700:DATE,2202:DATE,2203:DATE,2204:DATE,2205:DATE,2206:DATE,3734:DATE,
  3769:DATE,12396:DATE,702:NUMBER,1082:NUMBER,1083:NUMBER,1114:NUMBER,
  1184:NUMBER,1266:NUMBER,12403:NUMBER
};

const get_data_type = (code)=>dataValues[code]||'STRING';

This does something I like: it takes the string return value and assigns it to something else. Apart from that it's a wall of text and repetition, and I don't think it'll help (or scale!).

All of these extract the data and put it in global (or at least parent) scope. That means they're potentially polluting someone else's scope, and they have generic names like "dates" which is almost guaranteed to conflict with something.

Refactoring this can't really be done in isolation. You need a constant or something to represent your type rather than a string literal, because it's much easier to spot typos, use an IDE to rename across a project, etc.

What this function asks me is, "where do these numbers come from? What do they represent? Will they ever change? Should this whole thing be an object?"

Code should be readable and names like "code" and "data" are mostly meaningless. If you can't rename the function to something more specific, then it's a good place for a comment, I guess.

[Patrick]

I totally agree with you. The refactorings are much harder to read compared to the original code.

[Damian Cipolat]

It is true, a lot of semantics was lost, I say it above in the conclusions

[Clicker]

Great! Nice analysis

[🐤🥇 Jasper de Jager]

My take on this.

const codes = {
    "DATE": [702, 1082, 1083, 1114, 1184, 1266, 12304],
    "NUMERIC": [20, 21, 23, 24, 26, 700, 701, 790, 1700, 2202, 2203, 2204, 2205, 2206, 3734, 3769, 12396]
};

const get_data_type = code => Object.entries(codes).find(
    ([checkType, checkCodes]) => checkCodes.includes(code)
)?.[0] || 'STRING'

[Manuel Pascual]

Nice article! It's a shame that arrived late to post my solution. Anyway, thanks a lot for tagging me 😃 See you @ work 💻