3
\$\begingroup\$

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>';
            });
        });
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ You're declaring a function inside a loop. Pull the declaration out. \$\endgroup\$ – Gavin Dec 2 '16 at 2:16
2
\$\begingroup\$

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!

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

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>';
\$\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.