1
\$\begingroup\$

I have many json files that I want to serve through an API. The json files look something like this:

...
170102.json
170103.json
170104.json
170108.json
170109.json
170120.json
...

The name of each file is the date in the format 'yymmdd.json'. There are a lot of files missing, so if a user ask for a file that is not present, the data from the most recent previous file must be served. For example, with the above files, if someone ask for the data from date 2017-01-07, he should get the data from file 170104.json. But if the gap between files is greater than five, then no data should be provided. With the previous files if someone request for the data from date 2017-01-20, then a 404 error should be return.

I have this controller which does the job, but I'm concern that it is using the existSync function and therefore blocking the node process (the end point is localhost:3000/api/:year/:month/:day):

import fs from 'fs';
import path from 'path';

const getData = (req, res) => {
  const {
    year,
    month,
    day,
  } = req.params;
  const date = new Date(`${year}/${month}/${day}`);
  let fileDate = date.toISOString().substring(2, 10).replace(/-/g, '');
  let jsonFile = path.join(__dirname, 'data', `${fileDate}.json`);

  for (let i = 1; i <= 5; i += 1) {
    if (fs.existsSync(jsonFile)) {
      break;
    } else {
      date.setDate(date.getDate() - 1);
      fileDate = date.toISOString().substring(2, 10).replace(/-/g, '');
      jsonFile = path.join(__dirname, 'data', `${fileDate}.json`);
    }
  }

  fs.readFile(jsonFile, 'UTF-8', (err, data) => {
    if (err) {
      res.status(404).send('Not found');
    } else {
      res.json(JSON.parse(data));
    }
  });
};

export default getData;

// this is routes.js

import getData from '../controllers/apiController';

const routes = (app) => {
  app.route('/api/:year/:month/:day')
    .get(getData);
};

export default routes;

How can I improve this to be non blocking, or improve it's quality or performance overall? I'm learning nodejs and express and trying to grasp all of its best features, but having a hard time getting my head around the async functions, so any advice to improve this in that direction is very welcome.

\$\endgroup\$
1
\$\begingroup\$

I would consider asynchronously checking for all 5 potential files at once, meaning that checks can be paralellized. Once all are complete, you can evaluate the compiled results and return the most recent file.

Depending on your expected call pattern ( for example, are more recent files typically the ones being requested?), you may consider keeping a cached copy of all or part of the file listings in memory to avoid disk read altogether.

Another thing you may really want to think about is your API interface. I would find it extremely hard to understand that a request for a file that may not actually exist would return a 200 result. This would be very non-standard from a REST API standpoint. Perhaps the client calling this service should own the logic of checking for 5 days’ worth of results.

Why all the weird formatting steps from Date object? Just output directly to your desired format instead of chaining together a bunch of string manipulation.

If file is already JSON format, why parse it and then reserialize when delivering response? Had you consider serving these files statically? You could GREATLY simplify this code (down to literally two or three lines by serving these files statically and letting caller own the logic of looking for days in the past. Not to mention. You can get all the serve static behaviors around cache-control, etags, etc. this way if cacheability matters to your client.

\$\endgroup\$
  • \$\begingroup\$ Trying to slowly digest all those insights. The 200 response on a file that is not there is because although there is no file for that date, the data valid for that day is the one provided from the previous accessible file (unless the gap is bigger than five days). I though on serving static files but the caller has no idea when there is going to be valid date for a date and it would be wrong to provide a 404 if there is no file because there is actually data for many date gaps. The caller should not care if there is a file or not, but if there is data or not. \$\endgroup\$ – Mauricio Robayo Feb 14 '18 at 13:57
  • 1
    \$\begingroup\$ Why is it wrong to provide a 404 to the client? A 404 is a legitimate, non-error response code. The real is question is where business logic should reside. Are you forcing this logic onto the server because this is a client requirement? What if the client suddenly needs a 10 day lookup period? Do you want to have to make changes in the server to accommodate this business logic? Alternately, if the the client truly is a dumb client and just wants a file and doesn't care about the business logic, then perhaps you need this data in a data store queryable by date range. \$\endgroup\$ – Mike Brant Feb 15 '18 at 23:10
  • \$\begingroup\$ Mike, thanks for your time and insights. I've been thinking in your suggested approach and I think is the way to go: serving static files and let the client handle the logic. I was overcomplicating myself. This is really a learning project that I have created for myself and is very valuable for me to have the feedback from someone with experience and knowledge in this field. \$\endgroup\$ – Mauricio Robayo Feb 16 '18 at 2:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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