5
\$\begingroup\$

I'm new to Node.js (and also backend) for only a week, and now I'm trying to do a simple registration. I found that it's really hard to reduce complexity because pretty much every callback is error-first. Is there any way to reduce this? I found it seems impossible to avoid all of those if-elses.

const User = require('../../models/user')
const ERROR_MESSAGES = require('../../models/constant').ERROR_MESSAGES
const RESPONSE_STATUS = require('../../models/constant').RESPONSE_STATUS
const helper = require('../../helpers/helper')
const validator = require('validator')

module.exports.register = function(req, res) {
    const email = req.body.email
    const password = req.body.password
    const errors = []

    // input validation
    if (!email || email.trim().length == 0) {
        errors.push(ERROR_MESSAGES.EMPTY.EMAIL)
    } else if (!validator.isEmail(email)) {
        errors.push(ERROR_MESSAGES.INVALID.EMAIL)
    }

    if (!password) {
        errors.push(ERROR_MESSAGES.EMPTY.PASSWORD)
    }

    if (errors.length > 0) {
        return res.json(helper.generateResponse(errors, null, RESPONSE_STATUS.REQUEST_DENIED))
    } else {
        User.findOne({email: email}, function(err, user){
            if (err) { 
                res.json(helper.generateResponse([ERROR_MESSAGES.UNKNOW], null, RESPONSE_STATUS.REQUEST_DENIED)) 
            } else if (user) {
                res.json(helper.generateResponse([ERROR_MESSAGES.DUPLICATE.EMAIL], null, RESPONSE_STATUS.REQUEST_DENIED))
            } else {
                var user = new User()
                user.email = email
                user.password = user.generateHash(password)

                var errorValidation = user.validateSync();

                if (errorValidation) {
                    // input validation
                    errors = helper.getMongooseValidationErrors(errorValidation.errors)
                    res.json(helper.generateResponse(errors, null, RESPONSE_STATUS.REQUEST_DENIED))
                } else {
                    user.save(function(error, newUser) {
                        if (error) { 
                            res.json(helper.generateResponse([error], null, RESPONSE_STATUS.REQUEST_DENIED))
                        } else {
                            res.json(helper.generateResponse(errors, newUser, RESPONSE_STATUS.SUCCESS))
                        }
                    })
                }
            }
        })
    }
}
\$\endgroup\$
  • \$\begingroup\$ Your code could certainly be tidied up, by how much depends on what version of Node your using? \$\endgroup\$ – James Aug 25 '17 at 10:24
  • \$\begingroup\$ your code is not complex in my opinion, it is very clear what you're trying to do. However one method you can use to make it more clear is to divide your code piece by piece into methods or functions. It also helps avoid duplicate code. \$\endgroup\$ – Amin Jafari Aug 26 '17 at 10:02
  • \$\begingroup\$ @James I'm using v6.11.0. \$\endgroup\$ – Rookie Aug 26 '17 at 13:56
  • \$\begingroup\$ @AminJafari I tried to do so, but it turns out that I need to write heaps of callback functions to achieve what I wanted to do. \$\endgroup\$ – Rookie Aug 26 '17 at 13:57
  • \$\begingroup\$ @AminJafari I disagree, this is classic callback hell \$\endgroup\$ – James Aug 26 '17 at 15:09
2
\$\begingroup\$

Well, the code is fairly simple, but you've made it complex due to a bad design. I would encourage the following:

  • Use Promises. Mongoose implements promises and so does express, so most of the times you can avoid using callbacks. Also implement your own helpers with promises. Further more you can read up on Async/Await too.

  • Read up on ES6. Use Arrow-functions, const/let variables, etc. It's not just syntactic sugar, but also more consistent overall.

  • Destructure package variables and arrays. It shortens your code and makes it easier to read.

  • Use functions. Functions are your friends. Get used to breaking down problems into small pieces and code into small functions. As a rule of thumb no function should have more that two indentations. If you need to nest many if-else clauses, then function them.

  • Use semicolons. Even though JS accepts code without semicolons, it's a good practice to use them because it makes the code more readable and less error-prone.

Now, getting back to your code... I made a small refactorization so you can see how it should look, but it needs a complete refactor of architecture as well as code because imo there are many anti-patterns and wrong responsibility assignation, i.e. validations should be frontend, the constants should be in the same file they are used, etc. Nonetheless, this last is more opinion-based.

'use strict';

const User = require('../../models/user'),
    { ERROR_MESSAGES, RESPONSE_STATUS } = require('../../models/constant'),
    helper = require('../../helpers/helper'),
    validator = require('validator');

const register = (req, res) => {
    const email = req.body.email;
    const password = req.body.password;
    const errors = [];

    // input validation
    validate(email, password);

    if (errors.length > 0) {
        return res.json(helper.generateResponse(errors, null, RESPONSE_STATUS.REQUEST_DENIED))
    } else {
        User.findOne({email: email})
        .then(user => {
            if (user) {
                res.json(helper.generateResponse([ERROR_MESSAGES.DUPLICATE.EMAIL], null, RESPONSE_STATUS.REQUEST_DENIED))
            } else {
                createUser(email, password);
            }
        })
        .catch(err => {
            res.json(helper.generateResponse([ERROR_MESSAGES.UNKNOW], null, RESPONSE_STATUS.REQUEST_DENIED)) 
        });
    }
}

const validate =  (email, password) => {
    if (!email || email.trim().length == 0) {
        errors.push(ERROR_MESSAGES.EMPTY.EMAIL)
    } else if (!validator.isEmail(email)) {
        errors.push(ERROR_MESSAGES.INVALID.EMAIL)
    }

    if (!password) {
        errors.push(ERROR_MESSAGES.EMPTY.PASSWORD)
    }
};

const createUser = (email, password) => {
    let user = new User()
    user.email = email
    user.password = user.generateHash(password)

    const errorValidation = user.validateSync();
    if (errorValidation) {
        errors = helper.getMongooseValidationErrors(errorValidation.errors)
        res.json(helper.generateResponse(errors, null, RESPONSE_STATUS.REQUEST_DENIED))
    } else {
        saveUser(user);
    }
};

const saveUser = user => {
    user.save()
        .then(newUser => res.json(helper.generateResponse(errors, newUser, RESPONSE_STATUS.SUCCESS)))
        .catch(error => res.json(helper.generateResponse([error], null, RESPONSE_STATUS.REQUEST_DENIED)));
};

module.exports = { register };

As I said earlier, I'd suggest a complete refactor not just making syntactic changes like the ones I commented, but also architecture changes. If you need to decide how-to structure the project, etc. Ask other questions in Stackoverflow, maybe :)

\$\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.