8
\$\begingroup\$

I am new to Sails.js, Node.js, and web application development in general. My current understanding is that well-written controllers should be "skinny" meaning they should be as simple as possible, easily readable, and abstracted by using helper functions to reduce the amount of code in the controller itself.

I want my controllers to make it to those standards (as well as to good practices I am missing), but I have an ugly callback "Christmas tree" look going on with several of my controllers. I do not have a comfortable grasp on how to implement promises to reduce the nesting going on. I DO know how to implement helpers in Sails.js, and I plan to get that going to reduce the nesting by validating the request data first before making the async CRUD calls. However, I am not confident I can implement validation in the controller that will certainly execute before making it to the Waterline model methods.

Below is probably the ugliest controller; an admin uses this to update a junior user's account info.

update: function (req, res)
{
    if (!(req.param("email") && req.param("name") && req.param("car") && req.param("permissions") && req.param("id")))
    {
        return res.badRequest('Update attempt failed, proposed data invalid.')
    }
    else
    {
        if (!emailIsValid(req.param("email")))
        {
            return res.badRequest('Login is not a valid email address');
        }
        else
        {
            if (!(req.param("permissions") === "junior" || req.param("permissions") === "senior"))
            {
                return res.badRequest('Invalid permissions, choose "junior" or "senior"');
            }
            else
            {
                if (!nameIsValid(req.param("name")))
                {
                    return res.badRequest('Name is not in "First Last" format or contains invalid characters.');
                }
                else
                {
                    if (req.param("id") === "")
                    {
                        return res.badRequest('Invalid id.');
                    }
                    else
                    {
                        // Try to look up user using the provided id
                        User.findOne({ id: req.param('id') }, function foundUser(err, user)
                        {
                            if (err) return res.negotiate(err);
                            if (!user) return res.notFound();

                            //if the request included a password, update it
                            //if not, update all the other properties execept password
                            if (req.param("password") === "" || !req.param("password"))
                            {
                                User.update({ id : req.param("id")},
                                {
                                    email : req.param("email"),
                                    name : req.param("name"),
                                    car : req.param("car"),
                                    permissions : req.param("permissions")
                                }).exec(function (err, updatedUser)
                                {
                                    if (err)
                                    {
                                        return res.negotiate(err);
                                    }
                                    else
                                    {
                                        return res.ok();
                                    }
                                });
                            }
                            else
                            {
                                User.update({ id : req.param("id")},
                                {
                                    email : req.param("email"),
                                    password : req.param("password"),
                                    name : req.param("name"),
                                    car : req.param("car"),
                                    permissions : req.param("permissions")
                                }).exec(function (err, updatedUser)
                                {
                                    if (err)
                                    {
                                        return res.negotiate(err);
                                    }
                                    else
                                    {
                                        return res.ok();
                                    }
                                });
                            }
                        });
                    }
                }
            }
        }
    }
},

Can someone point me in the right direction so I can get all of my controller code looking neat and acceptable?

\$\endgroup\$
1
\$\begingroup\$

I have reached a far better implementation, by my standards, by first gaining a grasp of Q promises and how Waterline's model queries work.

I figured out Q promises by watching this video on Angular's promises, which are based on Q. Very helpful: https://www.youtube.com/watch?v=cdG_T6ufcbE

I learned to use .then() and .catch() instead of .exec() with Waterline: http://sailsjs.org/documentation/reference/waterline-orm/queries

I imported Q to my project by navigating to my sails project's directory and using:

npm install q --save

To use Q anywhere in the project I added sails.Q = require('q'); in the module.exports.bootstrap in config/bootstrap.js

My function from above in api/controllers/UserController.js went from 88 lines to 18:

//admin can update a user's account data
update: function (req, res)
{
    //validate request and create an object for update
    UserModule.validateUpdate(req)
    .then(function (userUpdates)
    {
        //update the user's data
        User.update({ id : req.param("id")}, userUpdates)
        .then(function (updatedUser)
        {
            return res.ok();
        });
    })
    .catch(function (err)
    {
        return res.send(400, err);
    });
},

UserModule.validateUpdate is a helper that is in api/services/UserModule.js. Irl Nathan has an excellent tutorial on how to use helper functions in Sails.js: https://www.youtube.com/watch?v=877gSvq05fM

//Validates request and creates object that will update model with .update()
validateUpdate : function (req)
{
    var deffered = sails.Q.defer();
    var userUpdate = {};

    if (!(req.param('email') && req.param('name') && req.param('car') && req.param('permissions') && req.param('id')))
    {
        deffered.reject('Missing "Update" request parameter.');
    }

    if (!emailIsValid(req.param('email')))
    {
        deffered.reject('Login is not a vaild email address');
    }

    if (!(req.param('permissions') === 'junior' || req.param('permissions') === 'senior'))
    {
        deffered.reject('Invalid permissions, choose "junior" or "senior"');
    }

    if (!nameIsValid(req.param('name')))
    {
        deffered.reject('Name is not in "First Last" format or contains invalid characters.');
    }

    //populate the object passed to User.update() based on
    //the properties present in the request
    if (req.param('password') === "" || !req.param('password'))
    {
        userUpdate =  {
                email : req.param("email"),
                name : req.param("name"),
                car : req.param("car"),
                permissions : req.param("permissions")
            };

        deffered.resolve(userUpdate);
    }
    else
    {
        userUpdate =  {
                email : req.param("email"),
                password : req.param("password"),
                name : req.param("name"),
                car : req.param("car"),
                permissions : req.param("permissions")
            };

        deffered.resolve(userUpdate);
    }

    return deffered.promise;

},

Hope this helps anyone else that wants to have neat, elegant, abstracted, and readable controller code in Sails.js!

\$\endgroup\$
0
\$\begingroup\$

To help with your issue more generally:

I created a little helper hook for validation in Sails 1.x.

https://github.com/xtrinch/sails-hook-validation-ev

In its most simplest form with using blueprint routes, it works by adding a validate function to a model:

Todo.js:

module.exports = {

  attributes: {
    title: {    
      type: 'string',   
      required: true    
    },  
     description: { 
      type: 'string',   
      required: true    
    },
  },

  validate: (req) => {
    req.check('title')
      .exists()
      .isLength({ min: 1 }).withMessage('must be at least 5 chars long');
    req.check('description').exists();
  }

};

Example response:

{
    "errors": [
        {
            "location": "params",
            "param": "title",
            "msg": "Invalid value"
        },
        {
            "location": "params",
            "param": "title",
            "msg": "must be at least 5 chars long"
        }
    ]
}

Check API is taken from express-validator in order to not reinvent the wheel. With it you could implement all of the above mentioned.

For more options and advanced usage, see full readme.md of the hook.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. \$\endgroup\$ – Toby Speight Sep 3 '18 at 14:10

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.