2
\$\begingroup\$

I have this JS ajax GET that returns menus to be used in a view. I wanted to see if there is any more way to write this method. Its a lot of code and just wanted to make sure there is no better way to do this?

 console.log('Processing Login ajax GetMenuLinkList')
 var url = "/login/GetMenuLinkList";
 $.ajax({
    type: "GET",
    data: param = "",
    contentType: "application/json; charset=utf-8",
    dataType: "json",
    url: url,
     success: function (data, status) {
         console.log(data);
         $('#favoritepagesListBlock').empty();
         $('#meetingsanddisclosuresBlock').empty();
         $('#memberinstitutionBlock').empty();
         $('#orderListBlock').empty();
         var orderShowMore = false;
         var subscriptionShowMore = false;
         $.each(data, function (index, data) {
             switch (data.PageSection) {
                case 1:
                     $('#favoritepagesListBlock').append("<a class='link-text' href='"+data.Url+"'>" + data.PageName + "</a>");
                     break;
                 case 2:
                     $('#meetingsanddisclosuresBlock').append("<a class='link-text' href='" + data.Url + "'>" + data.PageName + "</a>");
                     break;
                 case 3:
                     $('#memberinstitutionBlock').append("<a class='link-text' href='" + data.Url + "'><b>" + data.PageName + "</b></a>");
                     break;
                 case 4:
                     $('#orderListBlock').append("<a class='link-text' href='" + data.Url + "'>" + data.PageName + "</a>");
                     orderShowMore = data.ShowMore;
                     break;
                 case 5:
                     $('#subscriptionListBlock').append("<a class='link-text' href='" + data.Url + "'>" + data.PageName + "</a>");
                     subscriptionShowMore = data.ShowMore;
                     break;
             }
         });


         if (orderShowMore = true) {
                 $('#orderListBlock').append("<a class='show-more' href='/home/store/order-history'>View More</a>");
         }
         if (subscriptionShowMore=true) {
                 $('#subscriptionListBlock').append("<a class='show-more' href='/home/store/subscriptions'>View More</a>");
         }


    },
    error: function (request, status, error) {
     //   alert(request.responseText);
    }
});
\$\endgroup\$
2
\$\begingroup\$

First, make your code work. orderShowMore = true is assigning true to orderShowMore - the code in the block after this is inevitably going to get executed. Just go if(orderShowMore).

Second, don't use two variables named data - while scoping may make it unambiguous to the compiler, it's confusing to anyone reading your code. Also, consistent indenting goes a long way.

You really don't need a switch statement there - you're basically repeating almost the same code four or five times. You could assign all the ids to a variable - probably a good idea, since I'm assuming you want to clear $('#subscriptionListBlock') so it doesn't fill up with junk.

This seems like a perfect oppurtunity to use template strings - you're inserting values into a string, this is pretty much what they're made for.

You might actually want to look up the $.ajax docs - You're meant to call with (url, settings) not ({url: url}). Also, if you're not sending any data to the server, cut that out - right now, you're assigning an undeclared variable param to an empty string, which could cause errors if run in strict mode.

Use let and const over var - var allows redeclaration, which can cause problems and make debugging harder.

Final code:

console.log('Processing Login ajax GetMenuLinkList')
let url = "/login/GetMenuLinkList";
const idList = ['#favoritepagesListBlock','#meetingsanddisclosuresBlock','#memberinstitutionBlock','#orderListBlock','#subscriptionListBlock']
$.ajax(url, {
    type: "GET",
    contentType: "application/json; charset=utf-8",
    dataType: "json",
    success: function (data, status) {
        console.log(data);
        for(id of idList) $(id).empty();
        let orderShowMore = false;
        let subscriptionShowMore = false;
        for(let item of data){
            $(idList[item.PageSection - 1]).append(`<a class='link-text' href='${item.Url}'>${
                item.PageSection == 3 ?
                `<b>${item.PageName}</b>` :
                iiitem.PageName
            }</a>`);
            if(item.PageSection == 4) orderShowMore = true;
            if(item.PageSection == 5) subscriptionShowMore = true;
        }

        if (orderShowMore) {
            $('#orderListBlock').append("<a class='show-more' href='/home/store/order-history'>View More</a>");
        }
        if (subscriptionShowMore) {
            $('#subscriptionListBlock').append("<a class='show-more' href='/home/store/subscriptions'>View More</a>");
        }
    },
    error: function (request, status, error) {
        //console.log(request.responseText);
    }
});
\$\endgroup\$
2
  • \$\begingroup\$ idList[data.PageSection - 1]) what is that doing? should it be item ? \$\endgroup\$
    – Jefferson
    Nov 17 at 12:54
  • \$\begingroup\$ Oops, I misadapted your code. Fixing. \$\endgroup\$
    – emanresu A
    Nov 17 at 19:00
2
\$\begingroup\$

A short review;

  • You inlined the success function, I would use a properly named function outside of that block

  • This

       $('#favoritepagesListBlock').empty();
       $('#meetingsanddisclosuresBlock').empty();
       $('#memberinstitutionBlock').empty();
       $('#orderListBlock').empty();
    

    can be

       $('#favoritepagesListBlock,#meetingsanddisclosuresBlock,#memberinstitutionBlock,#orderListBlock').empty();
    
  • orderShowMore = data.ShowMore; seems like a bug, I would write orderShowMore = orderShowMore || data.ShowMore; Same thing for subscriptionShowMore

  • Your error handling is empty, you should fix that ;)

  • In production code, your success function should not write to the console

  • You can apply DRY techniques, a lot of the code looks copy pasted, especially this part;

      switch (data.PageSection) {
                  case 1:
                       $('#favoritepagesListBlock').append("<a class='link-text' href='"+data.Url+"'>" + data.PageName + "</a>");
                       break;
                   case 2:
                       $('#meetingsanddisclosuresBlock').append("<a class='link-text' href='" + data.Url + "'>" + data.PageName + "</a>");
                       break;
                   case 3:
                       $('#memberinstitutionBlock').append("<a class='link-text' href='" + data.Url + "'><b>" + data.PageName + "</b></a>");
                       break;
                   case 4:
                       $('#orderListBlock').append("<a class='link-text' href='" + data.Url + "'>" + data.PageName + "</a>");
                       orderShowMore = data.ShowMore;
                       break;
                   case 5:
                       $('#subscriptionListBlock').append("<a class='link-text' href='" + data.Url + "'>" + data.PageName + "</a>");
                       subscriptionShowMore = data.ShowMore;
                       break;
               }
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.