0
\$\begingroup\$

This is one of my first programs working with APIs and I was hoping to get some input in the form of suggestions, recommendations or corrections. The goal is to register the user location by IP, derive a latitude and longitude from the ip-api public site, pass this to Open Weather’s API for weather details and description, and use this information to search for a Flickr image of the location.

I have only been working with Javascript for a few months so any suggestions would be very much appreciated. If you can recommend any sources to remediate any shortcomings that would be appreciated as well.

var displayData = {
  city: document.querySelector("#city"),
  temperature: document.querySelector("#temperature"),
  weather: document.querySelector("#weather"),
  icon: document.querySelector("#icon"),
  source: document.querySelector("#source")
  };

function getWeather(latitude, longitude) {
  if (window.XMLHttpRequest){
    var xhr = new XMLHttpRequest();
    xhr.addEventListener("load", function(){
      var response = JSON.parse(xhr.responseText);
      displayData.city.innerHTML = response.name;
      displayData.temperature.innerHTML = response.main.temp;
      var detailWeather = response.weather[0];
      var description = detailWeather.description;

      flickrLoad(latitude, longitude, description); 

      displayData.weather.innerHTML = description;
      displayData.icon.src = "http://openweathermap.org/img/w/" + detailWeather.icon + ".png";
    }, false);

    xhr.addEventListener("error", function(err){
      alert("Initial request error");
      }, false);

    var target = "http://api.openweathermap.org/data/2.5/weather?lat=" + latitude + "&lon=" + longitude + "&appid=54e7bed8487f1b7de520fa68a9080abe&units=imperial" ;

  xhr.open("GET", target, true);
  xhr.send();
  } else {
    alert("Initial XMLHttpRequest");
  }
}


function flickrLoad(latitude, longitude, description){
    var searchDesc, splitDesc;
    splitDesc = description.split(" ");
    searchDesc = splitDesc.slice(-1)[0];
    console.log(searchDesc);

    if (window.XMLHttpRequest){
    var body = document.getElementsByTagName('body')[0];
    var xhr = new XMLHttpRequest();
    xhr.addEventListener("load", function(){
        var response = xhr;
        var fullText = response.responseText;
        var segment = (fullText.slice(14, -14)) + "}";
        var fullJSON = JSON.parse(segment); 
        var imgURL = fullJSON.photos.photo[0].url_l;
        var img = new Image();
        img.src = imgURL;
        body.style.backgroundImage = 'url(' + imgURL + ')';       

    }, false);
    xhr.open("GET", "https://api.flickr.com/services/rest/?method=flickr.photos.search&api_key=c004bd46564d26383d37d88c1cbd4154&lat=" + latitude + "&lon=" + longitude + "&accuracy=1&tags=" + searchDesc + "&sort=relevance&extras=url_l&format=json", true);
    xhr.send();
}}


function findIP(){
  if (window.XMLHttpRequest){
    var xhr = new XMLHttpRequest();
    xhr.addEventListener("load", function(){
        var response = JSON.parse(xhr.responseText);
        console.log(response.lat, response.lon);
        getWeather(response.lat, response.lon);
        displayData.source.innerHTML = "From: " + response.query;
    }, false);
    xhr.open("GET", "http://ip-api.com/json", true);
    xhr.send();
}}

findIP();
body {
  background-color: #696969;
  font-family: 'Open Sans', sans-serif;
}

#imageSupport {
  width: 100%;
  height: 100%;
  background-size: 100%;
}


#shell {
  position: absolute;
  top: 35%;
  width: 100%;
  text-align: center;
}


#main {
  width: 30%;
  margin: auto;
  border: solid 1px black;
  border-radius: 5px;
  background-color: rgba(255, 255, 255, 0.7)
}

#footer {
  margin: 0 5px 0 0;
  position: fixed;
  bottom: 10px;
  right: 10px;   
}

a {
  color: #800000;
}
<!DOCTYPE html>
<html>
<head>
  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width, initial-scale=1">
  <title>Weather App 25 Sept</title>
  <link href="https://fonts.googleapis.com/css?family=Open+Sans" rel="stylesheet">
  <link rel="stylesheet" href="wx25.css">
