Help with redundant code?

MichaelPascoe

My code is very redundant, what would be a more efficient way to write this? I thought it may be a good place to use a function, but I'm not sure how to do it.

                if (definition.groups[g].configScope == ConfigScope.EVEN_COLUMNS || definition.groups[g].configScope == ConfigScope.ALL || evenColumnPreset)
                {
                    if (evenColumn)
                    {
                        if (definition.groups[g].transform || evenColumnMovePreset)
                        {
                            coordMatrix[u][v].origin = planeToWorld3D(tPlane) * (vector(definition.groups[g].dx, definition.groups[g].dy, definition.groups[g].dz));
                            rotTransform = rotationAround(line(c1.origin, c1.zAxis), definition.groups[g].obAngle);
                        }

                        if (definition.groups[g].scale || evenColumnSizePreset)
                        {
                            sizeX = definition.groups[g].sizeX;
                            sizeY = definition.groups[g].sizeY;
                            sizeZ = definition.groups[g].sizeZ;
                        }
                    }
                }





                if (definition.groups[g].configScope == ConfigScope.ODD_COLUMNS || definition.groups[g].configScope == ConfigScope.ALL || oddColumnPreset)
                {
                    if (oddColumn)
                    {
                        if (definition.groups[g].transform || oddColumnMovePreset)
                        {
                            coordMatrix[u][v].origin = planeToWorld3D(tPlane) * (vector(definition.groups[g].dx, definition.groups[g].dy, definition.groups[g].dz));
                            rotTransform = rotationAround(line(c1.origin, c1.zAxis), definition.groups[g].obAngle);
                        }

                        if (definition.groups[g].scale || oddColumnSizePreset)
                        {
                            sizeX = definition.groups[g].sizeX;
                            sizeY = definition.groups[g].sizeY;
                            sizeZ = definition.groups[g].sizeZ;
                        }
                    }
                }

chadstoltzfus

I agree with Caden above, we definitely want to keep this readable and also scalable. Let's say you wanted to perform operations on even columns specifically, I would keep those things outside of the function to allow you to do more in the main code body. I whipped up this function based on the code I saw above.  

/**
 *  Returns a map containing variables to be used for setting matrix options.
 * 
 * @param group
 * @param coordMatrix
 * @param tPlane
 * @param c1
 * @param movePreset
 * @param sizePreset
 */ 
function getMatrixOpts(group, coordMatrix, tPlane, c1, movePreset, sizePreset) returns map
{
    var rotTransform;
    var sizeX;
    var sizeY;
    var sizeZ; 
    var origin;
    
    if (group.transform || movePreset)
    {
        origin = planeToWorld3D(tPlane) * (vector(group.dx, group.dy, group.dz));
        rotTransform = rotationAround(line(c1.origin, c1.zAxis), group.obAngle);
    }

    if (group.scale || sizePreset)
    {
        sizeX = group.sizeX;
        sizeY = group.sizeY;
        sizeZ = group.sizeZ;
    }

    return {
            "origin" : origin,
            "rotTransform" : rotTransform,
            "sizeX" : sizeX,
            "sizeY" : sizeY,
            "sizeZ" : sizeZ
        };

}

Then you can use it in those conditional statements (which are not redundant so I kept those out of the function.

 var opts; 
            
            if (definition.groups[g].configScope == ConfigScope.EVEN_COLUMNS || definition.groups[g].configScope == ConfigScope.ALL || evenColumnPreset)
            {
                if (evenColumn)
                {
                    opts = getMatrixOpts(definition.groups[g], coordMatrix[u][v], tPlane, c1, evenColumnMovePreset, evenColumnSizePreset);
                }
            }
            if (definition.groups[g].configScope == ConfigScope.ODD_COLUMNS || definition.groups[g].configScope == ConfigScope.ALL || oddColumnPreset)
            {
                if (oddColumn)
                {
                    opts = getMatrixOpts(definition.groups[g], coordMatrix[u][v], tPlane, c1, oddColumnMovePreset, oddColumnSizePreset);
                }
            }
            if (definition.groups[g].configScope == ConfigScope.EVEN_ROWS || evenRowPreset)
            {
                if (evenRows)
                {
                    opts = getMatrixOpts(definition.groups[g], coordMatrix[u][v], tPlane, c1, evenRowMovePreset, evenRowSizePreset);
                }
            }
            if (definition.groups[g].configScope == ConfigScope.ODD_ROWS || oddRowPreset)
            {
                if (oddRows)
                {
                    opts = getMatrixOpts(definition.groups[g], coordMatrix[u][v], tPlane, c1, oddRowMovePreset, oddRowSizePreset);
                }
            }
        }
        
        cordMatrix[u][v].origin = opts.origin;
        rotTransform = opts.rotTransform;
        sizeX = opts.sizeX;
        sizeY = opts.sizeY;
        sizeZ = opts.sizeZ;

