What is not right in this code? Post your solution.
Publish Date: May 25 '18
20 4
constfs=require("fs");const{promisify}=require("util");constcache=newMap();cache.set("file1","data of file 1...");cache.set("file2","data of file 2...");constreadFilePromise=promisify(fs.readFile);// What is wrong in this function?constgetFileData=(fileName,callback)=>{if(cache.has(fileName))returncallback(null,cache.get(fileName));returnreadFilePromise(fileName).then(data=>{cache.set(fileName,data);returncallback(null,data);}).catch(err=>callback(err));};
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
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:
constcache={["file1"]:"data of file 1...",["file2"]:"data of file 2..."};constgetFile=fileName=>{// Wrap the cache in a promise in order to do chaining;constcachePromise=cache[filename]!=null?Promise.resolve(cache):readFilePromise(fileName).then(content=>(cache={...cache,[fileName]:content}));// Return a promise with the datareturncachePromise.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:
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