So here is my current code:

/**
 * User Registration
 * Sends a response object with success true if user was registered.
 * Sends a response object with success false if user couldn't be registered.
 */
router.post('/', function(req, res, next) {
    var userInfo = req.body;

    userInfo.email = sanitize(userInfo.email);
    userInfo.password = sanitize(userInfo.password);

    if(!validEmail(userInfo.email) || !validPassword(userInfo.password)) { //If the request info is invalid
        sendJsonResponse(res, false, error, 400);
    }
    else {
        //Check if e-mail already exists
        User.find({email: userInfo.email}, function(err, docs) {
            if(err) {
                console.log(err);
                sendJsonResponse(res, false, "There was an error processing your request.", 400);
            }
            else if(docs.length > 0) { //If e-mail was found in database, generate error
                sendJsonResponse(res, false, "This e-mail already exists, please login or use another e-mail.", 409);
            }
            else {
                //Create user based on data submitted.
                var user = new User({email: sanitize(userInfo.email), password: bcrypt.hashSync(sanitize(userInfo.password), 10), permission_level: 'REGULAR'});

                //Save it in the database.
                user.save(function (err) {
                    if (err) {
                        sendJsonResponse(res, false, "There was an error processing your request.", 400);
                    }
                    else { //If everything is fine

                        //Create token to login the registered user
                        var token = jwt.sign(user, config.secret, {
                            expiresIn: '24h' // expires in 24 hours
                        });

                        sendJsonResponse(res, true, "You have been registered!", 200, token);
                    }
                });
            }
        });
    }
    error = "";
});

This code is for user registration. I'm using functions I wrote (validEmail/validPassord) to validate the email and password that are sent to my REST API.

There's also sendJsonResponse, which sends the response with specific errors and error codes.

I think what I'm doing isn't too bad, but I still feel there is room for improvement here. I'm particularly not fond of all the if/elses. Is there anything I can do here to improve best practices/code quality?

So here is my current code:

/**
 * User Registration
 * Sends a response object with success true if user was registered.
 * Sends a response object with success false if user couldn't be registered.
 */
router.post('/', function(req, res, next) {
    var userInfo = req.body;

    userInfo.email = sanitize(userInfo.email);
    userInfo.password = sanitize(userInfo.password);

    if(!validEmail(userInfo.email) || !validPassword(userInfo.password)) { //If the request info is invalid
        sendJsonResponse(res, false, error, 400);
    }
    else {
        //Check if e-mail already exists
        User.find({email: userInfo.email}, function(err, docs) {
            if(err) {
                console.log(err);
                sendJsonResponse(res, false, "There was an error processing your request.", 400);
            }
            else if(docs.length > 0) { //If e-mail was found in database, generate error
                sendJsonResponse(res, false, "This e-mail already exists, please login or use another e-mail.", 409);
            }
            else {
                //Create user based on data submitted.
                var user = new User({email: sanitize(userInfo.email), password: bcrypt.hashSync(sanitize(userInfo.password), 10), permission_level: 'REGULAR'});

                //Save it in the database.
                user.save(function (err) {
                    if (err) {
                        sendJsonResponse(res, false, "There was an error processing your request.", 400);
                    }
                    else { //If everything is fine

                        //Create token to login the registered user
                        var token = jwt.sign(user, config.secret, {
                            expiresIn: '24h' // expires in 24 hours
                        });

                        sendJsonResponse(res, true, "You have been registered!", 200, token);
                    }
                });
            }
        });
    }
    error = "";
});

This code is for user registration. I'm using functions I wrote (validEmail/validPassord) to validate the email and password that are sent to my REST API.

There's also sendJsonResponse, which sends the response with specific errors and error codes.

I think what I'm doing isn't too bad, but I still feel there is room for improvement here. I'm particularly not fond of all the if/elses. Is there anything I can do here to improve best practices/code quality?

• javascript
• node.js

• You may want to look into promises, it will prevent the callback hell. – Alexandre Borela Commented Mar 25, 2017 at 22:59
• Do you mind elaborating a little bit on this? Where would I use a promise here? – snanda Commented Mar 25, 2017 at 23:04
• I think what your doing is great. Best advice is if this works press on get the rest of your solution working and e back to this later. Chances are later on you'll find a solution for something else that will work here. I have wasted too many years trying to build great code. No one knows. Otherwise minify it. – MartinWebb Commented Mar 25, 2017 at 23:15
• check this video youtube./watch?v=2d7s3spWAzo and this library bluebirdjs./docs/working-with-callbacks.html – Alexandre Borela Commented Mar 25, 2017 at 23:18
• 1 strongloop./strongblog/… this article also pare callbacks to promises and other methods. But other than that, your code is fine, I would just try to reduce the excessive nesting e.g. in the first if, you could just "return" and you wouldn't need the "else". – Alexandre Borela Commented Mar 25, 2017 at 23:23

2 Answers 2

If we only look at your code, it's fine (logic, error handling, function to format your responses ...). But looking at your code, I assume that you're using the Express micro-framework to create your back-end service (API ?), so there's things to improve about the design of your API and the way you use Express ...

