1
\$\begingroup\$

I have been working on a simplistic weather application that utilizes the openweathermap.com API. I have an ajax call to make an HTTP request to get a JSON object that contains the data which is returned in the success callback from the HTTP request. I use JQuery to display the response object's data. All of which reside in a function that takes the browser's geolocation coordinates and structures the HTTP request with those parameters. I also have a setInterval function to make a new HTTP request to keep the data up-to-date - for accurate reporting. I feel that there is a better way to keep the data current a little better than how I am doing it. You can view the project on my GitHub page.

HTML:

<!DOCTYPE html>
<html>

<head>
  <meta charset="UTF-8">
  <title>Weather App</title>
  <meta name="viewport" content="width=device-width, initial-scale=1">

  <link rel='stylesheet prefetch' href='https://maxcdn.bootstrapcdn.com/bootstrap/3.3.5/css/bootstrap.min.css'>
  <link rel="stylesheet" href="css/weather.css">


</head>

<body>

  <div class="container-fluid all">
    <div class="jumbotron">
      <h1 class="text-center">Weather App</h1>
    </div>
    <div class="row well">
      <div class="col-md-12 " id="city">
        <h2 class="text-center">Weather for</h2>
        <h2 class="text-center data-item" id="city-text"></h2>
      </div>

      <div class="row">
        <div class="col-md-4 " id="weather">
          <img src="" id="weatherImg" class="center-block">
          <h3 class="text-center data-item" id="weather-text"></h3>
        </div>
        <div class="col-md-4 " id="temp">
          <h3 class="text-center">Temperature</h3>
          <h3 class="text-center data-item" id="temp-text"></h3>
          <div class="checkbox text-center">
            <label><input type="checkbox" id="tempMode" value="2">
          C&deg
        </label>
          </div>
        </div>
        <div class="col-md-4 " id="wind">
          <h3 class="text-center">Wind Speed</h3>
          <h3 class="text-center data-item" id="wind-text"></h3>
        </div>
      </div>
    </div>
    <div class="footer">
      <div class="container">
        <p class="copyright">Copyright © Brandon Gottschling 2016. All Rights Reserved. Made with <a href="http://www.openweathermap.org">openweathermap</a>.</p>
      </div>
    </div>

  </div>
  <script src='http://cdnjs.cloudflare.com/ajax/libs/jquery/2.1.3/jquery.min.js'></script>
  <script src='http://maxcdn.bootstrapcdn.com/bootstrap/3.3.5/js/bootstrap.min.js'></script>
  <script src="js/weather.js"></script>

</body>

</html>

CSS:

body{
  background-color: #393276;
}

.data-item{
  color: #444;
}

.jumbotron, .well{
  background: #888;
  color: white;
}

.copyright{
  color: #222;
}

JavaScript:

var tempMode = 1;



function getWeather(lat, lon) {
  var apiURI = "http://api.openweathermap.org/data/2.5/weather?lat=" + lat + "&lon=" + lon + "&appid=06170c100199dbae1e223cc3dfad960b";

  $.ajax({
    url: apiURI,
    dataType: "json",
    type: "GET",
    async: "false",
    success: function(resp) {

      $("#tempMode").on("click", function() {
        if (this.checked) {
          $("#temp-text").html(cels.toFixed(1) + " C&deg");
          console.log("checked");
        } else
          $("#temp-text").html(fahr.toFixed(0) + " F&deg");
      });
      console.log(apiURI);
      console.log(resp.name);
      if (resp.name) {
        $("#city-text").html(resp.name + ", " + resp.sys.country);
      }
      if (resp.wind) {
        var knots = resp.wind.speed * 1.9438445;
        $("#wind-text").html(knots.toFixed(1) + " Knots");
      }
      if (resp.main.temp) {
        var fahr = (resp.main.temp * 9 / 5) - 459.67;
        var cels = (resp.main.temp - 273.15);
        if (cels > 24){ 
          $("#temp-text").css("color", "red");
        } else if (cels < 18){
          $("#temp-text").css("color", "blue");
        }
        $("#temp-text").html((tempMode === 1 ? fahr.toFixed(0) + " F&deg" : cels.toFixed(0) + " C&deg"));
      }
      if (resp.weather) {
        var imgURL = "http://openweathermap.org/img/w/" + resp.weather[0].icon + ".png";
        console.log(imgURL)
        $("#weatherImg").attr("src", imgURL);
        $("#weather-text").html(resp.weather[0].description);
      }   
    },
    error: function(resp) {
       alert("Error: " + e);
       clearInterval(updateinter);
    }
  });
}

function getLocation() {
  if ("geolocation" in navigator) {
    navigator.geolocation.getCurrentPosition(function(position) {
      getWeather(position.coords.latitude, position.coords.longitude);
    })
  } else {
    alert("geolocation not available" + e);
    clearInterval(updateinter);
  }
}
var i = 0;
var updateinter = setInterval(function(){
  console.log("iteration# " + i++);
  if ("geolocation" in navigator) {
    navigator.geolocation.getCurrentPosition(function(position) {
      getWeather(position.coords.latitude, position.coords.longitude);
    })
  } else {
    alert("geolocation not available" + e);
  }
}, 300000);


getLocation();
\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Nice script. I tried it in Chromium and looked at the console a bit, first thing to note is the warning about deprecated getCurrentPosition on insecure origins, which you might want to take a look at.

From the design perspective I dislike the Celcius check box a bit - it would be nicer if the temperature label could be clicked to switch and has some affordance to make it obvious that it's clickable.

But enough of that, the code looks readable and since there's not too much going on it's easy to follow.

As you wrote the duplication with getLocation should be removed, e.g.:

var updateIntervalID;

// ...

if ("geolocation" in navigator) {
  function getLocation() {
    navigator.geolocation.getCurrentPosition(function(position) {
      getWeather(position.coords.latitude, position.coords.longitude);
    });
  }

  getLocation();
  updateIntervalID = setInterval(getLocation, 300000);
} else {
  alert("geolocation not available");
}

The error reporting also seems buggy - at least I couldn't find a definition for the e variable.

Clearing the interval is probably not necessary, or at least if there's only a transient error I'd rather have a button somewhere that restarts the cycle and indicates that there was an error.

The conversion between Celcius and Fahrenheit could be moved into its own function.

The click binding for the temperature button should also be done only once at the beginning. Move the necessary data into some higher scope instead, so that it can still access it. Also reuse the formatting function as that code is currently duplicated as well.

The names could be less truncated - temp is (at least in my mind) usually associated with temporary, not temperature.

\$\endgroup\$
2
  • \$\begingroup\$ I made some changes based off of your suggestions and was hoping you might be able to check out my revised code which can be found here: codepen.io/brandon-gottschling/pen/KVbEjv?editors=1010. It was hard to take the click handler out of the unit conversion function as it was necessary for the conversion to take place, so I just have a general purpose temperature format function. Its also hard to store asynchronous responses, so I used the data handler function to format most of everything else. Let me know what you think. \$\endgroup\$
    – brndng
    Commented Feb 20, 2016 at 5:47
  • \$\begingroup\$ @brndng you might want to post that as a new question; line 27/8 are very redundant, line 87 doesn't look like it can work; otherwise looks okay \$\endgroup\$
    – ferada
    Commented Feb 22, 2016 at 17:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.