patternjavascriptMinor
Drawing a picture of some trees using JavaScript
Viewed 0 times
picturejavascripttreesdrawingusingsome
Problem
I want to know how I can improve this piece of JS with respect to best practices/performance.
JS
```
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)
}
}
JS
```
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)
}
}
Solution
I am using color codes for clarity of my explanation, not suggesting you use them in the code.
As the image is static. Just use a set code.
You have 5 tree heights:
red, green, yellow, blue (=orange), purple
You have 5 left leaf, 4 right leaf
Using the classes in css.
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.
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 5You 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 //..etcUsing the classes in css.
red tree
green tree
yellow tree
space
green tree
yellow tree
blue tree
space //..etcYou 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.
Code Snippets
Tree.size 1 2 3 4 5leaf.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 //..etcred tree
green tree
yellow tree
space
green tree
yellow tree
blue tree
space //..etcContext
StackExchange Code Review Q#31892, answer score: 2
Revisions (0)
No revisions yet.