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

twitter logo github logo ・1 min read

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));
};
twitter logo DISCUSS (4)
markdown guide
 

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

  1. Use object instead of Map (Easier to have immutable structures)
  2. Wrap cache in a Promise.resolve in order to reuse code and be able to use a ternary operator
  3. 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
  });


 

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
 

To address your intrigue about the braces around promisify, this article may shed some light:

 

Oh that's cool. Just tried it in my node REPL and found whatever version of node you get with MacOS is too old (no surprises there though)

Classic DEV Post from Sep 11 '18

I'm having some "not this again" feelings with Parcel, how should I be feeling about this tool?

Manuel Romero profile image

Sore eyes?

dev.to now has dark mode.

Go to the "misc" section of your settings and select night theme ❤️