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

• Gavin: You're declaring a function inside a loop. Pull the declaration out.

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!

• Mast: I have rejected your edit on the question. Feel free to add your remarks to the answer instead. That's what answers are for.

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