That being said I can't really test this to see if it works with the rest of your code and I'm only human so let me know how this works for you. 

Alex_Kempen

If you wanted, you can do even better by combining if statement conditions together and setting a variable first to avoid redundant characters. Setting a variable is especially useful when nesting maps and arrays (like when using array parameters):

const group = definition.groups[g];
const configScope = group.configScope;
var opts;

You can also write predicates for common conditions:

// can use group or configScope, whatever is easier/more convenient
predicate evenColumn(group is map, evenColumnPreset is boolean, evenColumn is boolean)
{
    evenColumn;
    group.configScope == ConfigScope.EVEN_COLUMNS || 
    group.configScope == ConfigScope.ALL || 
    evenColumnPreset;
}<br>

And lastly, you can set variables in your if statements to avoid copying and pasting the opts call multiple times.

const group = definition.groups[g];
var runOpts = false; // if opts always runs, skip this
var movePreset;
var sizePreset;

if (evenColumn(group, evenColumnPreset, evenColumn))
{
    runOpts = true;
    movePreset = evenColumnMovePreset;
    sizePreset = evenColumnSizePreset;
}
// use "else if" if only one statement needs to run per execution
// i.e. if it's an even column, we don't need to check if it's also an odd column
else if (oddColumn(group, oddColumnPreset, oddColumn))
{
    
}
// rest of else if conditions

if (runOpts)
{
    opts = getMatrixOpts(definition.groups[g], coordMatrix[u][v], tPlane, c1, movePreset, sizePreset<span>);</span>
}

As a final note, you could save even more lines by grouping presets into a data type which stores multiple values, like a map. You can also write tiny sub predicates to check individual conditions faster. For some examples of this, see line 301 - 335 of the extrude feature. 
I hope some of this helps, or is at least useful stuff to know in the future.

caden_armstrong2

Firstly, efficient isn't always readable. Making your code more efficiently can also make it a pain to maintain.
When you say "efficient", are you looking for less lines of code or for it to have less regen time? 

It looks like the two if statesments for checking odd/even columns are the same... so those could be combined cutting out half of your code.
...Unless thats a mistake and they are different.

Is there a reason sizeX/Y/Z are not a vector?
That code could be changed to 

sizeVector = definition.groups[g].sizeVector;

if the tPlane is always the same for the iteration over g you could calculate that transform before the loop and use it as a variable -> tPlaneTransform = ...
and then you aren't redundantly recalculating it during each loop.
Same goes for the line for c1.

MichaelPascoe

@caden_armstrong
By efficient, I mean not having the same code copy and pasted 4 times. Here is the whole loop, you can ignore the "presets" section:

As you can see, It is the same format each statement with slight variations.

