3
\$\begingroup\$

I have a bunch of functions that return promises and need to chain them together. However, it's currently difficult to read and I would expect it to be even harder to maintain.

I have a function that performs the following actions:

  1. Logs off any inactive users.
  2. Verifies a user's credentials.
  3. Creates a session and logs the user in.

Is there a better way?

generateToken: function (email, password) {
    return new Promise((resolve, reject) => {
        //Kill inactive sessions first
        module.exports.destroyInactive()
        .then(() => {
            //Verify that the password matches the user
            users.checkPassword(email, password, sharedSecret)
            .then((user) => {
                //Check if there is another user already signed in, if there is then we can't
                //produce another token until they are signed out.
                signedon.getByEmailAddressThisProcess(email)
                .then((result) => {
                    let rows = result[0];

                    if (rows.length > 0)
                        return reject('A user with email: ' + email + ' is already logged in.');

                    //Get the current XPROCESS.
                    xprocess.getByProcessId(process.pid)
                    .then((xproc) => {
                        //Create a brand new session object
                        let session = generateSession(user);

                        //Save the session in the signed on table.
                        signedon.signin(session, xproc.pid)
                        .then(() => {
                            resolve(parseSessionToToken(session));
                        },
                        reject);
                    },
                    reject);
                },
                reject);
            },
            reject);
        },
        reject);
    });
},
\$\endgroup\$
4
  • \$\begingroup\$ Why is the client doing all those checks? The client should just try to log in with a single call and get a "forbidden" or "granted" kind of response, the server should take care of all the rest. \$\endgroup\$
    – ChatterOne
    Jan 31, 2017 at 9:13
  • \$\begingroup\$ @ChatterOne This code is on the server. The client makes a request to a login API endpoint and is returned either a token or an authentication error. \$\endgroup\$
    – Mike Eason
    Jan 31, 2017 at 9:14
  • \$\begingroup\$ Ok, you should probably add a tag to your question then, is it node.js ? \$\endgroup\$
    – ChatterOne
    Jan 31, 2017 at 9:33
  • \$\begingroup\$ @ChatterOne Ok yes, I've just re-tagged it. \$\endgroup\$
    – Mike Eason
    Jan 31, 2017 at 9:37

1 Answer 1

4
\$\begingroup\$

Potential improvements

generateToken(email, password) {
  let user, session;

  return module.exports.destroyInactive()
    .then(() => users.checkPassword(email, password, sharedSecret))
    .then(_user => user = _user)
    .then(() => signedon.getByEmailAddressThisProcess(email))
    .then(([rows]) => {
      if (rows.length > 0) {
        return Promise.reject(`A user with email: ${email} is already logged in`);
      }

      return xprocess.getByProcessId(process.pid);
    }).then(xproc => {
      session = generateSession(user);

      return signedon.signin(session, xproc.pid);
    }).then(() => parseSessionToToken(session));
}

Your method falls into the nested promises antipattern, very similar to the classic callback pyramid of doom.

This solution relies on chaining promises. Since we need the variables user and session in non-adjacent promises we need to have references of them outside of the promise chain.

Since you are already using Promises which are an ES6 feature, you can take advantage of extra ES6 goodness. See es6-features.org for more.

ES6 features

Enhanced object method properties

generateToken: function (email, password) can be simplified by using to generateToken(email, password)

Arrow Function Expression Bodies

users.checkPassword(...) is implicitly returned. Since this is a Promise, the chain won't continue to be executed until the returned Promise is resolved/rejected. This means we can just return the promise and not have to continue to nest using checkPassword(...).then(...)

Array Matching

let rows = result[0] can be simplified to rows = [result], which can then be used as the resolve parameter (([rows]) => ...)

Promise.reject

Returning Promise.reject allows us to reject the Promise without nesting.

Template Literals/String Interpolation

Provide a (subjectively) more readable solution by not having to concatenate strings with variables.

'A user with email: ' + email + ' is already logged in.'

becomes

`A user with email: ${email} is already logged in`

Arrow Function Statement Bodies

Since we are doing multiple things inside the rows Promise, we use => statement bodies.

Due to this we need to explicitly return the Promise (so that the chain doesn't continue to execute until the Promise is resolved/rejected). Otherwise undefined will be resolved, not waiting for the Promise.

Note:

Nowhere are you catching rejected promises using catch, are you catching a rejected generateToken attempt?

Promises can greatly improve readability and remove nested callbacks when used correctly, but it is important to not allow them to "swallow" errors. At the very least make sure to log rejected Promises. A simple somePromise(...).catch(console.error) is better than nothing. Prefer console.error over console.log when possible since it's more "noisy".

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.