JS Menu Post Get method

Question

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);
 }
});

Answer 1

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);
 }
});

• Jefferson: idList[data.PageSection - 1]) what is that doing? should it be item ?
• emanresu A: Oops, I misadapted your code. Fixing.

Answer 2

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;
   }