Discussion on: Refactoring IF, a real exercise

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.