1
\$\begingroup\$
var argon2 = require('argon2')
var nano = require('nano')('http://127.0.0.1:5984')

var users = nano.db.use('users')

var private = {
  generateHash: (password, callback) => {
    argon2.generateSalt().then((salt) => {
      argon2.hash(password, salt).then((hash) => {
        callback(hash)
      })
    })
  }
}

var public = {
  register: (username, password, callback) => {
    var calledback = false
    users.list((error, body) => {
      if (error) {
        calledback = true
        callback('dbError')
      } else {
        body.rows.forEach((value) => {
          if (value.id === username) {
            calledback = true
            callback('userExists')
          }
        })
      }
      if (!calledback) {
        private.generateHash(password, (hash) => {
          users.insert({
            _id: username,
            hash: hash
          }, (error) => {
            if (error) {
              calledback = true
              callback('dbError')
            } else {
              calledback = true
              callback(false)
            }
          })
        })
      }
    })
  }
}

module.exports = public

I made a function for registrating a user in a CouchDB database. How could I make the code more readable?

\$\endgroup\$
4
\$\begingroup\$

Use const and let

You're using ES6, so use these features. These are better than var as they communicate their intent better and are not hoisted.

Don't mix promises and callbacks

This one I can't really understand: You use a promise in one method (generateHash) but then try to use callbacks afterwards. Promises compose much better than callbacks for a little extra overhead and are much more readable (and would also make a huge improvement to your code). If you still want to use callbacks, that's completely fine, but try to stick to one - don't mix.

I've chosen to use Promises in the code I'm giving back to you, so I've wrapped your users.list function in a Promise. If you'd prefer to use callbacks, that's perfectly fine, but you may want to use async or a similar library to prevent the hacks you made with callbacked = false etc. That sort of thing is very brittle and does not scale very well at all, and makes it hard to read.

userExists return value

This makes things stringly typed. I've left it in as I'm not sure what you would prefer to use, but may I suggest using false to indicate register was unsuccessful (but did not error), such as when the user already exists, or return a Promise.reject(error) when there is actually an error. On the note of returning false, I'm not sure why you return false when registration was a success, but I'd advise against it.

Promises chain

You can return promises from inside of a then "callback" and that value will be resolved and then passed to the next promise in the chain. This means you can simplify your generateHash method to not have so much cyclomatic complexity. This is further improved by not using callbacks.

Don't split your functions into private and public

There's no need to attach these functions to these objects, and they have no bearing on what is actually exported or imported. To simplify things, you could let generateHash be a free function and then just export an object with register in.

Use function statements where possible

Unless you transpile with Babel, using function expressions will cause those functions to lose their names. This is fine for adhoc functions like arrow functions passed to map, but for library functions it may be more helpful to have those names in a stack trace.

Note that newer engines automatically assign the binding name to the function, but that can't be relied upon.

Reviewed code

const argon2 = require('argon2')
const nano = require('nano')('http://127.0.0.1:5984')

const users = nano.db.use('users')

function generateHash(password, callback) {
  return argon2.generateSalt()
    .then((salt) => argon2.hash(password, salt))
    .then(callback)
}

// Reviewers note: this wraps users.list in a promise. You
// may want to consider using Bluebird for this instead.
function listUsers() {
  return new Promise((resolve, reject) => {
    users.list((error, body) {
      if (error) {
        return reject(error)
      }

      return resolve(body)
    }
  })
}

function insertUser(userObject) {
  return new Promise((resolve, reject) => {
    users.insert(userObject, (error) => {
      if (error) {
        return reject(error)
      }

      // Reviewers note: I am not sure why you return false on success?
      return resolve(false)
    })
  })
}

module.exports = {
  register(username, password) {
    return listUsers()
      .then(body => body.rows.some(value => value.id === username))
      .then(doesUserExist => {
        if (doesUserExist) {
          return 'userExists'
        }

        return generateHash(password)
          .then(hash => insertUser({ _id: username, hash })
      })
  }
}
\$\endgroup\$

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.