2
\$\begingroup\$

I'm creating my first real life Nodejs project. I've created some files and all work, but I'm not satisfied about what my files look likes. It's unreadeble.

For example, I work of a file to manage JWT sessions and RSA keys.

For this file, I need two function to load and save the RSA private key.

This Node.js style guide says to put require at the top of documents.

'use strict';

const logger = require('./logger')(module.filename);
const NodeRSA = require('node-rsa');
const jwt = require('jsonwebtoken');
const fs = require('fs');

const loadKeyFromDisk = (path) => new Promise((resolve, reject) => fs.readFile(path, 'utf-8', (error, data) => {
  if (error) {
    reject(error);
    return;
  }
  resolve(data);
}));
const saveKeyToDisk = (path, key) => new Promise((resolve, reject) => fs.writeFile(path, key, 'utf-8', (error) => {
  if (error) {
    reject(error);
    return;
  }
  resolve();
}));

[...]

modules.exports = {...};

But, I also learn to put variables into small context as possible

'use strict';

const logger = require('./logger')(module.filename);
const NodeRSA = require('node-rsa');
const jwt = require('jsonwebtoken');

const { loadKeyFromDisk, saveKeyToDisk } = (() => {
  const fs = require('fs');

  return {
    loadKeyFromDisk: (path) => new Promise((resolve, reject) => fs.readFile(path, 'utf-8', (error, data) => {
      if (error) {
        reject(error);
        return;
      }
      resolve(data);
    })),
    saveKeyToDisk: (path, key) => new Promise((resolve, reject) => fs.writeFile(path, key, 'utf-8', (error) => {
      if (error) {
        reject(error);
        return;
      }
      resolve();
    }))
  };
})();

[...]

modules.exports = {...};

I've also tried some others style, but I think it's don't look like professionnal and clean code...

'use strict';

const logger = require('./logger')(module.filename);
const NodeRSA = require('node-rsa');
const jwt = require('jsonwebtoken');
const fs = require('fs');

const loadKeyFromDisk = (path) => {
  return new Promise((resolve, reject) => {
    fs.readFile(path, 'utf-8', (error, data) => {
      if (error) {
        reject(error);
        return;
      }
      resolve(data);
    });
  });
};
const saveKeyToDisk = (path, key) => {
  return new Promise((resolve, reject) => {
    fs.writeFile(path, key, 'utf-8', (error) => {
      if (error) {
        reject(error);
        return;
      }
      resolve();
    });
  });
};

[...]

modules.exports = {...};

Did you have somes advices or idea to improve my coding style?

My complete code Here : https://pastebin.com/kXvxNd2k

\$\endgroup\$
  • \$\begingroup\$ Thanks 200_success but I don't agree with the new title. The code work and I don't need help to correct It. Question is only about coding style. \$\endgroup\$ – NewbieDev Jul 6 at 20:59
1
\$\begingroup\$

require style

I (and most of the Node developers I've worked with) prefer the style where require is put on top. As stated in the guide you linked, it gives a clearer overview of what are dependencies of the module/file you're writing. No one likes to see that if some condition is successful, then a new module will be loaded. It's just unnatural.

On the other hand, you shouldn't look at const foo = require('bar') as just another variable assignment and treat it as such when you're writing Node or any other JS code. Calling require function will load a file (if not previously loaded, read more here) and make its exported object available through the variable in the file you're writing. This same thing in some other language like C/C++ would be expressed as #include <foo>, which exposes the whole file to another file.

Other things I've noted

Usage of fs module

I see that you have helper functions that wrap fs.readFile and fs.writeFile in promises. I'm not sure if you are aware of 2 things:

  1. There is a promisify method which can do this for you.
  2. There already exist promisifed versions of this functions.

My suggestion is to use fs.readFileSync and fs.writeFileSync instead of wrapping them yourself.

Line 55

I'm not sure what was your intention here, but you've just called a function which returns a promise without waiting for the promise to be resolved. Since you're writing to the disk, this operation might not finish once a function from your returned object gets called. I'd just add await here.

Line 73

First, add a space between the ) and ? to be consistent with the style you have for : and stuff around it. Second, break that long condition into either a function, or to a several variables. I'd personally go here with functions:

const notInvalidated = (invalidations, payload) => !invalidations[payload.userUuid] || payload.invalidationCounter === invalidations[payload.userUuid]
const hasValidIat = (invalidBefore, payload) => payload.iat >= invalidBefore

And then your condition could be written as:

notInvalidated(invalidationByUserUUID, payload) && hasValidIat(invalidBefore, payload) ? payload : false

Still long, but more readable.

module.exports mess

My personal preference is to have module.exports either export a function or an object, so make it as short as possible. But from your code on pastebin, it looks like your core logic is put there. For me, this is unreadable and I would look how to refactor this into something which will return either a single function or just a simple object. In your case, I'd first move that function to a new named function, something like this:

const myJwt = async (keyPath) => { /* the rest */ }

module.exports = myJwt

From there, I'd move the functions in the object you are returning to the file level:

const generateNewKeyPair = async () => { /* ... */ }
/* also for sign, verify and invalidateOldUserToken */

const myJwt = async (keyPaht) => {
  /* whatever goes here */
  return {
    generateNewKeyPair,
    sign,
    verify,
    invalidateOldUserToken,
  }
}

module.exports = myJwt

This refactoring will be a bit tricky, because you have these two variables invalidationByUserUUID and invalidBefore which should be shared with the functions that I suggested extracting. You should maybe consider to wrap this in a class and return the class instead of a function. Either that, or you'll have to pass invalidationByUserUUID and invalidBefore with every function call.


Hope my comments make sense. I didn't do a whole code refactoring since I'm not 100% sure what you wanted to achieve, but if needed, I can give it a try.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your help and comments. That's make sense for me :D I've move the init code and I temporary make this bin : pastebin.com/9N6Dtgib . It's more easy to understand. I will think about class, but I don't see what it will change... \$\endgroup\$ – NewbieDev Jul 6 at 20:29
  • \$\begingroup\$ I finally redid the code. With a class ( pastebin.com/fa2RnbkJ ), but I don't like the way I have to manage private. And without a real class (pastebin.com/5671MgYC ). \$\endgroup\$ – NewbieDev Jul 7 at 8:15
  • \$\begingroup\$ Yes, private is tricky in JS... At least for now... Anyhow, I'm glad that you find this easier to understand. I can also take a look later today or tomorrow at one of the links you've posted and edit my response to address the code there (or you can create a new question for that). I've already noticed some minor style issues, like not using curly brackets for one line else. \$\endgroup\$ – Pritilender Jul 7 at 10:57

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.