Functional Javascript Practice with AJAX

Question

I am currently doing projects in freecodecamp.com to brush up on my javascript skills and just finished a weather report widget. It does the following:

• Gets api from openweathermap.org and displays the weather based on user's latitude and longitude
• Displays the city, weather description and wind speed
• Can toggle between celsius and ferenheit

I want to become a better javascript developer and use best-practice tools and design patterns. Therefore, I would kindly ask if anyone has design pattern suggestions for making my code more efficient, less redundant, and more functional.

Also, the console displays getCurrentPosition() and watchPosition() are deprecated on insecure origins why is this, and is there an alternative for getting the user's position from directly from the browser?

The following is my javascript code:

if (navigator.geolocation) {
 navigator.geolocation.getCurrentPosition(function(position) {
 var latitude = position.coords.latitude;
 var longitude = position.coords.longitude;
 $.getJSON("http://api.openweathermap.org/data/2.5/weather?lat=" + latitude + "&lon=" + longitude + "&APPID=eee5ab7fffb62d126756d9b810ee1875", function(data) {
 var temp = JSON.stringify(data.main.temp);
 //Convert to Ferenheit
 var temp2 = temp * 9 / 5 - 459.67;
 //Round to 2nd decimal place
 var temp3 = Math.round(temp2 * 10) / 10;
 //Display
 $('#temperature').html(temp3 + " F");
 //Description
 var description = data.weather[0].description;
 //Wind speed
 var wind = JSON.stringify(data.wind.speed);
 //HTML disaply
 $(".report").html("<li>" + data.name + "</li>" + "<li>" + description + "</li><li>" + wind + " knots</li>");
 //Toggle Logic
 $('#celsius').on('click', function() {
 var celsius = temp - 273.15;
 var celsius2 = Math.round(celsius * 10) / 10;
 $('#temperature').html(celsius2 + " C");
 $('#celsius').removeClass('btn-default').addClass('btn-primary');
 $('#ferenheit').removeClass('btn-primary').addClass('btn-default');
 });
 $('#ferenheit').on('click', function() {
 var temp = JSON.stringify(data.main.temp);
 var temp2 = temp * 9 / 5 - 459.67;
 var temp3 = Math.round(temp2 * 10) / 10;
 $('#temperature').html(temp3 + " F");
 $('#ferenheit').removeClass('btn-default').addClass('btn-primary');
 $('#celsius').removeClass('btn-primary').addClass('btn-default');
 });
 //Icons logic
 if (description == "broken clouds" || "scattered clouds") {
 $("i").addClass("wi-cloudy");
 } else if (description == "few clouds") {
 $("i").addClass("wi-cloud");
 } else if (description == "clear sky") {
 $("i").addClass("wi-day-sunny");
 } else if (description == "shower rain" || "rain") {
 $("i").addClass("wi-rain");
 } else if (description == "thunderstorm") {
 $("i").addClass("wi-storm-showers");
 } else if (description == "snow") {
 $("i").addClass("wi-snowy");
 } else if (description == "mist") {
 $("i").addClass("wi-dust");
 };
 });
 });
}

You can find the rest of my code in the gist here.

• elclanrs: Does this work? Your logic doesn't look right, are you sure this is what you want description == "broken clouds" || "scattered clouds"?

Answer 1

• Move your APPID and API url from your code an into constants. That way, they're readily visible and easily configurable.

• Move the creation of your API url into a function, that way it's easily modifiable. In addition, use $.param to construct the query portion of your URL. jQuery encodes the params for you. If you can do ES6, you can simplify using shorthand literal notation and template strings

  function getApiUrl(lat, lon){
   return `${API_URL}?${$.param({lat, lon, APPID})}`;
  }
  

• Not entirely sure why you need to stringify data.main.temp when the logic comes after it expects a number.

• data.main.temp appears to be in Kelvin. It would be better if you name the variable appropriately to show that it is in Kelvin.

• Move your temperature calculations into functions, like convertFromKelvinToCelsius and convertKelvinToFahrenheit.

• It appears that you only get the data once, but your conversions to Celsius and Fahrenheit happen on click. Why not convert them just once, and on click, just display the pre-calculated values?

• In the same spirit, why not render the HTML in advance and just show/hide depending on the click. This avoids you from putting the values in html all the time.

• Construct your data first, then do the HTML. Makes it easier to have all your logic up top, and rendering down below. That way, your data is prepared and all in one place.

• The <i> part, you're repeating code. Why not determine the class name only and addClass to the <i> once instead of repeating the addClass for each condition.

I'll leave the code to you. Feel free to open up another review for a followup.

• Alejandro Gomez: Thanks @joseph-the-dreamer I'll be revising my code and let you know if I got any more questions. Cheers!