Finding clients in a 100 km circle

Posted on

Problem

I needed a node app that:

• reads a list in text file format from URL
• parses the rows on the list. (fields: id, name, longitude, latitude)
• finds names of the people who live maximum 100 kms away from a set location.
• sorts the list by user id

Could you take a look and let me know what I could have done better here?

To run:

nodejs scriptname.js

`````` var ClientsNearby = (function(){
'use strict';
var contacts = [],
dublinOffice = {
latitude: 53.3381985,
longitude: -6.2592576
};
var request = require('request');
function getDistanceFromLatLonInKm(lat2,lon2) {
var R = 6371; // Radius of the earth in km
var dLat = toRadians(lat2-dublinOffice.latitude);  // toRadians below
var dLon = toRadians(lon2-dublinOffice.longitude);
var a =
Math.sin(dLat/2) * Math.sin(dLat/2) +
Math.cos(toRadians(dublinOffice.latitude)) * Math.cos(toRadians(lat2)) *
Math.sin(dLon/2) * Math.sin(dLon/2);
var c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1-a));
var d = R * c; // Distance in km
return d;
}

function toRadians(deg) {
return deg * (Math.PI/180);
}
function processClients(contacts) {
// saving clients living in a 100km radius
var distanceIncludedClientList =
contacts.map(client => {
return {
latitude: client.latitude,
longitude: client.longitude,
user_id: client.user_id,
name: client.name,
distance: getDistanceFromLatLonInKm(client.latitude, client.longitude)
};
});

var closeByClients = distanceIncludedClientList.
filter(client => client.distance <= 100);
function compareUserID(a,b) {
if(a.user_id > b.user_id)
return 1;
if(a.user_id < b.user_id)
return -1;
return 0;
}
var closeByClientsSorted = closeByClients.sort(compareUserID);

report(closeByClientsSorted);
}
function report(contacts) {
console.log('Clients who live max 100 Kms from our office: n');
contacts.map(contact => {
console.log(` id: \${contact.user_id}, Name: \${contact.name}, Distance: \${contact.distance.toFixed(1)}Km`);
});
}

function init() {
var textFileUrl = 'https://gist.githubusercontent.com/brianw/19896c50afa89ad4dec3/raw/6c11047887a03483c50017c1d451667fd62a53ca/gistfile1.txt';

request.get(textFileUrl, function (error, response, body){
if (!error && response.statusCode === 200) {
contacts = body.split('n');

contacts = contacts.map(function(elm){
elm = JSON.parse(elm);
return elm;
});
}
processClients(contacts);
});

}
return {
init: init,
toRadians: toRadians,
processClients: processClients,
getDistanceFromLatLonInKm: getDistanceFromLatLonInKm
};
})();

ClientsNearby.init();
module.exports = ClientsNearby;
``````

Solution

I’ll just list things as I read them, so don’t expect any sort of meaningful order:

• Prefer `let` over `var``let` provides variable declaration in block scope, rather than functional scope. It also prevents hoisting. It solves a few language gotchas that `var` has. Please note that currently, you can only use `let` in contexts under strict mode (which you already have, good for you!)
• Your `getDistanceFromLatLonInKm()` function has many single letter variables, which makes it a bit hard to follow. What’s `R`? Earth’s radius in km? Why not `EarthRadius`?
• If you only need to return a single expression from an arrow function, you don’t need the full form.

``````(arg1, arg2) => { return something }
``````

can be shortened to

``````(arg1, arg2) => something
``````

in case `something` is an object literal, you can wrap it with braces like so:

``````(arg1, arg2) => ({something: here})
``````
• Your `compareUserID()` function can be shortened to

``````var compareUserID = (a, b) => a.user_id - b.user_id;
``````
• Think twice before defining a function within another function, the inner function will be redefined for every function call.

• The `[].map()` function is meant for mapping one array to another, not for logging. If you want to just iterate the array and do something without returning anything, use `[].forEach()`, also, you don’t need to full form of the arrow function for `console.log`:

``````contacts.forEach(contact => console.log(`...`));
``````
• Node supports object literal shorthands, if the key and the variable name you’re defining have the same name, you can do it like this:

``````return {init, toRadians, processClients, getDistanceFromLatLonInKm};
``````
• It would be likely better if the `init()` function accepted the URL to the file to be processed. That way, if you ever change URLs (which is more than likely, because raw GitHub urls stay relevant for so long), you can leave your module’s code untouched.

• Node’s modules are implicitly wrapped by IIFEs anyway, so your IIFE is redundant. Remove `var ClientsNearby = (function(){` and `}();` to save yourself a level of indentation across the script. Just assign your object literal with functions to `module.export` directly.

• It’s considered good practice to have as few side effects as possible in your modules (such as producing output). Think about how you’d want to use a module. Personally, I’d like to be able to invoke a function and get an array (or a collection) of data, for me to do what I please. Tomorrow you wouldn’t want to output the clients, you’d want to log them into a file, or push them into a database.