2

How to make those javascript statements to look more readable. Could functional library ramda.js be used to make this code look better?

var getTextSpace =  function(len)
    {
            var tlength;
            if (len >= 1 && len <= 4) {
                tlength = 10;
            } else if (len === 5) {
                tlength = 14;
            } else if (len === 6) {
                tlength = 16;
            } else if (len === 7) {
                tlength = 18;
            } else if (len >= 8 && len <= 10) {
                tlength = 20;
            } else if (len === 11) {
                tlength = 22;
            } else if (len === 12) {
                tlength = 24;
            } else if (len >= 13 && len <= 15) {
                tlength = 26;
            } else if (len === 16) {
                tlength = 28;
            } else if (len >= 17 && len <= 20) {
                tlength = 32;
            } else if (len >= 21 && len <= 34) {
                tlength = tlength * 2;
            } else if (len >= 35 && len <= 80) {
                tlength = Math.round((len + len / 100 * 50));
            }
            else {
                tlength = Math.round((len + len / 100 * 30));
            }
        return tlength;
    };

Thank you in advance.

Maybe it is possible to do something that will allow this ?

   value
     .between(2,20).then(20)
     .between(21,22).then(0)
     .greater(25).then(25))
     .less(30).then(function(value) {return value * 20 )})
Ant
  • 115
  • 6
  • 1
    I'm voting to close this question as off-topic because it it probably belongs on http://codereview.stackexchange.com/ – Andy Dec 02 '15 at 16:41
  • sure, but there you wont get any answers. but I will try and come here back. – Ant Dec 02 '15 at 16:52
  • `if (len >= 21 && len <= 34) tlength = tlength * 2;` should be `tlength = tlength * 2;`, shouldn't it? – Bergi Dec 02 '15 at 18:59
  • Yes, this is a bug. proves the code difficult to read. – Ant Dec 03 '15 at 09:00

6 Answers6

6

Ramda might help a bit. But the main thing is to structure your ranges in a readable manner. The code below assumes that the input values are integers and you don't need to test other numeric types. Those could be done, but then you'd need something more sophisticated than the simple between here. You'd need either multiple functions or a way to configure the one to determine whether each of the beginning and the end are inclusive or exclusive.

var getTextSpace =  (function() {
  // :: (Int, Int) -> (Int -> Bool)
  var between = (begin, end) => R.both(R.gte(R.__, begin), R.lt(R.__, end));
  return R.cond([
    [between(1, 5), R.always(10)],
    [between(5, 6), R.always(14)],
    [between(6, 7), R.always(16)],
    [between(7, 8), R.always(18)],
    [between(8, 11), R.always(20)],
    [between(11, 12), R.always(22)],
    [between(12, 13), R.always(24)],
    [between(13, 16), R.always(26)],
    [between(16, 17), R.always(28)],
    [between(17, 21), R.always(32)],
    [between(21, 35), R.multiply(2)], // assuming original was typo
    [between(35, 80), len => Math.round(len + len / 100 * 50)],
    [R.T, len => Math.round(len + len / 100 * 30)]
  ]);
}());

(There seems to bug in the original case:

        } else if (len >= 21 && len <= 34) {
            tlength = tlength * 2;

which I assume meant

        } else if (len >= 21 && len <= 34) {
            tlength = len * 2;

and I coded the equivalent here.)

You can see this in action on the Ramda REPL.

Scott Sauyet
  • 49,207
  • 4
  • 49
  • 103
2

function getTextSpace(len) {
  // If len falls within a range then return that length
  var map = [
    [1, 4, 10],
    [5, 5, 14],
    [6, 6, 16],
    [7, 7, 18],
    [8, 10, 20],
    [11, 11, 22],
    [12, 12, 24],
    [13, 15, 26],
    [16, 16, 28],
    [17, 20, 32]
  ];


  for (var i = 0; i < map.length; i++) {
    var range = map[i];
    if (len >= range[0] && len <= range[1]) {
      return range[2];
    }
  }

  // We didn't find a range so return return calculation
  // for the following ranges.
  if (len >= 21 && len <= 34) {
    return len * 2;
  } else if (len >= 35 && len <= 80) {
    return Math.round((len + len / 100 * 50));
  }

  // Return this calculation for everything else.
  return Math.round((len + len / 100 * 30));
}

