Getting JSON with jQuery, creating a function that displays the data from two separate feeds

Question

I have written my first JavaScript program from scratch and am looking for advice to check whether it is efficient and effective or how it could be improved.

        var apiURL = 'https://services.domain.com';

    function dataPlanOutput(countryCode) {
        document.getElementById("dataplan_list").innerHTML = "";
        network_url = apiURL + '/api/v4/countries/' + countryCode ;
        network_new_url = "http://cors.io/?" + network_url;
        $.getJSON(network_new_url, function (networkGroup) {
            $.each(networkGroup.list, function (i, list) {
                var countryName = list.region;
                var networkGroupId = list.networkGroupId;

                function myDataPlan(networkGroupId, countryCode) {
                    data_url = apiURL + '/api/v4/networkGroups/' + networkGroupId + '/plansExt?countryCode=' + countryCode;
                    data_new_url = "http://cors.io/?" + data_url;
                    $.getJSON(data_new_url, function (dataPlan) {
                        $.each(dataPlan.list, function (i, list) {
                            //Price
                            content = '<tr>';
                            var currencyLetter = list.currency;
                            if (currencyLetter == 'USD') {
                                var curencySymbol = currencyLetter.replace("USD", "$");
                            }
                            else if (currencyLetter == 'GBP') {
                                var curencySymbol = currencyLetter.replace("GBP", "£");
                            }
                            else if (currencyLetter == 'JPY') {
                                var curencySymbol = currencyLetter.replace("JPY", "¥");
                            }
                            else if (currencyLetter == 'EUR') {
                                var curencySymbol = currencyLetter.replace("EUR", "€");
                            };
                            content += '<td>' + curencySymbol + list.price + '</td>';
                            //Data Limits
                            var dataLimit;
                            if (list.dataUnlimited) {
                                dataLimit = 'Unlimited';
                            }
                            else {
                                var dataLimitInKB = list.dataLimitInKB;
                                if ((dataLimitInKB / (1024.0 * 1024.0)) >= 1.0) {
                                    var measure = " GB";
                                    var data = dataLimitInKB / ((1024.0) * (1024.0));
                                }
                                else if (dataLimitInKB / 1024.0 >= 1.0) {
                                    var measure = " MB";
                                    var data = dataLimitInKB / 1024.0;
                                }
                                data = Math.round(data * 100) / 100;
                                dataLimit = data + measure;
                            }
                            content += '<td>' + dataLimit + '</td>';
                            //Data Length
                            content += '<td>' + list.validityPeriodInDays + '&nbsp;Days</td>';
                            content += '</tr>';
                            $(content).appendTo("#dataplan_list");
                        });
                    });
                }
                myDataPlan(networkGroupId, countryCode);
                document.getElementById('dataplan_list').innerHTML += '<tr><td>' + countryName + '<td></tr>';
            });
        });
    }

Answer 1

Code Cohesion

A great way to simplify code is to break things up so that each section focuses primarily on one specific task. I think your biggest problems with this are in your $.forEach(networkGroup.list loop, in which it appears you are mixing three separate tasks:

1. getting the symbol
2. getting the data limit
3. producing html

---

Price: getting the symbol

Here's a quick tip to simplify converting the currencyLetter to the currencySymbol. Instead of using a list of if else blocks, it would be best to factor this part of the code out into its own function and use an object to map the keys:

function getCurrencySymbol(currencyLetter) {
  return {
    "USD": "$",
    "GBP": "£",
    "JPY": "¥",
    "EUR": "€"
  }[currencyLetter];
}

This shortens the code, makes it easy to add other currencies, and allows the function to be used in other places.

$.each(dataPlan.list, function (i, list) {
  var currencySymbol = getCurrencySymbol(list.currency); // note you misspell curencySymbol

  //Price
  content = '<tr>';
  content += '<td>' + curencySymbol + list.price + '</td>';
  //Data Limits
  // ...

Data Limits: getting the data limit

I'd also recommend breaking out the part of code where you find the data limit into its own function for similar reasons:

function getDataLimit(list) {
  if (list.dataUnlimited) {
    return = 'Unlimited';
  }
  else {
    var dataLimitInKB = list.dataLimitInKB;
    var measure;
    var data;

    if ((dataLimitInKB / (1024.0 * 1024.0)) >= 1.0) {
      measure = " GB";
      data = dataLimitInKB / ((1024.0) * (1024.0));
    }
    else if (dataLimitInKB / 1024.0 >= 1.0) {
      measure = " MB";
      data = dataLimitInKB / 1024.0;
    }

    data = Math.round(data * 100) / 100;
    return data + measure;
  }
}

Now the code in the $.forEach loop is focused primarily on generating the HTML which makes it much easier to see what's going on:

$.forEach() {
  //Price
  content += '<td>' + getCurrencySymbol(list.currency) + list.price + '</td>';

  //Data Limits
  content += '<td>' + getDataLimit(list)+ '</td>';

  //Data Length
  content += '<td>' + list.validityPeriodInDays + '&nbsp;Days</td>';
  content += '</tr>';
  $(content).appendTo("#dataplan_list");
}

---

It would be a good idea to break up the code even more to limit how far the indentation gets. Currently, there are too many things nested inside of each other which adds a lot of complexity.

That all said, this is some pretty intense code for some of your first JavaScript. Nice work!

Answer 2

There is room for a lot more improvements but here is one suggestion.

Instead of

 var currencyLetter = list.currency;
 if (currencyLetter == 'USD') {
   var curencySymbol = currencyLetter.replace("USD", "$");
 }
 else if (currencyLetter == 'GBP') {
   var curencySymbol = currencyLetter.replace("GBP", "£");
 }
 else if (currencyLetter == 'JPY') {
   var curencySymbol = currencyLetter.replace("JPY", "¥");
 }
 else if (currencyLetter == 'EUR') {
   var curencySymbol = currencyLetter.replace("EUR", "€");
 };

Use a object for the currency converstion.

   var currencySybolsByAcronym = {
    'USD': '$',
    'GBP': '£',
    'JPY': '¥',
    'EUR': '€',
   }

And then simply

  //Price
  var content = '<tr>' +
    '<td>' + currencySybolsByAcronym[list.currency] + list.price + '</td>';