Extract rotation values from css string Javascript

Question

I have the following function that takes out the rotation values from a css string and returns them as an array.

var extractRotationValues = function(code) {
  var getAllIndexes = function(arr, val) {
    var indexes = [],
      i = -1;
    //To understand the second parameter of indexOf function
    //have a look at this : https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/indexOf
    while ((i = arr.indexOf(val, i + 1)) != -1) {
      indexes.push(i);
    }
    return indexes;
  };
  var indexes = [];
  var closingIndexes = getAllIndexes(code, ")")
  var values = [];
  var rindexes = [];
  rindexes.push(code.indexOf("X("), code.indexOf("Y("), code.indexOf("Z("));
  //No value for rotateX was found so consider it 0
  if (rindexes[0] == -1)
    values.unshift(0);
  else {
    //We have a value for rotateX
    //Where does rotateX start from => rindexes[0]
    //Where does it end? =>closingIndexes[0]
    var value = code.substr(rindexes[0], closingIndexes[0]).split("(")[1].split("deg)")[0];
    values.push(value)
  }
  //No value for rotateY
  if (rindexes[1] == -1)
    values.splice(1, 0, 0);
  else {
    //We have a value for rotateY
    //Where does rotateY start from => rindexes[1]
    //Where does it end? =>closingIndexes[1]
    values.push(code.substr(rindexes[1], closingIndexes[1]).split("(")[1].split("deg)")[0])
  }
  //No value for rotateZ
  if (rindexes[2] == -1)
    values.splice(2, 0, 0);
  else {
    //We have a value for rotateZ
    //Where does rotateZ start from => rindexes[2]
    //Where does it end? =>closingIndexes[2]
    var zvalue = code.substr(rindexes[2], closingIndexes[2]).split("(")[1].split("deg)")[0]
    values.push(zvalue);
  }
  return values;
};

console.log(extractRotationValues("rotateX(90deg) rotateY(87deg) rotateZ(22deg)"))

I was wondering.Is there anything that can be done in a better way here or something to improve?

Answer 1

First: A little regexp will do ya just as well, I'd imagine. Something like:

function extractRotationValues(string) {
  var values = {x: 0, y: 0, z: 0};
  string.replace(/\brotate([XYZ])\s*\(\s*([+-]?[\d.]+)\s*deg\s*\)/g, function (_, axis, value) {
    values[axis.toLowerCase()] = parseFloat(value);
  });
  return values;
}

Note that this does a couple of things differently:

1. parses the string values into Number values
2. returns an object with x, y, and z properties rather than an array (just because I feel that that's more useful - it can be made to return an array, though)

Basically it looks for rotate[name of axis]([a number]deg) (while skipping over possible whitespace - the \s* stuff - that might be in the string) and captures the axis name and number. Despite the use of replace nothing's actually replaced (or rather we don't care about replacing things). It's just that String.replace takes a function as its 2nd argument, which gets passed the matched/captured substrings, which sort of turns replace into a "scan"-like function.

Anyway, that's what I'd do. Not saying it's bulletproof (spent a minute writing it, and robustly matching a number can be more complicated), but it's certainly less code.

Your code works too, of course, though the lower-level approach of finding indices and slicing and dicing strings is less elegant in my eyes. But it's a valid approach nonetheless.

There are however a few things I want to comment on:

• I'd encourage you to always use strict equals (=== and !==). Just because it's a good habit to get into.

• closingIndexes is a nice descriptive name by itself, but it gets confusing when you also have rindexes where you've abbreviated, and just plain indexes (which isn't actually used). I'd give all of them equally descriptive names, instead of having one that's descriptive, one that's slightly cryptic, and one that's generic. (I also tend to favor indices for the plural form, but "indexes" is correct English too.)

• Why are default zero values inserted into the array in a different way than extracted/found values? The found values you just insert with push, which makes a lot of sense. But when you want to insert 0 for x, y, or z, you use unshift or splice? Why not use push in all cases, as that's the simplest?
  I'd rather have a branch that determines the value to insert, and a common push(value) call after that, than branch the array-insertion itself, because in both cases you want to append some value. The branching should be about what to insert, because how and where to insert it is always the same.

• Incidentally, if you find a value, you insert it as a string. If you don't find one, you insert zero as a number. Don't mix types like that; keep it consistent. If you'd used null or undefined instead of zero to denote "no value", that'd be fine, but here you're essentially "making up" a rotation that's not present in the CSS string, so it's not "no value", it's "fallback value". And thus it should be the same type as a non-fallback value and vice-versa.

• Your code can stumble on malformed CSS. E.g. rotateX(42deg rotateY(23deg) (note the missing closing parenthesis) won't be parsed right. It'll find 42deg rotateY as the value for x-axis rotation, which you might say is better than nothing. However, the CSS was malformed, so technically speaking there is no x-axis rotation; nothing would actually be better.

• Most importantly: Why write 3 separate if-else blocks when all that changes in them is an index? That's what loops are for. Write a regular for loop, or call forEach on rindexes, e.g.:

  rindexes.forEach(function (rindex, i) {
    var value = 0;
    if(rindex !== -1) {
      value = code.substr(rindex, closingIndexes[i]).split("(")[1].split("deg)")[0];
    }
    values.push(value);
  });