What is not right in this code? Post your solution.

#challenge #node #javascript

const fs = require("fs");
const { promisify } = require("util");

const cache = new Map();

cache.set("file1", "data of file 1...");
cache.set("file2", "data of file 2...");

const readFilePromise = promisify(fs.readFile);

// What is wrong in this function?

const getFileData = (fileName, callback) => {
    if (cache.has(fileName)) return callback(null, cache.get(fileName));
    return readFilePromise(fileName)
        .then(data => {
            cache.set(fileName, data);
            return callback(null, data);
        }).catch(err => callback(err));
};

Top comments (4)

J. Pichardo • May 25 '18

Well besides this line ...


cache.set("file2, 'data of file 2...");

// Should be (Note: quotes)

cache.set("file2", "data of file 2...");

This are the changes I'd make

Use object instead of Map (Easier to have immutable structures)
Wrap cache in a Promise.resolve in order to reuse code and be able to use a ternary operator
For consistency DO NOT use a callback syntax for a promise based function

So, the final script would be:


const cache = {
  ["file1"]: "data of file 1...",
  ["file2"]: "data of file 2..."
};

const getFile = fileName => {

  // Wrap the cache in a promise in order to do chaining;
  const cachePromise =
    cache[filename] != null
      ? Promise.resolve(cache)
      : readFilePromise(fileName).then(
          content => (cache = { ...cache, [fileName]: content })
        );

  // Return a promise with the data
  return cachePromise.then(cache => cache[fileName]);
};

// DO NOT pass a callback.
getFile("file1")
  .then(data => {
    // Do Something
  })
  .catch(err => {
    // Do Something else
  });

Ben Sinclair • May 25 '18

Well it took me > 10 second to twig that "cb" meant "callBack" so there's that: use explicit variable names. Same with err - is it a message or a code or an object or what?

Other than that I have no idea what the braces around { promisify } are for because I don't do this language and am not sure what to google for to find out. I don't know any of these libraries so don't know what to expect.

Having said that, shots in the dark from an outside developer:

there's nothing that sets an expiry on the cached data - if I assume cache.has returns true even if the data's expired, and that you are expected to check some expiry time property, then it will never get updated.
fs.readFile might process a condition which is transiently invalid without throwing an exception, in which case it'll be stored for eternity.
cache.get(..) might return an object containing metadata alongside the actual payload, such as expiry time