6
\$\begingroup\$

while practicing Promises, I ended up with a simple weather application which works on the command line. I would highly appreciate it if you can point out any flaws in my code. They can be related to the efficiency, etc. I'm looking for anything you can provide in the review.

package.json:

{
  "name": "weather-app",
  "version": "1.0.0",
  "description": "A simple weather app.",
  "main": "app.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "Hassan Althaf",
  "license": "MIT",
  "dependencies": {
    "request": "^2.67.0",
    "yargs": "^3.30.0"
  }
}

app.js:

var weather = require('./weather.js');
var location = require('./location.js');
var argv = require('yargs')
    .options({
        location: {
            demand: false,
            alias: 'l',
            type: 'string',
            description: 'Enables you to specify a location.'
        }
    })
    .help('help')
    .argv;

if(argv.location) {
    weather(argv.location).then( function(message) {
        console.log(message);
    }).catch( function(error) {
        console.log(error);
    });
} else {
    location().then( function (location) {
        return weather(location);
    }).then( function(output) {
        console.log(output);
    }).catch ( function(error) {
        console.log(error);
    });
}

location.js:

var url = 'http://ipinfo.io';
var request = require('request');

module.exports = function() {
    return new Promise( function(resolve, reject) {
        request({
            url: url,
            json: true
        }, function (error, response, body) {
            if (error || !body) {
                reject('Failed to fetch the location.');
            }

            resolve(body.city);
        });
    });
};

weather.js:

var request = require('request');

module.exports = function (location) {
    return new Promise( function(resolve, reject) {
        location = encodeURIComponent(location);

        if(!location) {
            reject('No location provided!');
        }

        var url = 'http://api.openweathermap.org/data/2.5/weather?q=' + location + '&units=metric&appid=2de143494c0b295cca9337e1e96b00e0';

        request({
            url: url,
            json: true
        }, function (error, response, body) {
            if(error) {
                reject('Failed to fetch the weather!');
            }

            resolve('It\'s ' + body.main.temp + ' degrees Celsius in ' + body.name + '!');
        });
    });
};
\$\endgroup\$
  • 1
    \$\begingroup\$ shouldn't you add return statements below most rejects / resolves to prevent continuing code execution? \$\endgroup\$ – Jan-Stefan Janetzky Dec 1 '15 at 13:47
  • \$\begingroup\$ @Jan-StefanJanetzky I believe the code execution is stopped when you use Promises and you invoke either of those functions passed to it. \$\endgroup\$ – Hassan Althaf Dec 1 '15 at 13:51
4
\$\begingroup\$

Let's start with weather.js.

  • Use Node's querystring module to generate the query string from an object instead of concatenating strings. It's easier to read, plus you don't have to deal with escaping.

  • is appid your API key? You shouldn't put any keys in the code. I suggest you move it out of your code and into an environment variable. That way, it's not part of the history of your version control.

  • url seems constant (except the query part). You should also move that outside into a variable like you did with location.js. Just combine the url with your constructed query string during calls.

  • I would suggest you don't construct the text for resolve in the weather module. Have it resolve the data instead. Let the caller decide what it does with the data. It's better that way in the long run, so that you can just chain tools that speak data rather than presentation.

In location.js

  • Reject with the error object. Let the caller decide what it wants to do with the error. Same as mentioned earlier, it's easier when the caller decides what to do with the data and for functions to just speak in data.

In app.js

You can merge both logic branches together. If you have the location, you can create a pre-resolved promise using the static function Promise.resolve(value). Otherwise, call location(). Then you can have the rest of your logic chain afterwards.

(argv.location ? Promise.resolve(argv.location) : location()).then(function(location){
  return weather(location);
}).then(function(output){
  console.log(output);
}).catch (function(error){
  console.log(error);
});
\$\endgroup\$
  • \$\begingroup\$ then(function(location){ return weather(location); }) could be just then(weather). \$\endgroup\$ – Pavlo Dec 2 '15 at 10:37
  • \$\begingroup\$ @Pavlo True, but it's good to show where the logic in the else fits in and how it looks verbosely. Also, should location need some common post-processing, it's also a good place to do so. \$\endgroup\$ – Joseph Dec 2 '15 at 13:06

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.