JavaScript/jQuery - How do I improve this js code/ refactor

Question

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/

This question appears to be off-topic because it is about code review: http://codereview.stackexchange.com — Toto, Sep 27 '13 at 07:57

score 1 · Answer 1 · edited May 23 '17 at 10:25

1

You might stick to images instead of writing such complex HTML and CSS. See this question to decide if you need it.

If raster is fine, you could have 3 PNG's (since simple PNGs can be compressed really well): one for left leaf hanging on a trunk, one for right leaf (with trunk as well) and one for tree trunk without leaves. All of equal widths with transparent placeholders or styled with appropriate margins. Or you may use negative scaling instead of a second leaf. Probably won't impact performance too much.

So, to compose a tree, you could place several blocks backgounded with tree parts to form a column. Having a one-pixel high trunk image allows for easy vertical scaling. You could even base64-encode pics into CSS to avoid extra file requests.

While this requires a bit of preparation, this might be faster on older browsers, that can't handle complex CSS at nice speeds.

edited May 23 '17 at 10:25

Community

1
1

answered Sep 27 '13 at 09:26

D-side

9,150
3
28
44

I think in that case, instead of using images itself, this can be done only through html/css only with no js, with the only downside that there would be quite a lot of DOM elements for this. – whyAto8 Sep 27 '13 at 09:31
Both methods can work without JS. Your code just creates new DOM elements, so practically it's the same. And it can be done the same way, just with a bit different HTML structure. What I suggest is replace CSS border and round tricks with predrawn PNGs. – D-side Sep 27 '13 at 10:11
Hmm..that's right. And isn't it good to have border radius instead of using images of that, which will create extra http, just curious? – whyAto8 Sep 27 '13 at 10:23
@whyAto8 base64-encode them into CSS and no extra HTTPs anymore. While this will make them bigger in size, they're incredibly small with adaptive palette anyway. – D-side Sep 27 '13 at 10:28
That's kind of new to me, but will try this anyway. Thanks – whyAto8 Sep 27 '13 at 16:14

score 0 · Answer 2 · answered Sep 27 '13 at 08:43

0

Update Code:

Line No              Code
 1                    $(document).ready(function(){ });
 11                   for (j = 0;l = treeGroupTypes.length ,j < l; j++)
 13                   treeGroup = $("<div />");
 16                   for (i = 0; l = treeType.length, i < l; i++)
 18                   leftLeaf = $("<span />");
 20                   leftIcon = $("<i />");
 25                   var tree = $("<div />");

answered Sep 27 '13 at 08:43

Jyoti Prakash

1,180
8
9

Well, regarding the creation of elements using $("
") , I think $(document.createElement("div")) is the fastest. Please check these links: http://stackoverflow.com/questions/268490/jquery-document-createelement-equivalent/268520#268520 , http://stackoverflow.com/questions/327047/what-is-the-most-efficient-way-to-create-html-elements-using-jquery – whyAto8 Sep 27 '13 at 08:57

score 0 · Answer 3 · answered Sep 27 '13 at 08:58

0

Have shorten a few lines of code

http://jsfiddle.net/5NrfQ/

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

answered Sep 27 '13 at 08:58

Abhidev

7,063
6
21
26

Well, not sure, but dont we need to declare other variables such as rightLeaf etc. The code might work though, but shouldnt we be declaring variables. – whyAto8 Sep 27 '13 at 09:00
the variables have been declared...if you look closely its ',' and not ';' after each variable declaration. – Abhidev Oct 04 '13 at 07:27
Oh, I am sorry, I missed that. Just curious, is it kind of some best practice to do like this, or it gives some performance gain. – whyAto8 Oct 04 '13 at 08:27
not a performance gain but you don't have too many var statements...just one var with all your variable declarations in one place. – Abhidev Oct 04 '13 at 11:25

JavaScript/jQuery - How do I improve this js code/ refactor

3 Answers3