1
\$\begingroup\$

I have a function that creates a css transform property string based on some global variables. This string can be copied later by a user, so I would prefer to keep the output short.

This is my code:

var createTransformation = () => {
        var final_transformation = "";
        if (x)
            final_transformation += "rotateX(" + x + "deg)";
        if (y)
            final_transformation += " rotateY(" + y + "deg)";
        if (z)
            final_transformation += " rotateZ(" + z + "deg)";
        if (scalex !== 1 || scaley !== 1)
            final_transformation += " scale(" + scalex + "," + scaley + ")";
        if (translatex !== 0 || translatey !== 0 || translatez !== 0) {
            if (translatex !== 0 && translatey == 0 && translatez == 0)
                final_transformation += " translateX(" + translatex + "px)";
            else if (translatey !== 0 && translatex == 0 && translatez == 0)
                final_transformation += " translateY(" + translatey + "px)";
            else if (translatez !== 0 && translatex == 0 && translatey == 0)
                final_transformation += " translateZ(" + translatez + "px)";
            else if (translatex == 0)
                final_transformation += " translateY(" + translatey + "px) translateZ(" + translatez + "px)";
            else if (translatez == 0)
                final_transformation += " translateX(" + translatex + "px) translateY(" + translatey + "px)";
            else if (translatey == 0)
                final_transformation += " translateX(" + translatex + "px) translateZ(" + translatez + "px)";
            else
                final_transformation += " translateX(" + translatex + "px) translateY(" + translatey + "px) translateZ(" + translatez + "px)";
        }
        //if scalex != 1 || scaley != 1 
        //If the skew's sliders have been changed e.g, moved from 0 to 1 or from 0 to 100 add skew property
        if (skewX || skewY)
            final_transformation += " skew(" + skewX + "deg," + skewY + "deg)";
        return final_transformation;
    };

As you can see there are a lot of if statements. I was wondering, is there anything I can do to improve this code?

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

I would strongly consider storing the various variables that define the transformation in a structured object like this:

let transformConfig = {
    rotate: {
        x: ...,
        y: ...,
        z: ...
    },
    scale: {
        x: ...,
        y: ...
    },
    translate: {
        x: ...,
        y: ...,
        z: ...
    },
    skew: { 
        x: ...,
        y: ...
    }
};

That gives you the ability to easily iterate over this object to create transformation strings. An example of this iteration might be:

function createTransformString(transformConfig) {
    // a variable to store array of individual transform strings
    var transformStrings = [];

    // store functions that build strings for each transform type
    // not shown here but you may want to consider ECMA6 templating
    var buildString = {
        rotate: function(rotate) {
            for (dim in rotate) {
                transformStrings.push( 'rotate' + dim + '(' + rotate[dim] + 'deg)' );
            }
        },
        scale: function(scale) {
            scale.x = scale.x || 1;
            scale.y = scale.y || 1;
            transformStrings.push( 'scale(' + scale.x + ',' + scale.y + ')' ); 
        },
        translate: function(translate) {
            for (dim in translate) {
                transformStrings.push( 'translate' + dim + '(' + translate[dim] + 'px)' );
            }
        },
        skew: function(skew) {
            skew.x = skew.x || 0;
            skew.y = skew.y || 0;
            transformStrings.push( 'skew(' + skew.x + 'deg,' + skew.y + 'deg)' );
        }
    };   

    // iterate the config object to generate strings
    for (transformAction in transformConfig) {
        let transform = transformConfig[transformAction];
        buildString[transformAction](transform);
    }
    return transformStrings.join(' ');
};

Usage:

let transformConfig = { ... };
let transformString = createTransformString(transformConfig);

This approach would limit the repeat code you have for each transformation string and would also take care of your if concern in that only those transformation types specified in the config would be executed.

This also more directly injects the transformation configuration information into the function that generates the strings, meaning you no longer have to rely on variables that may or may not be present in inherited scope. This means you could now have your createTransformString() function declared in a wholly separate include file from the code where the actual transformation(s) values are generated, promoting code re-use.

Since you are using ECMA6, I would also consider building this as a class (or classes).

Finally, from a stylistic standpoint, I would recommend sticking with camelCase instead of snake_case, as this is pretty much the de facto standard for javascript. At a minimum, don't intermingle the two. Additionally, I would suggest watching your line length. You should strive to keep lines of code under ~80 characters per line to improve code readability.

\$\endgroup\$
3
\$\begingroup\$

First I would suggest using template literals. This avoids you from splitting the string, and potentially breaking it.

final_transformation += `rotateX(${x}deg)`;

Next, instead of concatenating final_transformation manually, consider storing those values in an array first, then collapsing them to a string with array.join at the end.

const transforms = [];

if(...) transforms.push(`rotateX(${x}deg)`);
...

const finalTransformations = transforms.join(' ');

It appears that your if statements are simply checking for value presence. A way to avoid this is to simply assign them values. That way, they always have values. It's the same concept as data types. If a certain variable is of type Number, it should exist and it should be a Number.

Also, if x always had a number, you can always add rotateX. It doesn't matter if the value is 0, since rotateX(0deg) will do nothing anyways.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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