up vote 3 down vote favorite

I have two elements on my page:

<ul id="VerticalMenu></ul>
<ul id="AccordionMenu"></ul>

I'm calling some JSON with jQuery and loading elements in the divs. I'm curious if there are things I can be doing more efficiently, better ways to use selectors and JSON. Here's a huge pile of jquery, thanks for any help:

$().ready(function() {
    //Load sections
    GetCarlineSections(_BucketID);
});

function GetCarlineSections(bucketID) {
    //Get section json list
    $.ajax(
    { 
        type: "POST",
        url: _ApplicationRootURL + 'GetChildBucketList', //Call the ActionResult to return the JSON object
        data: 'parentID=' + bucketID,
        success: function (sections) { //'sections' is an array of JSON objects returned by GetChildBucketList
            $(sections).each(function () {
                $('#VerticalMenu') //Append each item to the #VerticalMenu <ul>
                    .append(
                        $('<li/>') //Append a new <li> element to <ul> #VerticalMenu
                        .addClass('Section')
                        .html(
                            $('<h4/>') //Create a new <h4> inside of the <li>
                            .addClass(this.BucketName)
                            .html(this.BucketName)
                            .click({ parentID: this.BucketID }, function (event) { //Attach a click event to the <h4> element
                                $('#AccordionMenu').empty();
                                GetSectionSubSections(event.data.parentID); //Event.data.parentID is the id of the bucket represented by this <h4> element
                            })
                        )
                    );
            });
        }
    });
}

function GetSectionSubSections(bucketID) {
    $.ajax(
    { 
        type: "POST",
        url: _ApplicationRootURL + 'GetChildBucketList',
        data: 'parentID=' + bucketID,
        success: function (SubSections) { //SubSections are the children buckets of Section, local var bucketID
            $(SubSections).each(function () {
                $('#AccordionMenu')
                    .append(
                        $('<li/>')
                        .addClass('SubSection')
                        .html(
                            $('<h4/>')
                            .addClass(this.SEOURLName)
                            .html(this.BucketName)
                            .click({ parentID: this.BucketID }, function (event) { //Eventdata parentID passes the SubSectionID to the event object
                                GetSubSectionHeadlines(this, event.data.parentID)
                            })
                        )
                    );
            });
        }
    });
}

function GetSubSectionHeadlines(parentElement, SubSectionID) {
    //Get the Headlines based on the parent SubSection
    $(parentElement).after($('<ul/>').addClass(parentElement.className));
    $.ajax({
        type: 'POST',
        url: _ApplicationRootURL + 'GetChildBucketList',
        data: 'parentID=' + SubSectionID,
        success: function (Headlines) {
            $(Headlines).each(function () {
                $('ul.' + parentElement.className).prepend(
                    $('<li/>')
                        .html(this.BucketName)
                )
            })
        }
    });
}
share|improve this question

1 Answer

up vote 1 down vote accepted

Two small nickpicks at the start:

  • Empty HTML lists (ul, ol) are strictly spoken invalid. They should contain at least one list item. That said, I believe there is no browser limitation or other technical reason an empty list shouldn't work.
  • It is custom for the names of functions, variables and object fields in JavaScript to be written with a small letter at the beginning.

You can optimize adding the list item to the list, by using .map() to create the items and appending them all at once. 

$('#VerticalMenu') 
  .append(
    $(sections).map(function () {
      $('<li/>') 
      // ...
    })
  );

BTW, since sections is an array of simple JavaScript objects and not DOM objects it's a bit weird to wrap them in a jQuery object ($(sections).each(...)), because jQuery unwraps them immediately anyway. You should use $.each(sections, ... ) (or $.map(sections, ... )) instead.

You are assigning all items in your lists the same class (Section and SubSection respectively). Unless you remove the class later, this is usually a sign of wrong CSS design. If you leave out the class, then instead of the selector .Section { ... } you can use a descendent selector: #VerticalMenu li { ... }.

One last thing: The use of h4 elements seems wrong to me. Either this is a menu, then you shouldn't be using header elements at all, or the items in AccordionMenu are subheadlines and be using h5 instead.

share|improve this answer
Awesome. This is exactly the kind of feedback I was looking for. Don't feel bad about being nitpicky, it's that kind of stuff that makes for strong development. – MrGrigg Apr 22 '11 at 13:09

Your Answer

 
or
required, but never shown
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.