for (var g = 0; g < size(definition.groups); g += 1)
            {
                //debug(context, definition.groups[g].dx == 0 * meter, DebugColor.RED);
                //debug(context, definition.patternTyp, DebugColor.RED);

                //___________________________________________________________
                //                          PRESETS
                //___________________________________________________________

                // var sizeX = 1 * inch;
                // var sizeY = 1 * inch;
                // var sizeZ = 1 * inch;

                var moveX = 0 * inch;
                var moveY = 0 * inch;
                var moveZ = 0 * inch;
                var moveA = 0 * degree;
                
                var evenColumnPreset = false;
                var oddColumnPreset = false;
                var evenRowPreset = false;
                var oddRowPreset = false;

                var evenColumnMovePreset = false;
                var oddColumnMovePreset = false;
                var evenRowMovePreset = false;
                var oddRowMovePreset = false;
                
                var evenColumnSizePreset = false;
                var oddColumnSizePreset = false;
                var evenRowSizePreset = false;
                var oddRowSizePreset = false;

                if (definition.texture == Texture.BRICK)
                {
                    sizeX = 8 * inch;
                    sizeY = 3.625 * inch;
                    sizeZ = 2.25 * inch;
                    
                    evenColumnSizePreset = true;
                    oddColumnSizePreset = true;
                }
                if (definition.patternTyp == PatternTyp.STAGGERED)
                {                    
                    if (definition.groups[g].dx == 0 * meter)
                    {
                        if (evenRows)
                        {
                            moveX = sizeX / 2;
                            println("no x input");
                        }
                    }
                    println("x input");
                    oddRowPreset = true;
                    oddRowMovePreset = true;
                    
                }

                //___________________________________________________________
                //                          -------
                //___________________________________________________________



                if (definition.groups[g].configScope == ConfigScope.EVEN_COLUMNS || definition.groups[g].configScope == ConfigScope.ALL || evenColumnPreset)
                {
                    if (evenColumn)
                    {
                        if (definition.groups[g].transform || evenColumnMovePreset)
                        {
                            coordMatrix[u][v].origin = planeToWorld3D(tPlane) * (vector(definition.groups[g].dx, definition.groups[g].dy, definition.groups[g].dz));
                            rotTransform = rotationAround(line(c1.origin, c1.zAxis), definition.groups[g].obAngle);
                        }

                        if (definition.groups[g].scale || evenColumnSizePreset)
                        {
                            sizeX = definition.groups[g].sizeX;
                            sizeY = definition.groups[g].sizeY;
                            sizeZ = definition.groups[g].sizeZ;
                        }
                    }
                }
                if (definition.groups[g].configScope == ConfigScope.ODD_COLUMNS || definition.groups[g].configScope == ConfigScope.ALL || oddColumnPreset)
                {
                    if (oddColumn)
                    {
                        if (definition.groups[g].transform || oddColumnMovePreset)
                        {
                            coordMatrix[u][v].origin = planeToWorld3D(tPlane) * (vector(definition.groups[g].dx, definition.groups[g].dy, definition.groups[g].dz));
                            rotTransform = rotationAround(line(c1.origin, c1.zAxis), definition.groups[g].obAngle);
                        }

                        if (definition.groups[g].scale || oddColumnSizePreset)
                        {
                            sizeX = definition.groups[g].sizeX;
                            sizeY = definition.groups[g].sizeY;
                            sizeZ = definition.groups[g].sizeZ;
                        }
                    }
                }
                if (definition.groups[g].configScope == ConfigScope.EVEN_ROWS || evenRowPreset)
                {
                    if (evenRows)
                    {
                        if (definition.groups[g].transform || evenRowMovePreset)
                        {
                            coordMatrix[u][v].origin = planeToWorld3D(tPlane) * (vector(definition.groups[g].dx, definition.groups[g].dy, definition.groups[g].dz));
                            rotTransform = rotationAround(line(c1.origin, c1.zAxis), definition.groups[g].obAngle);
                        }

                        if (definition.groups[g].scale || evenRowSizePreset)
                        {
                            sizeX = definition.groups[g].sizeX;
                            sizeY = definition.groups[g].sizeY;
                            sizeZ = definition.groups[g].sizeZ;
                        }
                    }
                }
                if (definition.groups[g].configScope == ConfigScope.ODD_ROWS || oddRowPreset)
                {
                    if (oddRows)
                    {
                        if (definition.groups[g].transform || oddRowMovePreset)
                        {
                            coordMatrix[u][v].origin = planeToWorld3D(tPlane) * (vector(definition.groups[g].dx, definition.groups[g].dy, definition.groups[g].dz));
                            rotTransform = rotationAround(line(c1.origin, c1.zAxis), definition.groups[g].obAngle);
                        }

                        if (definition.groups[g].scale || oddRowSizePreset)
                        {
                            sizeX = definition.groups[g].sizeX;
                            sizeY = definition.groups[g].sizeY;
                            sizeZ = definition.groups[g].sizeZ;
                        }
                    }
                }
                // }
            }

chadstoltzfus

I agree with Caden above, we definitely want to keep this readable and also scalable. Let's say you wanted to perform operations on even columns specifically, I would keep those things outside of the function to allow you to do more in the main code body. I whipped up this function based on the code I saw above.  

/**
 *  Returns a map containing variables to be used for setting matrix options.
 * 
 * @param group
 * @param coordMatrix
 * @param tPlane
 * @param c1
 * @param movePreset
 * @param sizePreset
 */ 
