Finding clients in a 100 km circle

Question 1

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;

Question 2

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.

Leave a Reply Cancel reply