current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I want to know how can I improve this piece of js with respect to best practices/performance.

JS code:

var treeGroupTypes, treeType, leftLeafClass, rightLeafClass;
treeGroupTypes = ["tree-group-small", "tree-group-avg", "tree-group-large", "tree-group-large", "tree-group-avg", "tree-group-small", "tree-group-small", "tree-group-avg", "tree-group-large", "tree-group-large", "tree-group-avg", "tree-group-small"];
treeType = ["small-tree", "avg-tree", "large-tree"];
leftLeafClass = "left-leaf";
rightLeafClass = "right-leaf";

//Both the above arrays have css classes in them, and then the 2 variables as well. Basically the whole js codes builds some trees and appends leaves to them.

buildTrees(treeGroupTypes, treeType, leftLeafClass, rightLeafClass);

buildTrees: function (treeGroupTypes, treeType, leftLeafClass, rightLeafClass) {
        for (j = 0; j < treeGroupTypes.length; j++) {
            var treeGroup;
            treeGroup = $(document.createElement("div"));
            treeGroup.addClass(treeGroupTypes[j]).appendTo(".trees")

            for (i = 0; i < treeType.length; i++) {
                var leftLeaf, rightLeaf, leftIcon, rightIcon;
                leftLeaf = $(document.createElement("span"));
                rightLeaf = leftLeaf.clone();
                leftIcon = $(document.createElement("i"));
                rightIcon = leftIcon.clone();
                leftLeaf.addClass(leftLeafClass).append(leftIcon);
                rightLeaf.addClass(rightLeafClass).append(rightIcon);

                var tree = $(document.createElement("div"));
                tree.addClass(treeType[i]).appendTo(treeGroup);
                leftLeaf.appendTo(tree);

                if (treeGroupTypes[j] == "tree-group-large" && treeType[i] == "large-tree") {
                    for (l = 1; l < 4; l++) {
                        var more = rightLeaf.clone();
                        more.css("top", (tree.height() / 4) * l).appendTo(tree)
                    }
                }
                else if (treeGroupTypes[j] == "tree-group-avg" && treeType[i] == "large-tree") {
                    for (l = 1; l < 3; l++) {
                        var more = rightLeaf.clone();
                        more.css("top", ((tree.height() / 3) * l) + 10).appendTo(tree)
                    }
                }
                else {
                    rightLeaf.css("top", tree.height() / 3).appendTo(tree)
                }
            }
        }
    }

CSS required:

There are 3 tree groups - avg, large, small , as per height, shown in fiddle. By group, it means it has 3 trees in this together and those 3 trees in each group are further sub divided as large-tree, avg-tree, small-tree

.trees { padding:0 10px;}
.tree-group-avg {margin:0 8px; display:inline-block;}
.tree-group-avg div {position:relative; display:inline-block; margin:0 10px 0 0; background:#83919F; width:2px; vertical-align:bottom;}
.tree-group-avg .large-tree { height:120px; }
.tree-group-avg .avg-tree { height:90px;}
.tree-group-avg .small-tree { height:60px;}

.tree-group-large {margin:0 8px;  display:inline-block;}
.tree-group-large div {position:relative; display:inline-block; margin:0 10px 0 0; background:#83919F; width:2px; vertical-align:bottom;}
.tree-group-large .large-tree { height:150px; }
.tree-group-large .avg-tree { height:120px;}
.tree-group-large .small-tree { height:90px;}

.tree-group-small {margin:0 8px;  display:inline-block;}
.tree-group-small div {position:relative; display:inline-block; margin:0 10px 0 0; background:#83919F; width:2px; vertical-align:bottom;}
.tree-group-small .large-tree { height:90px; }
.tree-group-small .avg-tree { height:60px;}
.tree-group-small .small-tree { height:30px;}

Below are the leaf classes which are attached to tree, left leaf class means it will be on left side of tree and right leaf on right side

.left-leaf i{width:10px; height:10px; border-radius:0 10px 0 10px; display:inline-block; background:#ACCF37; position:relative;behavior: url(css/PIE.htc); }
.left-leaf  {position:absolute; left:-10px;}
.right-leaf i{width:10px; height:10px; border-radius:10px 0 10px 0; display:inline-block; background:#ACCF37; position:relative; behavior: url(css/PIE.htc);}
.right-leaf  {position:absolute;}

HTML:

<section class="trees"></section>

jsfiddle link of what it produces: http://jsfiddle.net/5NrfQ/

share|improve this question
    
You want to create a static image of these trees? –  user29649 Sep 27 '13 at 8:47
    
Yes, its just a static image. –  whyAto8 Sep 27 '13 at 8:48
    
Can I provide an algorithm, rather than refactor the entire code? –  user29649 Sep 27 '13 at 8:48
    
Yeah, sure. I will try that as well. –  whyAto8 Sep 27 '13 at 8:50
    
give me a minute.. writing it up –  user29649 Sep 27 '13 at 8:51

1 Answer 1

up vote 2 down vote

I am using color codes for clarity of my explanation, not suggesting you use them in the code.

trees

As the image is static. Just use a set code.

You have 5 tree heights:
red, green, yellow, blue (=orange), purple 

Tree.size 1 2 3 4 5

You have 5 left leaf, 4 right leaf 

leaf.left 1 2 3 4 5   
leaf.right 1 2 3 4 

red tree = .size1 +  leaf.left1 + leaf.right1.
green tree = .size2 + leaf.left2 + leaf.right2  //..etc

Using the classes in css. 

 red tree
 green tree
 yellow tree
 space
 green tree
 yellow tree
 blue tree
 space //..etc

You know how to code, just use the above algorithm.

To change the width of the trunks, just add change to format accordingly.

Honestly, your ability to loop shows you have a talent, and put a lot of thought into your code. But in this case, where you are producing a static image, it is over complicated and unnecessary. Save your code for situations, where you require user input; I am sure it could be modified to be useful for a more dynamic website.

You can achieve the same result using css and no js. I think the use of js is an overkill. With websites, if you can achieve the same result will good css, I think (my opinion) that it is the better way to go. Back in the day js was treated suspiciously and often disabled on web browser, but this is less so the case now. However, I would recommend, that if you can achieve the same result using css, I wouldn't code such a thing using js.

Personally, I would reserve js for client side validation, or interaction in a dynamic website. Not for producing a static image.

My opinion.

Edit
Also, you have a lot of nested looping going on.. and I haven't addressed that in this answer, I am sure that could be optimised.

share|improve this answer
    
well, just to confirm, that when you say use a set code, would you mean just use html/css with the above mentioned algo to create the image, instead of doing it through js. –  whyAto8 Sep 27 '13 at 9:27
    
@whyAto8 yes, I just edited my answer to address this and why I wouldn't use js.. sorry, I should've been more explicit from the outset.. I think you are very capable, but sometimes restraint and a simpler method is better.. –  user29649 Sep 27 '13 at 9:30
    
As I said, keep your code.. it will be useful for something else, I am sure –  user29649 Sep 27 '13 at 9:31
    
Thanks a lot for that info. My first attempt at doing this was only through html/css and no js, but then I thought of that I would end up writing a lot of html elements for this, so would it make sense to use js and loop it through. I did raise a question on this :stackoverflow.com/questions/18975586/… , hence I went ahead with js solution. I am still kindda confused, what to do. I am ready with both the solutions though. –  whyAto8 Sep 27 '13 at 9:38
    
@whyAto8 this is one answer, one opinion.. why don't you try to write it (or part of it) using css, and see if it seems less complicated.. I am happy to come back to this with you and look at coding it.. I just have to go now –  user29649 Sep 27 '13 at 9:42

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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