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

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

• You're declaring a function inside a loop. Pull the declaration out. – Gavin Dec 2 '16 at 2:16

## 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!

• I have rejected your edit on the question. Feel free to add your remarks to the answer instead. That's what answers are for. – Mast Dec 2 '16 at 8:00

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

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