function test() {
  var out = document.getElementById("out");
  var text = "";

  for (var i = 0; i < 100; i += 3) {
    text += i + ": " + getTextSpace(i) + "\n";
  }
  out.innerHTML = text;
}

test();
<pre id="out"></pre>
wolfhammer
  • 2,641
  • 1
  • 12
  • 11
1

You could use a switch statement to avoid all the else if statements.

Additionally, if len is always an integer you could put the tlengths in an Array where the index matches the value of len:

var getTextSpace = function(len) {

var tlengthArray = [10,10,10,10,14,16,18,20,20,20,22,24,26,26,26,28,32,32,32,32, len*2, Math.round((len + len / 100 * 50)), Math.round((len + len / 100 * 50))];

var tlength;

if (len >= 1 && len <=20) {
    tlength = tlengthArray[len-1];
}
else if (len >= 21 && len <= 34) {
    tlength = tlengthArray[20];
}
else if (len >= 35 && len <= 80) {
    tlength = tlengthArray[21];
}
else {
    tlength = tlengthArray[22];
}

return tlength;

}
digglemister
  • 1,477
  • 3
  • 16
  • 31
  • maybecould be easy to do somehing to be able to do: value .between(2,20).then(20) .between(21,22).then(0) .greater(25).then(25)) .less(30).then(function(value) {return value * 20 )}) .... ) – Ant Dec 02 '15 at 17:04
  • Sorry, not sure what you mean. – digglemister Dec 02 '15 at 17:09
1

Ramda is very functional, which means that the best use of it is using as many declarative and pure functions (generic functions, that can be used in many places, not just your code). My suggestion would be something like this code:

var getTextSpace = function (len) {
  var conds = [
      {range: [1, 4], result: 10},
      {range: [5, 5], result: 14},
      {range: [6, 6], result: 16},
      {range: [7, 7], result: 18},
      {range: [8, 10], result: 20},
      {range: [11, 11], result: 22},
      {range: [12, 12], result: 24},
      {range: [13, 15], result: 26},
      {range: [16, 16], result: 28},
      {range: [17, 20], result: 32},
      {range: [21, 34], result: len * 2}, // You wrote tlength * 2 but it's not defined yet so I asumed you ment len * 2
      {range: [35, 80], result: Math.round((len + len / 100 * 50))}
  ];

  var test = function (obj) {
    var rangeLens = R.lensProp('range');
    var range = R.view(rangeLens, obj);

    var lte = R.curry(R.lte)(range[0]);
    var gte = R.curry(R.gte)(range[1]);
    return R.both(lte, gte)(len);
  }

  var resultLens = R.lensProp('result');
  var getResult = R.curry(R.view)(resultLens);

  var chosen = R.find(test)(conds);
  var defIfNotFound = R.defaultTo( {result: Math.round((len + len / 100 * 30))} );

  return getResult(defIfNotFound(chosen));

};

I tried to give each function a name that explains what it does, and break them into many parts, which made it almost like reading a sentence

Someonation
  • 175
  • 10
0

Maybe switchis an alternative. There is a post with quit a similar topic.

Have look right here.

Community
  • 1
  • 1
frank
  • 1,217
  • 2
  • 10
  • 18
0

If you want a vanilla JS solution this maybe an alternative.

const isBetween = x => (s, e) =>
 (Number(s) <= Number(x) && Number(x) <= Number(e))
 ? true : false

const getTextSpace = len => {
  const lenIsBetween = isBetween(len)
  return lenIsBetween(1,4)? 10
  : lenIsBetween(5, 5)    ? 14
  : lenIsBetween(6, 6)    ? 16
  : lenIsBetween(7, 7)    ? 18
  : lenIsBetween(8, 10)   ? 20
  : lenIsBetween(11, 11)  ? 22
  : lenIsBetween(12, 12)  ? 24
  : lenIsBetween(13, 15)  ? 26
  : lenIsBetween(16, 16)  ? 28
  : lenIsBetween(17, 20)  ? 32
  : lenIsBetween(21, 34)  ? len * 2
  : lenIsBetween(35, 80)  ? Math.round((len + len / 100 * 50))
  : Math.round((len + len / 100 * 30))
}
John Swindin
  • 79
  • 1
  • 1