2
\$\begingroup\$

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?

\$\endgroup\$
1
\$\begingroup\$

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);
    });
    
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ As for why I use splice.I need to push a value into a specific position in the array \$\endgroup\$ – cssGEEK Jan 14 '17 at 19:30
  • \$\begingroup\$ @cssGEEK No, you don't. You're going through x, y, and z in order, so the zero just has to go at the end. If you did need to put the value into a position other than the end of the array, then the code wouldn't work for the else/found-value cases, where you use push instead. Again, the if-else branches aren't about how or where to insert something into the array, they're about what to append/push: Zero or found value. \$\endgroup\$ – Flambino Jan 14 '17 at 19:33
  • \$\begingroup\$ By the way what is the _ parameter in the function after the replace? \$\endgroup\$ – cssGEEK Jan 14 '17 at 19:33
  • \$\begingroup\$ @cssGEEK _ is a somewhat common/conventional name for "I'm not using this argument for anything". Could call it ignored or something instead. Point is, the function is only interested in the 2nd and 3rd arguments, but you have to use some name for the 1st one too, even if you don't care about it. And _ is the conventional name (though it gets confusing if you're also using the underscore.js or lodash libraries, so avoid it if that's the case) \$\endgroup\$ – Flambino Jan 14 '17 at 19:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.