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.

I have a nested loop which creates a drop down with 3 levels. Works fine on modern browsers but on IE8 it hangs and takes several seconds when there's even a moderate volume of data.

It creates a list of publishers, when I click a publisher I get that publishers authors, click an author I get the years they're published.

I'm looking for a way of optimizing this to reduce the time it takes to run in IE.

Any ideas?

function createDropDown() {        
        var target = $('#dropdownMenu');
        for (var i = 0; i < data.genres.length; i++) {
                var genreval = data.genres[i].genre;
                for (var j = 0; j < data.genres[i].publishers.length; j++) {
                        var publisherval = data.genres[i].publishers[j].publisher;
                        if (typeof app.cache.publisher[publisherval] == 'undefined') {
                                target.append('<li class="publisher" data-value="' + publisherval + '"><a href="#" class="nextmenu">' + publisherval + '</a><ul class="sub-menu" data-title="publishers"></ul></li>');
                                app.cache.publisher[publisherval] = {
                                        'ul': target.find('>.publisher[data-value="' + publisherval + '"]>.sub-menu'),
                                        'author': {}
                                }; 
                        }
                                var ulauthors = app.cache.publisher[publisherval].ul;
                                for (var k = 0; k < data.genres[i].publishers[j].authors.length; k++) {
                                        var authorval = data.genres[i].publishers[j].authors[k].author + ' (' + genreval + ')';
                                        var author_val = data.genres[i].publishers[j].authors[k].author;
                                        var originalgenre = data.genres[i].publishers[j].authors[k].originalgenre;
                                        if (typeof app.cache.publisher[publisherval].author[authorval] == 'undefined') {
                                                ulauthors.append('<li class="author" data-value="' + authorval + '"><a href="#" class="nextmenu">' + authorval + '</a><ul class="sub-menu" data-title="authors"></ul></li>');
                                                app.cache.publisher[publisherval].author[authorval] = ulauthors.find('>.author[data-value="' + authorval + '"]>.sub-menu');                             
                                        }
                                                                    var ulyears = app.cache.publisher[publisherval].author[authorval];
                                                                    var gItems = "";
                                                                    for (var m = 0; m < data.genres[i].publishers[j].authors[k].authorYears.length; m++) {
                                                                            var yearval = data.genres[i].publishers[j].authors[k].authorYears[m];
                                                                            var year = ulyears.find('.year[data-value="' + yearval + '"]');
                                                                            if (year.size() == 0) {
                                                                                    var id = ++count;
                                                                                    gItems+='<li class="year" data-value="' + yearval + '"><a id="selyear' + id + '" class="addItem" data-id="' + id + '" data-originalgenre ="' + originalgenre + '"data-year="' + yearval + '" data-publisher="' + publisherval + '" data-author="' + author_val '">' + yearval + '</a></li>';
                                                                            }
                                                                    }
                                        ulyears.append(gItems);
                                };
                };
        };
    }

Edited Code which doesnt append the authors:

if (typeof app.cache.publisher[publisherval].author[authorval] == 'undefined') {
app.cache.publisher[publisherval].author[authorval] = $('<li class="author" data-value="' + authorval + '"><a href="#" class="nextmenu">' + authorval + '</a><ul class="sub-menu" data-title="authors"></ul></li>').children('ul').appendTo(ulauthors);
}
share|improve this question
    
Welcome to Code Review! This is a good question! Do know that reviewer can still review other aspect of your code (style, formatting, algorithm and etc)! –  Marc-Andre Jul 22 at 13:52
    
Thanks, i didn't know that but i'll keep it in mind! –  Hulaz Jul 22 at 14:05

2 Answers 2

up vote 8 down vote accepted

Formatting

I found this exceedingly difficult to read. The indentation in particular is inconsistent. I highly suspect that this is because your code contains mixed tabs and spaces. (StackExchange silently converts tabs into four spaces. If this is not the case, please ignore this section.)

