patternjavascriptMinor
JavaScript Weather App
Viewed 0 times
javascriptappweather
Problem
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:
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:
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");
This is a screenshot of the working site:
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:
Local Weather
°C
°F
This application requires JavaScript to function. Please enable scripts in your browser.
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");
Solution
- 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
localStorageis 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
inactivethe default styling, instead of a class.
- I DRYed up the system selection.
- I changed
localStorageto 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.
Context
StackExchange Code Review Q#56705, answer score: 5
Revisions (0)
No revisions yet.