Welcome to the Onshape forum! Ask questions and join in the discussions about everything Onshape.

First time visiting? Here are some places to start:
  1. Looking for a certain topic? Check out the categories filter or use Search (upper right).
  2. Need support? Ask a question to our Community Support category.
  3. Please submit support tickets for bugs but you can request improvements in the Product Feedback category.
  4. Be respectful, on topic and if you see a problem, Flag it.

If you would like to contact our Community Manager personally, feel free to send a private message or an email.

Help with redundant code?

MichaelPascoeMichaelPascoe Member Posts: 929 PRO
edited May 2021 in FeatureScript
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;
                        }
                    }
                }

Best Answers

  • chadstoltzfuschadstoltzfus Member, Developers Posts: 84 PRO
    Answer ✓
    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_KempenAlex_Kempen Member Posts: 221 EDU
    Answer ✓
    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.

Answers

  • caden_armstrongcaden_armstrong Member, User Group Leader Posts: 52 PRO
    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.

    Software Development Lead - CADSharp.com - Email
    We specialize in custom FeatureScripts, and Onshape integrated applications
  • MichaelPascoeMichaelPascoe Member Posts: 929 PRO
    edited May 2021
    @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;
                            }
                        }
                    }
                    // }
                }




  • chadstoltzfuschadstoltzfus Member, Developers Posts: 84 PRO
    Answer ✓
    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. 
  • MichaelPascoeMichaelPascoe Member Posts: 929 PRO
    Thank you @chadstoltzfus & @caden_armstrong, This is perfect! 


  • Alex_KempenAlex_Kempen Member Posts: 221 EDU
    Answer ✓
    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.
  • MichaelPascoeMichaelPascoe Member Posts: 929 PRO
    edited May 2021
    That is extremely helpful. Thanks @Alex_Kempen!
Sign In or Register to comment.