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

Manuel Romero @mrm8488

Location:
Murcia (Spain)
Joined:
Mar 29, 2017

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

Publish Date: May 25 '18
20 4
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));
};
Enter fullscreen mode Exit fullscreen mode

Comments 4 total

  • Ben Sinclair
    Ben SinclairMay 25, 2018

    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
  • J. Pichardo
    J. PichardoMay 25, 2018

    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
      });
    
    
    
Add comment