JavaScript Weather App

Question 1

I am pretty new to JavaScript, and put together a weather application. I'd like to get some feedback on improvements I could make to the code itself and also possible ways to speed up weather data display.

This is a screenshot of the working site:

Website preview

The basics for how the application works:

It checks if local storage has units set, there are 2 options, imperial and metric. If units doesn't exist, a default unit is picked based upon the browser's language settings. If a unit is set, then the application will use that. You can set the units via buttons on the page.
The application uses geolocation to get the user's coordinates, and generate a link to a json file from open weather map. I then use jsonp to grab the weather data and stick it into local storage as a cache.
I add a timestamp to local storage.
The local storage weather data is parsed and displayed on the page. If the data is older than 30 minutes, it gets downloaded again and the cache is updated.

Here is the HTML for the site:

<!DOCTYPE html>
 <html>
 <head>
 <meta charset=utf-8">
 <title>Local Weather</title>
 <meta description content="Get your Local weather using geolocation.">
 <meta name=viewport content="width=device-width, initial-scale=1">
 <link rel="stylesheet" type="text/css" href="//cdnjs.cloudflare.com/ajax/libs/normalize/3.0.1/normalize.min.css">
 <link rel="stylesheet" type="text/css" href="//fonts.googleapis.com/css?family=Arimo:400,700">
 <link rel="stylesheet" type="text/css" href="assets/style.css">
 <!--[if lt IE 9]>
 <script src="http://html5shiv.googlecode.com/svn/trunk/html5.js"></script>
 <![endif]-->
 </head>
 <body>
 <form id="toggle">
 <button type="button" id="cel" class="inactive" onclick="SetCelsius();">&deg;C</button>
 <button type="button" id="far" class="inactive" onclick="SetFahrenheit();">&deg;F</button>
 </form>
 <article>
 <section id="weather">
 <img alt="loading" id="spinner" src="assets/spinner.gif">
 </section>
 <section id="forecast">
 <noscript>This application requires JavaScript to function. Please enable scripts in your browser.</noscript>
 </section>
 </article>
 <script src="//cdnjs.cloudflare.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
 <script src="assets/weather.js"></script>
 </body>
 </html>

Here is the JavaScript:

$(function SetUnits () {
 switch (localStorage.getItem("Units")) {
 case null:
 if (window.navigator.language == "en-US") {
 localStorage.Units = "imperial";
 $("#far").removeClass("inactive");
 $("#far").addClass("active");
 }
 else {
 localStorage.Units = "metric";
 $("#cel").removeClass("inactive");
 $("#cel").addClass("active");
 }
 break;
 case "metric":
 $("#cel").removeClass("inactive");
 $("#cel").addClass("active");
 break;
 case "imperial":
 $("#far").removeClass("inactive");
 $("#far").addClass("active");
 break;
 }
});
function SetCelsius(){
 localStorage.Units = "metric";
 $("#cel").removeClass("inactive");
 $("#cel").addClass("active");
 $("#far").removeClass("active");
 $("#far").addClass("inactive");
 location.reload();
}
function SetFahrenheit() {
 localStorage.Units = "imperial";
 $("#far").removeClass("inactive");
 $("#far").addClass("active");
 $("#cel").removeClass("active");
 $("#cel").addClass("inactive");
 location.reload();
}
$(function geolocation (){
 if (navigator.geolocation){
 navigator.geolocation.getCurrentPosition(getcoordinates,showError);
 }
 else {
 $("#weather").html("Geolocation is not supported by this browser.");
 }
});
function getcoordinates(position) {
 var lat=position.coords.latitude;
 var long=position.coords.longitude;
 var units=localStorage.getItem("Units");
 var CurrentWeatherURL = "http://api.openweathermap.org/data/2.5/weather?lat="+lat+"&lon="+long+"&units="+units;
 var DailyForecastURL = "http://api.openweathermap.org/data/2.5/forecast/daily?lat="+lat+"&lon="+long+"&units="+units+"&cnt=1";
 if (units == "imperial") {
 getWeather(CurrentWeatherURL, DailyForecastURL, "F", "mph")
 }
 else {
 getWeather(CurrentWeatherURL, DailyForecastURL, "C", "m\/s")
 }
}
function showError(error) {
 switch(error.code) {
 case error.PERMISSION_DENIED:
 $("#weather").html("User denied the request for Geolocation.");
 break;
 case error.POSITION_UNAVAILABLE:
 $("#weather").html("Location information is unavailable.");
 break;
 case error.TIMEOUT:
 $("#weather").html("The request to get user location timed out.");
 break;
 case error.UNKNOWN_ERROR:
 $("#weather").html("An unknown error occurred.");
 break;
 }
}
var data_timestamp=Math.round(new Date().getTime() / 1000);
function getWeather(data_url, forecast_url, temp, wind) {
 $.ajax ({
 url: data_url,
 type: 'GET',
 cache: false,
 dataType: "jsonp",
 success: function(data) {
 localStorage.WeatherCache = JSON.stringify(data);
 },
 error: function (errorData) {
 $("#weather").html("Error retrieving current weather data :: "+ errorData.status);
 }
 });
 $.ajax ({
 url: forecast_url,
 type: 'GET',
 cache: false,
 datatype: "jsonp",
 success: function(data) {
 localStorage.ForecastCache = JSON.stringify(data);
 displayData(temp, wind);
 },
 error: function (errorData) {
 $("#forecast").html("Error retrieving forecast data :: "+ errorData.status);
 }
 });
 localStorage.timestamp = data_timestamp;
};
function displayData(temp_units, wind_units) {
 try {
 // If the timestamp is newer than 30 minutes, parse data from cache
 if ( localStorage.getItem('timestamp') > data_timestamp - 1800){
 var data = JSON.parse(localStorage.WeatherCache);
 var forecast = JSON.parse(localStorage.ForecastCache);
 document.body.style.background = "url('assets/backgrounds/" +data.weather[0].icon+ ".jpg') no-repeat fixed 50% 50%";
 document.body.style.backgroundSize = "cover";
 $("#weather").html('<h2>' + data.name + '</h2><img class="icon" src="assets/icons/'+data.weather[0].icon+'.png"><span id="temp">'+ data.main.temp + ' </span><span id="units">&deg;'+temp_units+'</span><p id="description">'+ data.weather[0].description + '</p><p><span id="humidity">'+ data.main.humidity + '% humidity</span>&nbsp;&nbsp;&nbsp;&nbsp;'+ Math.round(data.wind.speed) + wind_units +' wind</p>');
 $("#forecast").html('<p id="daily">Today\'s Forecast: '+forecast.list[0].weather[0].main+'</p><p>max: '+Math.round(forecast.list[0].temp.max)+'&deg;'+temp_units+' &nbsp;&nbsp;&nbsp;&nbsp;min: ' +Math.round(forecast.list[0].temp.min)+'&deg;'+temp_units+'</p>');
 }
 else {
 geolocation ();
 }
 }
 catch(error){
 window.console && console.error(error);
 }
}

