FreeCodeCamp/HelpBackEnd

@CloudBranch It looks nice but I have some comments about the code:

• What you now have in a config.js are things which better go in a .env file or are somehow else injected to your app through environment variables for the sake of easier deployment and keeping passwords and sensible data out of source control (The .env should not be pushed to github)

• Consider using something like https://www.npmjs.com/package/express-flash-2 for pushing messages to the views

• You have a lot of nesting going on in your routes - especially in register. This makes it hard to read and is not necessary. Especially for the if/else nesting you can apply "return early":

  if (!email || !password || !password2) 
    return res.redirect("/?message=emptyField");
  if (password !== password2) 
    return res.redirect("/?message=passwordsDoNotMatch");
  if (password.length < 6)
    return res.redirect("/?message=passwordLength");

• Error handling: You currently just log errors instead of handling them correctly.

  bcrypt.hash(newUser.password, salt, (err, hash) => {
                  if (err) throw err;

  Async errors, to enter express error handlers, should not be thrown but pushed to the middleware chain with next(err) so an error handler middleware can chime in and handle it appropriately (Send HTTP error codes - redirect - flash messages... whatever).

  newUser
                    .save()
                    .then(user => {
                      req.login(user, function (err) {
                        if (err) {
                          return next(err);
                        }
                        return res.redirect("/dashboard");
                      });
                    })
                    .catch(err => console.log(err));

  Here you pass the error from the callback correctly but add a .catch error handler which only logs errors. The catch will probably never enter as you don't re-throw the error but if it would happen anyway (for example in the redirect), the user would know nothing about an error which is only logged.

• Avoid nesting promises/callbacks. Promises were designed for avoiding deep nesting through callbacks. If you have functions which only take callbacks and do not return promises - make a function which maps the callback to a promise. bcrypt functions return promises if you do not pass a callback so use those instead of the callback versions to keep the nesting out.

function hashPassword(password) {
  return bcrypt.genSalt(10).then(salt => bcrypt.hash(password, salt));
}

function login(req, user) {
  return new Promise((res, rej) => {
    req.login(user, err => {
      if (err) rej(err);
      else res();
    });
  });
}

let user;

USER.findOne({
  email: email
})
  .then(foundUser => {
    if (foundUser) throw new Error("alreadyInSystem");
    user = new USER({
      email,
      password
    });

    return hashPassword(user.password);
  })
  .then(hash => {
    newUser.password = hash;
    return newUser.save();
  })
  .then(user => login(req, user))
  .then(() => res.redirect("/dashboard"))
  .catch(err => next(err));