Refactor business logic from controller into service in Node.js

Question

I'm learning Node.js and I have a problem with code refactoring. I read about the code architecture in Node.js and good coding practice and I want to refactor my code.

My current code:

user.controller.js

const bcrypt = require('bcryptjs');
const User = require('../models/user');

exports.createUser = (req, res, next) => {
  bcrypt.hash(req.body.password, 10)
    .then(hash => {
      const user = new User({
        email: req.body.email,
        password: hash
      });
      user.save()
        .then(result => {
          res.status(201).json({
            message: 'User created!',
            result: result
          })
        })
        .catch(err => {
          res.status(400).json({
            message: 'An unknown error has occurred.'
          })
        });
    });
}

I want to put all business logic into services. I tried something like this:

user.controller.js

const UserService = require('../services/user.service');

exports.createUser = async function (req, res, next) {

  try {
    var result = await UserService.createUser(req.body.email, req.body.password);
    return res.status(200).json({ result: result, message: "User created!" });
  } catch (e) {
    return res.status(400).json({ message: e.message });
  }

}

user.service.js

const bcrypt = require('bcryptjs');
const User = require('../models/user.model');

exports.createUser = async function (email, password) {

  bcrypt.hash(password, 10)
    .then(hash => {
      const user = new User({
        email: email,
        password: hash
      });
      user.save()
        .then(result => {
          return result;
        })
        .catch(err => {
          throw new Error(err);
        });
    });

}

But I get many errors about promise:

(node:3760) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not ha
ndled with .catch(). (rejection id: 1)
(node:3760) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

I'm newbie in Node.js and JavaScript. How to fix this?

[go to the link, Similar kind of question](https://stackoverflow.com/questions/39716569/nodejs-unhandledpromiserejectionwarning) — imLohith, Oct 29 '19 at 12:20

score 0 · Answer 1 · answered Oct 29 '19 at 12:25

If you want tu uses promises use promises, when coding consistency is gold. If you want to use async await (syntactic sugar for promises) stick with async await every where.

Promises handle errors with a chained catch at the end.

MyPromise().then(() => bar).catch((err) =>  do something with err )

For async / await you should put a

try { } catch (err) {}

In your case it only saying that in any of your async function that contains a promise you should wrap them with try catch.

Hope it helps. My last recomendation learn one way first then the other way and don't mix them.

score 0 · Answer 2 · answered Oct 29 '19 at 16:23

Thank you for answers. I modified my code and now looks like this:

user.controller.js

const UserService = require('../services/user.service');

exports.createUser = async function (req, res, next) {

  try {
    let user = await UserService.createUser(req.body.email, req.body.password);
    return res.status(201).json({ data: user, message: 'User created!' });
  } catch (e) {
    let errorMsg;
    let statusCode;
    if(e.errors['email'].kind === 'unique') {
      statusCode = 422;
      errorMsg = 'E-mail already exists.';
    } else {
      statusCode = 500;
      errorMsg = 'An unknown error has occurred.';
    }
    return res.status(statusCode).json({ message: errorMsg });
  }

}

user.service.js

const bcrypt = require('bcryptjs');
const User = require('../models/user.model');

exports.createUser = async function (email, password) {
  const hash = bcrypt.hashSync(password, 10);
  const user = new User({
    email: email,
    password: hash
  });
  await user.save();
  return result;
}

Is my code correct? I am talking about coding architecture. Works fine, but I'm not sure if my code is "good practice" programming. I am asking because I want to start writing the whole backend and I'm not sure of my code.

I think this part of the code doesn't look good:

  } catch (e) {
    let errorMsg;
    let statusCode;
    if(e.errors['email'].kind === 'unique') {
      statusCode = 422;
      errorMsg = 'E-mail already exists.';
    } else {
      statusCode = 500;
      errorMsg = 'An unknown error has occurred.';
    }
    return res.status(statusCode).json({ message: errorMsg });
  }

In other controllers, for example, I would have 5 If statements. Is this the correct solution? I am asking for advice what can be changed here to make the code professional.

Instead of service, I think what you are trying to do is a Model. Then about the way you handle the error as you comment it smells (i contains a lot of ifs) you can look for strategy pattern... And finally, I would say that start with something you can always refactor your code later. Create good habits take time so you have to read a lot and share your ideas out there. Best of luck! — servatj, Nov 01 '19 at 16:55

Refactor business logic from controller into service in Node.js

2 Answers2