matiu / dolar-blue

Client for the Dolar blue API in nodejs
24 stars 16 forks source link

Modularize data sources and always use most recent result #10

Closed andrewfhart closed 9 years ago

andrewfhart commented 9 years ago

@matiu This is an awesome library you have developed, and thank you in particular for taking the time to write a test suite! That has been hugely helpful to me and taught me about nock which is an amazing tool.

I am interested in adding to the list of sources that this API can query. However, extending the current nested callback structure that dictates the order in which data sources get called will eventually make the code difficult to read and maintain.

A possible alternative, implemented in this PR, is to use the recency (freshness) of the results from each data source as the indicator of which result should be "accepted." In other words, instead of querying each source in a sequence until one succeeds, this approach queries all sources in parallel (in an arbitrary order) and responds with the one that reported the most recent update date/time. So, if Bluelytics has a more recent update timestamp than LaNacion, the API will respond with Bluelytics' data, even if there was no problem accessing LaNacion, because Bluelytics is more "fresh."

This approach is worth taking only if you "trust" each source equally. Perhaps you feel strongly that LaNacion is the most reliable and that other sources should only be used if LaNacion is not available. If that is the case, then this approach is not optimal. However, if the sources can be considered interchangeable, then this approach is arguably the best, since it guarantees the most recent results, all other things being equal.

One other very nice characteristic of the parallel approach is that it allows the logic specific to querying and parsing each data source to reside in its own module (see for example ./lib/sources/LaNacion.js). In this approach, rather than being constrained to a nested callback structure, a simple for loop can execute each package's getData function in parallel:

// Process each requested source
_(sources).forEach( function (source) { 
  try {
    var sourceModule = require('./sources/' + source);  // Bluelytics, LaNacion, etc.
    sourceModule.getData(handleResponse);
  } catch (err) {
    cb("Unknown source provided: " + source);
  }
});

... and use a simple manager function (handleResponse in this case) to update a counter that keeps track of responses received, returning the best result once all have completed. For example, here is the handleResponse function from the previous snippet (see ./lib/dolar-blue.js for context):

// Function to handle response from sources
var handleResponse = function (err, sourceData) {

  // Store/replace data if sourceData exists and is the most recent data available so far
  if (sourceData && sourceData.rates && moment(sourceData.rates.date).isAfter(bestDate)) {
    data = sourceData;
    data[sourceData.source.name] = sourceData.data; // Backwards compatibility with old API
    bestDate = sourceData.rates.date;
  }

  // Mark this request responded.
  responseCount++;

  // If we've heard back from all requests, invoke the callback
  if (responseCount == sources.length) {
    (data == null) 
      ? cb('no data') 
      : cb(null, data);
  }
};

I think that this structure will make it considerably easier to add new data sources in the future.

I have extended the test suite (there are now 18) and used it to verify that this change is backwards compatible:

screen shot 2014-10-24 at 4 32 58 pm

Finally, I made some updates to the README file to clarify what happens behind the scenes with this approach and explicitly declare the list of data sources that are currently supported.

I would love to hear your thoughts on this and am happy to make any changes you suggest. In case it is a little difficult to get the context from this diff, my forked repository is at https://github.com/andrewfhart/dolar-blue. Thank you again for creating this very useful library!

matiu commented 9 years ago

Love the change! Fantastic work! Could you rebase it so we can merge it.

Ping @gabegattis

andrewfhart commented 9 years ago

woot! done.