function getMatrixOpts(group, coordMatrix, tPlane, c1, movePreset, sizePreset) returns map
{
    var rotTransform;
    var sizeX;
    var sizeY;
    var sizeZ; 
    var origin;
    
    if (group.transform || movePreset)
    {
        origin = planeToWorld3D(tPlane) * (vector(group.dx, group.dy, group.dz));
        rotTransform = rotationAround(line(c1.origin, c1.zAxis), group.obAngle);
    }

    if (group.scale || sizePreset)
    {
        sizeX = group.sizeX;
        sizeY = group.sizeY;
        sizeZ = group.sizeZ;
    }

    return {
            "origin" : origin,
            "rotTransform" : rotTransform,
            "sizeX" : sizeX,
            "sizeY" : sizeY,
            "sizeZ" : sizeZ
        };

}

Then you can use it in those conditional statements (which are not redundant so I kept those out of the function.

 var opts; 
            
            if (definition.groups[g].configScope == ConfigScope.EVEN_COLUMNS || definition.groups[g].configScope == ConfigScope.ALL || evenColumnPreset)
            {
                if (evenColumn)
                {
                    opts = getMatrixOpts(definition.groups[g], coordMatrix[u][v], tPlane, c1, evenColumnMovePreset, evenColumnSizePreset);
                }
            }
            if (definition.groups[g].configScope == ConfigScope.ODD_COLUMNS || definition.groups[g].configScope == ConfigScope.ALL || oddColumnPreset)
            {
                if (oddColumn)
                {
                    opts = getMatrixOpts(definition.groups[g], coordMatrix[u][v], tPlane, c1, oddColumnMovePreset, oddColumnSizePreset);
                }
            }
            if (definition.groups[g].configScope == ConfigScope.EVEN_ROWS || evenRowPreset)
            {
                if (evenRows)
                {
                    opts = getMatrixOpts(definition.groups[g], coordMatrix[u][v], tPlane, c1, evenRowMovePreset, evenRowSizePreset);
                }
            }
            if (definition.groups[g].configScope == ConfigScope.ODD_ROWS || oddRowPreset)
            {
                if (oddRows)
                {
                    opts = getMatrixOpts(definition.groups[g], coordMatrix[u][v], tPlane, c1, oddRowMovePreset, oddRowSizePreset);
                }
            }
        }
        
        cordMatrix[u][v].origin = opts.origin;
        rotTransform = opts.rotTransform;
        sizeX = opts.sizeX;
        sizeY = opts.sizeY;
        sizeZ = opts.sizeZ;

That being said I can't really test this to see if it works with the rest of your code and I'm only human so let me know how this works for you. 

MichaelPascoe

Thank you @chadstoltzfus & @caden_armstrong, This is perfect! 

Alex_Kempen

If you wanted, you can do even better by combining if statement conditions together and setting a variable first to avoid redundant characters. Setting a variable is especially useful when nesting maps and arrays (like when using array parameters):

const group = definition.groups[g];
const configScope = group.configScope;
var opts;

You can also write predicates for common conditions:

// can use group or configScope, whatever is easier/more convenient
predicate evenColumn(group is map, evenColumnPreset is boolean, evenColumn is boolean)
{
    evenColumn;
    group.configScope == ConfigScope.EVEN_COLUMNS || 
    group.configScope == ConfigScope.ALL || 
    evenColumnPreset;
}<br>

And lastly, you can set variables in your if statements to avoid copying and pasting the opts call multiple times.

const group = definition.groups[g];
var runOpts = false; // if opts always runs, skip this
var movePreset;
var sizePreset;

if (evenColumn(group, evenColumnPreset, evenColumn))
{
    runOpts = true;
    movePreset = evenColumnMovePreset;
    sizePreset = evenColumnSizePreset;
}
// use "else if" if only one statement needs to run per execution
// i.e. if it's an even column, we don't need to check if it's also an odd column
else if (oddColumn(group, oddColumnPreset, oddColumn))
{
    
}
// rest of else if conditions

if (runOpts)
{
    opts = getMatrixOpts(definition.groups[g], coordMatrix[u][v], tPlane, c1, movePreset, sizePreset<span>);</span>
}

As a final note, you could save even more lines by grouping presets into a data type which stores multiple values, like a map. You can also write tiny sub predicates to check individual conditions faster. For some examples of this, see line 301 - 335 of the extrude feature. 
I hope some of this helps, or is at least useful stuff to know in the future.

MichaelPascoe

That is extremely helpful. Thanks @Alex_Kempen!