0
\$\begingroup\$

This class reads a log file stored on my server, parses it, and returns it. It also caches the data so that subsequent calls to get the analytics data will not have to read from the file. The analytics files will be relatively small, so memory usage is not a concern.

The callback passed to getAnalytics() is expected to accept an error as its first parameter and data as its second parameter.

Suggestions on improvements and cleanups are welcome.

Link to file on GitHub

const fs = require('fs');
const geoip = require('geoip-native');

/**
 * Constructor for an Analytics object.
 * @constructor
 * @param {string} analyticsFile The path to the server analytics log
 */
function Analytics(analyticsFile) {
  this.analyticsFile = analyticsFile;
  this.cache = {};
}

/**
 * Milliseconds in an hour.
 * @const
 * @type {number}
 */
Analytics.CACHE_KEEP_TIME = 3600000;

/**
 * Factory method for an Analytics object.
 * @param {string} analyticsFile The path to the server analytics log
 * @return {Analytics}
 */
Analytics.create = function(analyticsFile) {
  return new Analytics(analyticsFile);
};

/**
 * Fetches analytics on recent site traffic and passes it to a callback.
 * Caches the results for a day.
 * @param {function()} callback The callback function to which the analytics
 *   are passed.
 * @return {?}
 */
Analytics.prototype.getAnalytics = function(callback) {
  /**
   * First check if we have analytics cached. If not, then we should fetch it
   * again.
   */
  var currentTime = (new Date()).getTime();
  if (this.cache.analytics && currentTime < this.cache.expires) {
    return callback(null, this.cache.analytics);
  }
  var context = this;
  fs.readFile(this.analyticsFile, 'utf-8', (error, data) => {
    if (error) {
      return callback(error);
    }
    try {
      data = data.trim().split('\n').map(function(entry) {
        entry = JSON.parse(entry);
        entry.country = geoip.lookup(entry.ip).name;
        return entry;
      });
      context.cache.analytics = data;
      context.cache.expires = currentTime + Analytics.CACHE_KEEP_TIME;
      return callback(null, data);
    } catch (error) {
      return callback(error);
    }
  });
};

/**
 * This line is needed on the server side since this is loaded as a module
 * into the node server.
 */
module.exports = Analytics;
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

First of all, why restrict to ES5? Unless you're running/targetting Node < 4 (which is a security hazard, a story for another time), just write using the syntax available to the Node version you're running. Restricting your syntax is unnecessary overhead.

Now you don't need to go OOP on this one. I find that once people resort to OOP, the code usually ends up very boilerplate-y, contains lots of indirection, you worry about what this is. You can simply have a global cache keyed by file name and holds the data and expiry. Keep it simple.

data = data.trim().split('\n').map(function(entry) {
  entry = JSON.parse(entry);
  entry.country = geoip.lookup(entry.ip).name;
  return entry;
});

Why are you storing multiple JSON data inside one file separated by newline? The split by \n can be brittle. If it happens that your writer newlines the data in the middle, this will blow up. If you want to keep this data together, consider writing them in a stringified array instead. Stringified arrays are perfectly valid JSON.

Use promises. Callbacks are so 2 years ago, and are very clunky. There's a module called fs-extra that promisifies fs operations.

And Date.now() is available since ES5 to get the current millisecond timestamp.

const fs = require('fs-extra')
const geoip = require('geoip-native')
const cacheKeepTime = 3600000
const cache = {}

exports.getAnalytics = function(path){
  const currentTime = Date.now()
  const cacheHit = cache[path] && cache[path].data && cache[path].expiry < currentTime

  return cacheHit ? Promise.resolve(cache[path].data) : fs.readFile(path, 'utf8').then(contents => {
    const data = JSON.parse(contents) // Error in a callback rejects the promise
    const expiry = currentTime  + cacheKeepTime

    // Update cache
    cache[path] = { data, expiry }

    return data
  })
}

// Usage
const analytics = require('./analytics')
const someAnalyticsFile = 'path/to/analytics.json'

analytics.getAnalytics(someAnalyticsFile).then(data => {
  // We got data
}, error => {
  // Something blew up
})
\$\endgroup\$
2
  • \$\begingroup\$ Thanks very much for your review. I"m guaranteed that the data can be separated safely by newline. None of the fields will have a newline. Date.now() is very useful, I didn't know about it. I think the primary reason I did this OOP style was because I wanted to save the name of the analytics file in the object. ¯\_(ツ)_/¯ \$\endgroup\$
    – omgimanerd
    Jul 19, 2017 at 4:14
  • \$\begingroup\$ @omgimanerd I just find the constructor/class pattern clunkier than its factory counterpart. You can do the same with a factory returning an object and storing the name in a closure. \$\endgroup\$
    – Joseph
    Jul 19, 2017 at 16:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.