I don't want to start a holy war or anything, but there are three main camps here.

  1. Indent exclusively with tabs. This is nice when using text editors with a configurable width for tabs. It also can marginally speed up a site, but that's only if you aren't already stripping out whitespace before sending to client. It's also negligible in the era of gzip.

  2. Indent exclusively with spaces. This guarantees that the code will look the same for everyone. Since it isn't configurable, though, if you like eight spaces and someone else likes two spaces, they can't just make it happen with a setting; they'll need to do a replace on the file.

  3. Mixed: indentation levels use tabs, but alignment is done with spaces. This is the style I personally use.

Aside: Semicolons are typically not necessary after braces. You're using a code block for a for loop, not a function expression.

Naming

  • I don't see any point in naming a variable genreval; just genre is enough.
  • Convention dictates that jQuery object variables are prefixed with a $.
  • I think that it's particularly confusing to have authorval and author_val. If anything, I'd have author_val → author and authorval → authorWithGenre or something... I really don't know how I'd name this. Is it critical that an author's genres are distinguished?
  • You have app.cache.publisher[].ul and app.cache.publisher[].author[] both being jQuery objects for a ul. This is inconsistent. I might prefer each element of author[] to be an object with just the key ul, i.e. author[] = { 'ul': ... };.

Performance

It looks like you're appending an element and immediately performing a find for it. This is incredibly wasteful. You can just use appendTo instead.

I can think of two ways; one creates two objects and uses two appendTos, while one performs a children query. I wrote a quick-and-dirty jsPerf. The children query runs the fastest; your original is 91% slower.

// children
app.cache.publisher[publisherval].author[authorval] = {
    'ul': $('<li class="author" data-value="' + authorval + '"><a href="#" class="nextmenu">' + authorval + '</a><ul class="sub-menu" data-title="authors"></ul></li>').children('ul').appendTo(ulauthors)
}
// double append
app.cache.publisher[publisherval].author[authorval] = {
    'ul': $('<ul class="sub-menu" data-title="authors"></ul>').appendTo($('<li class="author" data-value="' + authorval + '"><a href="#" class="nextmenu">' + authorval + '</a></li>').appendTo(ulauthors))
}
share|improve this answer
    
Hi, i've tried implementing the above but get an error when i then come to do the years. Provided the new code snippet. –  Hulaz Jul 22 at 15:30
    
@Hulaz You'll note that I changed it to be an object. If you want to just plug it in and ignore the rest of my recommendations, just copy and paste: app.cache.publisher[publisherval].author[authorval] = $('<li class="author" data-value="' + authorval + '"><a href="#" class="nextmenu">' + authorval + '</a><ul class="sub-menu" data-title="authors"></ul></li>').children('ul').appendTo(ulauthors);. –  Schism Jul 22 at 15:31
2  
Maybe you did not want to start a holy war ..... but, on SE, the only indent style that renders consistently and plays nicely with the in-the-ask-a-question-text-area is spaces-only. There are some tools out there to help you if you currently have tab-indents and want to switch to spaces for Code Review –  rolfl Jul 22 at 16:52
2  
There is a fourth main camp actually : I don't how to indent so I'm doing what I want, how I want and with no consistency. This group is the darker one! –  Marc-Andre Jul 22 at 16:56
1  
It seems like the issue with the indentation is because for the OP, one tab was not equal to four spaces. While on StackExchange, one tab is always equal to four spaces. I post most of my code here using tabs and I have had no issue with this, thanks to the fact that I'm using the rule 1 tab = 4 spaces already. –  Simon André Forsberg Jul 22 at 16:56

There are a few things you can do to improve your JavaScript code.

For example, you can save the element you are using in each loop:

var currentPuiblisher = data.genres[i].publishers[j]

Every time you use a object like data.genres[i].publishers[j], the browser has to walk through every property, slowing down the process.

Your loops are checking the length of the array every iteration. This will be faster:

for (var k = 0, authLength = data.genres[i].publishers[j].authors.length; k < authLength; k++)

A microtemplate library would help you a lot generating the HTML, but anyway it would be better if you generate the full string and append once to the domDOM, there are a lot of append operations that require some time.

In general, I would try to use a dynamic solution: just generating the first level of the menu, and then dynamically add the submenus on hover.

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.