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 1 down vote favorite

I am new to jquery, and after spending weeks to convert my pages using jquery, now I am reading about jquery optimization. In that effort, I rewrite one of my functions like below. Appreciate if you comment whether it is write (both are working btw). Also any further improvement tips would be helpful.

PREVIOUS CODE

$("#mydiv").empty();
var i = 2;
$("#axisa option").each(function() {
    $("#mydiv").append("<div id='tempdiv"+i+"'></div>");
    updateCharts($("#wd").slider("value"), $("#ht").slider("value"),$(this).val());
    i++
});

$("#axisb option").each(function() {
    $("#mydiv").append("<div id='tempdiv"+i+"'></div>");
    updateCharts($("#wd").slider("value"), $("#ht").slider("value"),$(this).val());
    i++
});
$("#hiddendiv").show("slow");

OPTIMIZED CODE

var mydiv = $("#mydiv");
var mywid = $("#wd").slider("value");
var myhet = $("#ht").slider("value");
var total = $("#axisa option").length + $("#axisb option").length+2;

mydiv.empty();
var design = "";

for(var i=2;i<total;i++){
    design += "<div id='tempdiv"+i+"'></div>";
}
mydiv.append(design);       
i = 2;

$("#axisa option").each(function() {
    updateCharts(mywid, myhet, $(this).val());
    i++
});

$("#axisb option").each(function() {
    updateCharts(mywid, myhet, $(this).val());
    i++
});
$("#hiddenDiv").show("slow");
share|improve this question
    
you define mydiv as a variable, but then you still use $('#mydiv'). Could you throw this up into a jsFiddle with some target HTML? I'm not quite clear on what you are trying to do. –  Michael Paulukonis Jan 28 '13 at 19:36
    
frankly, I still am not totally clear how to optimize jquery ... I read somewhere that I should not call same selector again and again to avoid creating too many jquery objects... but not sure how exactly to do that. stackoverflow.com/questions/3230727/… –  Abdullah Fahim Jan 28 '13 at 19:51
2  
that makes sense, and I see you doing it, but then you use $("#mydiv").append(design) when you could have used mydiv.append(design). Larger picture though -- I don't know what your code is trying to accomplish. –  Michael Paulukonis Jan 28 '13 at 20:30
    
o yaa, that was a mistake..i missed that ... corrected ... –  Abdullah Fahim Jan 29 '13 at 4:37
    
Caching selectors with a variable is a good optimisation. A handy naming convention that a lot of people use is to prefix $ on your variable name. var $mydiv = $('#mydiv'). That way you will remember that what you have is a jQuery selector (JavaScript doesn't treat the $ as anything special, its just a visual reminder) –  Sean Feb 5 '13 at 19:37

2 Answers 2

up vote 3 down vote

How about this? I haven't tested it, but try it:

var mydiv = $("#mydiv").empty();
var mywid = $("#wd").slider("value");
var myhet = $("#ht").slider("value");
var total = $("#axisa option").length + $("#axisb option").length + 2;
var design = "";

for(var i = 2; i < total; i++){
    $("#mydiv").append("<div id='tempdiv"+i+"'></div>");
}

i = 2;

$("#axisa option, #axisb option").each(function() {
    updateCharts(mywid, myhet, $(this).val());
    i++ // is not used anywhere else? can remove.
});

$("#hiddenDiv").show("slow");

EDIT

var mydiv = $("#mydiv").empty();
var mywid = $("#wd").slider("value");
var myhet = $("#ht").slider("value");
var total = $("#axisa option").length + $("#axisb option").length + 2;
var design = "";
var i;
var concat;

for(i = 2; i < total; i++){
    concat += "<div id='tempdiv"+i+"'></div>";
}

$("#mydiv").append(concat);

i = 2;

$("#axisa option, #axisb option").each(function() {
    updateCharts(mywid, myhet, $(this).val());
    i++;
});

$("#hiddenDiv").show("slow");
share|improve this answer
1  
thanks ... but regarding 1st for loop, somebody wrote somewhere that we should try to avoid repeated DOM manipulation .. so, I used .append only once, not inside for loop ... which one is better? –  Abdullah Fahim Jan 28 '13 at 19:53
    
I think you should concatenate the string inside the loop, then append to the variable mydiv once the loop is complete (as you had in your optimized version). @jsanc623 nicely smushed the other functions together. –  Michael Paulukonis Jan 29 '13 at 15:11
    
You should do what @MichaelPaulukonis Paulukonis said and concatenate the string inside the loop, THEN append. The reason for this is that every time you append, you're crawling the DOM to find that element to append to. I'll edit the code above. –  jsanc623 Jan 29 '13 at 18:35
up vote 2 down vote

Here are some general optimizations that I usually do.

When you have multiple var declarations you can separate with "," making it a bit easier to read and less of a hassle to write:

var mydiv = $("#mydiv").empty(),
    mywid = $("#wd").slider("value"),
    myhet = $("#ht").slider("value"),
    etc = etc.....

Also when I name my variables, ids, classes, etc. it's good to put names that reference what's inside the element, or the role it plays. It will help you remeber what does what when you get into big projects with lots of these declarations.

As a rule of thumb, if you have to call on an element more than once, you should cache its value. This way jQuery doesn't have to jump into the DOM and look for that element each time you call it. You should also try to limit the scope of elements jQuery has to look through to find the one you want. You can use the type of element previous to the id/class. For example:

$("#mydiv").doSomething();
$("#mydiv").doSomethingElse();

Instead do:

var mydiv = $("div #mydiv");

$(mydiv).doSomething();
$(mydiv).doSomethingElse();

Like you said, you are just starting out with jQuery, and with that I highly recommend this screencast by Jeffrey Way called 30 Days to Learn jQuery. He does a really good job of explain some basic principals as well as some more complex concepts.

share|improve this answer

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.