I have this entire project on GitHub here.

The live site is here.

Question 2

A minor suggestion on the overall design: before trying to access geolocation info, I'd give the user a choice to type in a location or "discover my location." This way they don't get taken by surprise when the browser asks for permission, and they have another option if their browser doesn't support geolocation. You can let them type in a location in a free-form way -- an area code, city name, airport code, or whatever they want -- and pass this on to Google Maps API to get latitude/longitude. You can also initially pull the location from an online IP-to-geolocation service.

Question 3

Thanks for commenting. I wrote this script specifically to use geolocation because there are already so many weather apps and jquery plugins that do reverse ip lookups or have users manually input a location. If I develop this further though that is something I would consider adding.

Question 4

I haven't yet reviewed all the code so far, but I do have some suggestions. For example, if you were to combine your SetCelsius and SetFahrenheit functions into a single SetScale which takes one argument (either "metric" or "imperial") and then set the other values based on that argument, you should be able to save a few lines of code and make (at least, that part) a little more expressive.

Question 5

Some parts of your code are extremely repetitive.
I also wonder about reloading the page upon switching units, since you could surely just convert them in-place.
In your first block, what if (somehow) the value in your localStorage is set, but not to "metric" or "imperial"? Your application probably (didn't look too deeply) won't load.
Your unit conversions don't actually work. (Does this qualify for a flag for off-topic?)
You're missing a quote in your meta charset.

I wrote a fiddle. You can see the full version here. Here's a summary of changes:

I made inactive the default styling, instead of a class.
I DRYed up the system selection.
I changed localStorage to use the API instead of direct assignment.
I moved the event handlers into the script to separate them from the structure.
I made temperatures auto-convert when changing units.
I got rid of the  s and replaced it with some styling.

There may have been some changes that I left out.

Question 6

Thank you for taking the time to look over the script I wrote. I agree there is a ton of repetitive code, I wasn't sure what the best way of combining some of the repetitive functions would be. Your advice is helpful. If the units aren't imperial or metric, the code will still work, but everything will be in kelvins. see: api.openweathermap.org/data/2.5/… Of course this depends on open weather map's API remaining the same. I think doing real time conversion makes sense and has better usability. Thanks for posting a fiddle with your improvements.

Schism Schism 3,64517 silver badges31 bronze badges · Accepted Answer · 2014-07-11 02:42:30Z

Some parts of your code are extremely repetitive.
I also wonder about reloading the page upon switching units, since you could surely just convert them in-place.
In your first block, what if (somehow) the value in your localStorage is set, but not to "metric" or "imperial"? Your application probably (didn't look too deeply) won't load.
Your unit conversions don't actually work. (Does this qualify for a flag for off-topic?)
You're missing a quote in your meta charset.

I wrote a fiddle. You can see the full version here. Here's a summary of changes:

I made inactive the default styling, instead of a class.
I DRYed up the system selection.
I changed localStorage to use the API instead of direct assignment.
I moved the event handlers into the script to separate them from the structure.
I made temperatures auto-convert when changing units.
I got rid of the  s and replaced it with some styling.

There may have been some changes that I left out.

Thank you for taking the time to look over the script I wrote. I agree there is a ton of repetitive code, I wasn't sure what the best way of combining some of the repetitive functions would be. Your advice is helpful. If the units aren't imperial or metric, the code will still work, but everything will be in kelvins. see: api.openweathermap.org/data/2.5/… Of course this depends on open weather map's API remaining the same. I think doing real time conversion makes sense and has better usability. Thanks for posting a fiddle with your improvements.

Stack Exchange Network

JavaScript Weather App

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

JavaScript Weather App

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions