2
\$\begingroup\$

I have an express handler which i thought was pretty simple, but CodeClimate flagged this method as having a Cognitive complexity of 6 (5 is the max by default without flagging something)

Curious how I could simplify this method to reduce Cognitive complexity.

function updateUser (req, res) {
  // Update any fields that were passed in.
  // Explicitly checking for undefined b/c passing null values should set them to null in the db
  if (req.body.firstName !== undefined) {
    req.user.firstName = req.body.firstName
  }

  if (req.body.lastName !== undefined) {
    req.user.lastName = req.body.lastName
  }

  if (req.body.email !== undefined) {
    req.user.email = req.body.email
  }

  if (req.body.phone !== undefined) {
    req.user.phone = req.body.phone
  }

  if (req.body.fax !== undefined) {
    req.user.fax = req.body.fax
  }

  if (req.body.notes !== undefined) {
    req.user.notes = req.body.notes
  }

  req.user.save()
    .then(user => {
      return res.status(HttpStatus.OK).send(user)
    })
    .catch(err => {
      req.log.error(err)
      return handleErr(res, HttpStatus.INTERNAL_SERVER_ERROR, err.message)
    })
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Cognitive complexity or cyclomatic complexity (as in the added tag)? \$\endgroup\$ – Alexei Feb 28 '19 at 20:56
2
\$\begingroup\$

Assignments can be contracted to something like below:

 req.user.firstName = req.body.firstName || req.user.firstName;
 req.user.lastName = req.body.lastName || req.user.lastName;
 req.user.email = req.body.email || req.user.email;
 req.user.phone = req.body.phone || req.user.phone;
 req.user.fax = req.body.fax || req.user.fax;
 req.user.notes = req.body.notes || req.user.notes;

I think it can further be reduced if properties are put in some array and iterated over (since source and destination property have the same name), but I would stick with this arguably more readable text.

I am not familiar with express.js, so the following might not be applicable (I usually work with Typescript in Angular).

Your request have both a body and some user property. It is recommended to clearly separate output from input in order to obtain pure functions as much as possible. Something like below:

 updateUserFromRequest(req, user) {

     // clone to avoid changing provided reference
     var ret = [... user];

     // assignment logic here using ret

     return ret;
 }
\$\endgroup\$
1
  • \$\begingroup\$ This is a bit off. I only want to set req.user.xxx if it exists on req.body (meaning if it's part of the request payload) \$\endgroup\$ – Catfish Mar 5 '19 at 0:21
2
\$\begingroup\$

If you are using Mongoose, another possibility is to simply use your model validation to check if required data are present or not.

Then it will give you something like this:

function updateUser (req, res) {
  req.user.set(req.body);
  req.user.save()
  //... Then catch and handle validation error :)
}

More info on .set() method.
More info on model validation.

\$\endgroup\$
2
  • \$\begingroup\$ Not using mongoose. Using Sequelize. It doesn't validate payloads though. \$\endgroup\$ – Catfish Mar 5 '19 at 0:20
  • \$\begingroup\$ You can see more about model validation with sequeliez here : docs.sequelizejs.com/manual/tutorial/… but it seems to mee that it works the same :) \$\endgroup\$ – laudeon Mar 6 '19 at 9:29

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.