5
\$\begingroup\$

This is the first time I'm trying to use promises in JavaScript so I'm not sure if I have done it correctly, but it seems to work. The problem is that I have ended up with a "nested when" and it doesn't look that great. There probably is a nicer solution out there. I would really appreciate some feedback on how I can improve my code.

I am using the navigator.geolocation to get the position and then I'm using two different APIs to get the current weather and the address of the location.

What I would like it to look like would be something like:

$.when(getPosition())

.then(getWeather(position),getLocationName(position))

.done(displayWeather);

But it doesn't seem to work with two calls in then() like you can have in when().

This is my current solution:

var displayWeather = function(weatherData, locationData) {

  $('#userLocation').text(locationData.results[3].formatted_address);

  $('#weatherDescription').text(weatherData.currently.temperature);

  $('#temperature').text(weatherData.currently.summary);

}


var getPosition = function() {

  var deferred = $.Deferred();

  navigator.geolocation.getCurrentPosition(deferred.resolve, deferred.reject);

  return deferred.promise();

};


var getWeather = function(position) {

  return $.getJSON('https://api.forecast.io/forecast/{APIKEY}/' + position.coords.latitude + ',' + position.coords.longitude + '?callback=?')
     .then(function(data) {
        return data;
    });

}


var getLocationName = function(position) {

  return $.getJSON('https://maps.googleapis.com/maps/api/geocode/json?latlng=' + position.coords.latitude + ',' + position.coords.longitude + '&key={APIKEY}')
    .then(function(data) {
        return data;
  });

}


$(document).ready(function() {

  $.when(getPosition())

    .done(function(position) {

      $.when(getWeather(position), getLocationName(position)).done(displayWeather);

    });

});
\$\endgroup\$
4
\$\begingroup\$
$.when(getPosition())

$.when is only used when listening to multiple promises. Since getPosition is just one and it already returns a promise, $.when isn't required and you can simply chain then to getPosition().

var getWeather = function(position) {

  return $.getJSON('https://api.forecast.io/forecast/{APIKEY}/' + position.coords.latitude + ',' + position.coords.longitude + '?callback=?')
     .then(function(data) {
        return data;
    });

}


var getLocationName = function(position) {

  return $.getJSON('https://maps.googleapis.com/maps/api/geocode/json?latlng=' + position.coords.latitude + ',' + position.coords.longitude + '&key={APIKEY}')
    .then(function(data) {
        return data;
  });

}

Attaching then which just returns the resolved data data is unnecessary. $.getJSON already returns a promise-like object that resolves to data. Any then attached to it will get data in the same way this then receives it.

Additionally, suggesting you use then instead of the jQuery-specific done. then is standard, and allows you to easily move over to the standard Promises.

I usually recommend pushing off the "flow logic" of promises to the caller instead of spreading it across different functions. This allows you to easily write functions as promise-returning operations while keeping all the "flow logic" in one place.

On other stuff, suggesting you move the url into a variable. This keeps your AJAX operation short and free from the string concatenation madness. A;so recommending using named function instead of function expressions. They're easier to debug as their names appear in the stack trace.

Your code could be simplified into:

function getPosition () {

  return $.Deferred(function(deferred){
    navigator.geolocation.getCurrentPosition(deferred.resolve, deferred.reject);
  }).promise();

};


function getWeather (position) {
  var url = 'https://api.forecast.io/forecast/{APIKEY}/' + position.coords.latitude + ',' + position.coords.longitude + '?callback=?'
  return $.getJSON(url);
}


function getLocationName (position) {
  var url = 'https://maps.googleapis.com/maps/api/geocode/json?latlng=' + position.coords.latitude + ',' + position.coords.longitude + '&key={APIKEY}';
  return $.getJSON(url);
}


$(document).ready(function() {

  getPosition().then(function(position){

    return $.when(getWeather(position), getLocationName(position));

  }).then(function(weatherData, locationData){

    $('#userLocation').text(locationData.results[3].formatted_address);
    $('#weatherDescription').text(weatherData.currently.temperature);
    $('#temperature').text(weatherData.currently.summary);

  });

});

If you can write using ES6, the code becomes simpler and you can drop the jQuery dependency for the data gathering functions. ES6 now has native promises as well as template strings, and modern browsers have fetch. This reduces jQuery footprint to only the DOM-interacting operations.

function getPosition() {
  return new Promise((resolve, reject) => navigator.geolocation.getCurrentPosition(resolve, reject));
};


function getWeather(position) {
  const url = 'https://api.forecast.io/forecast/${APIKEY}/${position.coords.latitude},${position.coords.longitude}?callback=?';
  return fetch(url).then(response => response.json());
}


function getLocationName(position) {
  const url = 'https://maps.googleapis.com/maps/api/geocode/json?latlng=${position.coords.latitude},${position.coords.longitude}&key=${APIKEY}';
  return fetch(url).then(response => response.json());
}