</head>
<body >
  <div id="imageSupport">
  <div id="shell">

  <section id="main">
    <h1 id="city">Weather Web App</h1>
    <h4 id="source">Local IP</h4>
    <p id="temperature"></p>
      <span id="weather"> By Cameron</span><br>
      <img id="icon">
 
  </section>
  </div>
  </div>

<footer id="footer">

      <a href="http://flickr.com/services/api/"> Flickr</a> and <a
      href="http://openweathermap.org"> Openweathermap.org</a>

</footer>

<script src="wx25.js"></script>
</body>
</html>

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$ 
var xhr = new XMLHttpRequest();

Consider using the newer fetch. it's simpler to use. Although it's not supported in about half the browsers in the market, there's a polyfill.

Should you still want to continue down the XMLHttpRequest path, consider wrapping the XHR logic in a function for reusability. You're repeating your code significantly on all 3 functions.

Whether it's fetch or XMLHttpRequest, consider using Promises. It allows for better async control than just plain callbacks. fetch returns promises by default. For XMLHttpRequest, you might want to wrap it in one.

Here's an example of how to wrap xhr with a promise:

function promisingXHRGet(url, options){
  return new Promise(function(resolve, reject){
    var xhr = new XMLHttpRequest();
    xhr.addEventListener("load", function(){
      resolve(JSON.parse(xhr.responseText));
    }, false);
    xhr.open("GET", url, true);
    xhr.send();
  });
}

Here's how the could could have looked like using fetch and promises.

function getWeather(latitude, longitude) {
  return fetch(...).then(function(response){
    // Convert response object to JSON before passing it along
    return response.json();
  });
}

function getFlickrImage(latitude, longitude, description){
  return fetch(...).then(function(response){
    // Convert response object to JSON before passing it along
    return response.json();
  });
}

function getIP(){
  return fetch(...).then(function(response){
    // Convert response object to JSON before passing it along
    return response.json();
  });
}

function getInfo(){
  return getIP().then(function(data){
    // Do stuff with IP data
    // Retrieve weather data
    return getWeather(...);
  }).then(function(data){
    // Do stuff with weather data
    // Retrieve Flickr image
    return getFlickrImage(...);
  }).then(function(data){
    // Do stuff with flickr data
  });
}

getInfo();

The code isn't very different. you still have 3 functions to interface with the API and there's your logic.

However, notice that we've reversed the responsibility. The logic now resides in the caller side of the code, in getInfo, instead of in the 3 functions. This allows the 3 functions to be reusable and not be bound to business/UI logic.

Also, with promises, your code now appears linear with the use of then. The magic of then is that when you return a promise from the callback, the next then's callback will not execute until that resolves. That gives you the feeling that the code is "waiting".

Now onto your HTML and CSS.

One issue that's as clear as day is the use of id. First, IDs must be unique on the page. Although nothing will stop you from reusing an ID, there's no guarantee libraries and APIs will work. When you drop your code onto a page, there's no guarantee that it will not collide with other elements. Use classes to be safe.

Another problem with IDs is that it has a very high specificity. ID styles can only be overridden by either:

  • A style declaration that uses the ID but comes after your styles.
  • A style declaration that uses an ID and another attribute.
  • An inline style or style set using JavaScript.
  • Any selector, but with properties using !important.

You don't want your CSS specificity to be very high. Otherwise, you'll be eating up this list up to the last item until you can't style the element anymore.

With both issues, consider using the BEM naming scheme. In a gist, it solves both problems by 1) using classes and 2) classes have very low specificity. This will give you collision-free naming as well as easy overriding of styles.

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ thank you very much for taking the time to provide this review. I had heard of "Promises" but haven't covered them, I will now. "Fetch" is not something I had heard of...Thank you for the references, I appreciate the help. \$\endgroup\$
    – Cameron
    Sep 26, 2016 at 19:58

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.