Separate DB from routes

One request to your DB could result in different errors (duplicate key, some constraints, bad values ...) and serve different perpices. So it's better to create modules to handle interactions with the DB, so error handling will be cleaner and code more reusable.

Example :

Creating a module userModel wich will contain multiple methods to deal with users in the DB.
Consider the getBy method that will allow you to filter users by 'keys' (getBy({name: 'Doe', age: 25})).
This method can be used by different routes and return different kind of errors, so it's better to write it inside of a module.
It will minify code repetition, make the code more readable and help you to improve error handling.

Express routers

I see that you're using Express router (router.post('/', ... )).
Since you're designing a REST API, your methodes GET, POST, PUT and DELETE will share the same route name.
Express routers allows you to do so with the following syntax :

router.route('/routeName')
      .get(...)
      .post(...)
      .put(...)
      .delete(...);

This isn't something big, but it makes the code nicer and forces you to respect REST architectural constraints.

Express middlewares

• Modifiy the recieved request (by modifing the req parameter)
• send a Response with the methods provided by the res parameter (ex: res.status(200).json({created: true}))
• call the next middleware just like that : next()

So middlewares allows you to build the logic of your program with a simple list of functions that will be called one after the other.

Example :

[check req.body](1) --> [write user in the DB](2) --> [format response](3)

Middleware are just amazing and will make you write better, cleaner and well designed APIs :)

async/await

We all hate callback hell, that's a fact, and that's why we generally use things like Promise. But even Promises can be painful sometimes, so we have async/await that makes things just simplier :
It allows you to write asynchronous code the same way you write synchronous code !

Some code to illustrate the above points

|___models/
|   |___users.js
|
|___controllers/
|   |___users.js
|
|___routes/
|   |___register.js
|
|___middlewares/
    |___validation.js

// models/users.js
import db from 'dbModule';

const save = async (userObj) => {
    const testUserMail = await db.users.find({email: userObj.email});
    if (testUserMail.length > 0)
      return {success: false, err: "The email already exists"};
    await db.users.save(userObj);
    const savedUser = await db.users.findById(userObj.id);
    return {success: true, user: savedUser};
}

exports default {save};

// controllers/users.js
import userModel  from '../models/users';

const register = (req, res, next) => {
  console.log(req.example); // "hey I'm here"
  const dbReq = await userModel.save(req.body);
  if (dbReq.err && !dbReq.success)
    return res.status(400).json({success: false, data: dbReq, message: "failed !"})
  const registeredUser = dbReq.user;
  res.json({success: true, data: {user: registeredUser}, message: "User is registered !");
}

export default {register};

// middlewares/validation.js
const checkUser = (req, res, next) => {
  req.example = "hey I'm here";
  if (isOk(req.body.email) && isOk(req.body.password))
    return next();
  else
    return res.status(400).json({success: false, data: null, message: "failed: bad request body"}))
}

export default {checkUser};

// routes/register.js
import express    from 'express';
import validation from '../middlewares/validation';
import userCtrl   from '../controllers';

const router = express.Router();
router.route('/')
      .post(validation.checkUser, userCtrl.register)

Quick and last (I swear) explanation :

• First of all we check our body (we can modify it if we want). If something goes wrong, we send a 400 Bad request Response, else, we call next and go to the next middleware (userCtrl.register).
• We retrieve the req with the modifications performed by the previous middleware (here, we added the example key to our req object).
• we save the user in the DB by calling ( = awaiting for) an async function --> avoiding the callback hell
• That's all, we save the user and send a 200 Response.
  

The answer above is quite good, but I just thought I'd add a few points. The validation logic looks pretty good to me: the only things I can think of are that you ensure that the email is of a valid format, the password meets your app's requirements, and that the email doesn't already exist in your app, all of which you've already done.

However, it does look a bit messy, and doesn't exactly take advantage of Express. Once the sendJsonResponse call is hit, all other execution stops, which eliminates the necessity of the else clauses. So, what I've always done, is similar to the following:

if (!validEmail || !validPassword) /* do error stuff here*/ 

There is no need for an else in this case, since the error response is sent at this point, and if the credentials are valid, then we simply continue execution.

Now I know this is slightly off the topic of your question, but it looks like you created your own method for handling errors in your sendJsonResponse method. Express has built in error handling, which you may be interested in; you simply declare a function with the following signature: errFunc(err, req, res, next), and declare, for example, app.use(errFunc). Then, in order to activate it, you simply call next in your middleware methods with an error object passed as the argument. So, for example, you can do the following:

if (!validEmail || !validPassword) next(new Error("invalid credentials supplied"))

And Express knows to skip the rest of the middleware pipeline and go straight to the error handler.

Lastly, I have learned to only send a response to the user in the last middleware function in the pipeline, since it can easily get way too messy having multiple places in your app sending stuff back to the user. (I once spent the better part of 3 hours wondering why a function wasn't getting called, only to find that I had a res.json in a previous function). So, what I've always done, is defined simple methods that just call res.jsonor res.sendFile or whatever else I need, and use those as the last function in my pipeline.

Hope this helps!