A simple weather app in Node.js

Question

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 + '!');
        });
    });
};

Answer 1

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);
});