$(document).ready(function() {

  getPosition()
    .then(position => Promise.all(getWeather(position), getLocationName(position)))
    .then(function(data){
      const weatherData = data[0];
      const locationData = data[1];

      $('#userLocation').text(locationData.results[3].formatted_address);
      $('#weatherDescription').text(weatherData.currently.temperature);
      $('#temperature').text(weatherData.currently.summary);
    });

});
\$\endgroup\$
  • \$\begingroup\$ Thank you so much for the answer! One thing that didn't work for me was to remove the then after the $.getJSONlike you suggested. The weatherDataand locationDatais then an array instead of a json object. I will definitely try to write it using ES6 instead though. Looks a lot nicer! Thanks again! \$\endgroup\$ – Veronika May 28 '16 at 19:12
1
\$\begingroup\$

You could use native promises... It took me some experimenting to get used to the new javascript features like destructuring, template strings, .reduce, and promises, but I like how the code looks...

You can return a Promise from getPosition:

var getPosition = () => new Promise((resolve, reject) => 
  navigator.geolocation.getCurrentPosition(resolve, reject))

$.getJSON result can be treated like a promise. So getWeather can take coords and destructure into latitude and longitude right in the argument list, then using template strings you can just return $.getJSON()

var getWeather = ({ latitude, longitude }) =>
    $.getJSON(`https://api.forecast.io/forecast/${FORECAST_API_KEY}/${latitude},${longitude}?callback=?`);

You can call getPosition() and call Promise.all() with promises to get the weather and location data at the same time. You can modify the value you return from .then() or return another promise from it. So what I do here is convert the result of getWeather() into an object with a weather property with the result, same for location, then use .reduce() on Promise.all waiting on those two promises, then Object.assign() to merge all those results into one object like { position: {}, weather: {}, location: {} }

$(() => {
  getPosition()
  .then(position =>
    Promise.all([
      getWeather(position.coords).then(weather => ({ weather })),
      getLocation(position.coords).then(location => ({ location }))
    ]).then(all => all.reduce((acc, val) => Object.assign(acc, val), { position }))
  )
  .then(data => {
      console.log('data:', data);
      displayWeather(data.weather, data.location);
  })
  .catch(err => alert('ERROR!\r\n' + JSON.stringify(err)))
})

So Promise.all() ends up with an array that looks like this (... is the data from the services):

[{weather:{...}, {location:{...}}]

Reduce starts with the accumulator (acc) as {postion:{...}} and operates on each element in that array calling Object.assign with the accumulator and the array value, returning the result with merged properties.

The Promise.all() could be changed to use the array indices with the raw results and it looks cleaner, but I like to avoid accidentally using the wrong index for results. Also you can easily just add another ajax call without changing the then...

Promise.all([
  getWeather(position.coords),
  getLocation(position.coords)
]).then(all => ({
  weather: all[0],
  location: all[1],
  position
}))

I inserted the code as a snippet, but I can't seem to enable location running it. It works on jsfiddle though if you set the api keys...

let FORECAST_API_KEY = 'GET YOUR OWN';
// https://darksky.net/dev/account
let GOOGLE_API_KEY = 'GET YOUR OWN';
// https://developers.google.com/maps/documentation/geocoding/get-api-key

var displayWeather = function(weatherData, locationData) {
  $('#userLocation').text(locationData.results[3].formatted_address);
  $('#weatherDescription').text(weatherData.currently.summary);
  $('#temperature').text(weatherData.currently.temperature);
}

var getPosition = () => new Promise((resolve, reject) => {
  navigator.geolocation.getCurrentPosition(resolve, reject);
});

var getWeather = ({ latitude,longitude }) => {
  return $.getJSON(`https://api.forecast.io/forecast/${FORECAST_API_KEY}/${latitude},${longitude}?callback=?`);
}

var getLocation = ({latitude, longitude }) => {
  return $.getJSON(`https://maps.googleapis.com/maps/api/geocode/json?latlng=${latitude},${longitude}&key=${GOOGLE_API_KEY}`)
}


$(() => {
  getPosition()
  .then(position =>
    Promise.all([
      getWeather(position.coords).then(weather => ({ weather })),
      getLocation(position.coords).then(location => ({ location }))
    ]).then(all => all.reduce((acc, val) => Object.assign(acc, val), { position }))
  )
  .then(data => {
      console.log('data:', data);
      displayWeather(data.weather, data.location);
  })
  .catch(err => alert('ERROR!\r\n' + JSON.stringify(err)))
})
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<p>
Replace <a href="https://darksky.net/dev/account">forecast.io</a> and
<a href="https://developers.google.com/maps/documentation/geocoding/get-api-key">google</a> api keys with your own!
</p>
<p><span id="userLocation">Loading...</span></p>
<p><span id="weatherDescription">Loading...</span></p>
<p><span id="temperature">Loading...</span></p>

\$\endgroup\$
  • \$\begingroup\$ This is a very clean and modern way of doing things. Also, one may look at forkJoin() as an option. \$\endgroup\$ – Igor Soloydenko May 9 '17 at 3:32

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.