CODEWITHSUNDEEP
javascriptnode.jsmongodbexpress
Altair312
Altair312

Reputation: 348

JavaScript NodeJs - How to refactor this bit

So I am in a middle of creating a simple CRUD application, but I stumbled with getting MongoDB auto-incrementing value of latest created account.

To be more specific, I have written the tidbit below to enable the following:

1) When registering, do some validation checks

2) Check what was the latest account number, increment by 1

3) Create a new user, add to the DB

Now, if you see below, I've marked three EXHIBITS

1 & 2) For some odd reason, if I remove the code from the route itself, then it stops working properly, but I have no idea how to get rid of repeating code since the functions are pretty much identical, but removing either of those just breaks the sequence. How can I fix this and make my code neater?

3) How would I go about extracting this function into a separate one? After fiddling with this, I only get to the point where "accountNumber is not defined".

const getLastAccountNumber = function() {
  User.find({}, { accountNumber: 1, _id: 0 }) **// EXHIBIT 1**
    .sort({ accountNumber: -1 })
    .limit(1)
    .then(function(doc) {
      if (!doc) throw new Error("Error?");
      accountNumber = doc[0].accountNumber;
      return doc[0].accountNumber;
    });
};
// TODO: Refactor methods

router.post(
  "/register",
  [check("email").isEmail(), check("password").isLength({ min: 4 })],
  function(req, res) {
    User.find({}, { accountNumber: 1, _id: 0 }) **// EXHIBIT 2**
      .sort({ accountNumber: -1 })
      .limit(1)
      .then(getLastAccountNumber())
      .then(function() { **// EXHIBIT 3**
        const errors = validationResult(req);
        if (!errors.isEmpty()) {
          return res.status(422).json({ errors: errors.array() });
        }
        const { email, password } = req.body;
        const amount = 0;
        accountNumber++;
        const user = new User({
          email,
          password,
          accountNumber,
          amount
        });
        user.save(function(err) {
          if (err) {
            console.log(err);
            res.status(500).send("Error registering new user");
          } else {
            res.status(200).send("User successfully added");
          }
        });
      });
  }
);

Really appreciate any feedback!

Upvotes: 0

Views: 81

Answers (2)

Leonard Pauli
Leonard Pauli

Reputation: 2673

Relating to your error, I believe defining the variable by adding let accountNumber; to the top of the file might be enough to get your code working, (though I do not believe it to be a great solution...), though as you asked about refactoring:

  • embracing promises: Imagine a water/data flowing through a series of pipes, and in each step, the data can be transformed. Keeping this linear flow often makes the code clean and easy to follow. If any error happens on the way, all pipes until "catch" are bypassed.
  • if an error occurs and we get straight to "catch", we might want to handle the situation differently depending on the failure reason. Thus, we can add error wrappers like ValidationError to check for.
  • furthermore, we can name the pipes properly, like getNewAccountNumber, which will work even when there are no accounts in the db
  • arrow functions are nice
// error handling

class ValidationError {
  constructor (errors) {
    this.errors = errors
  }
}

const checkValidation = (req, res)=> {
  const errors = validationResult(req)
  return errors.isEmpty()
    ? Promise.resolve()
    : Promise.reject(ValidationError(errors.array()))
}

const successResponse = (req, res, data)=> ()=> res.status(200).send(data)
const errorResponse = (req, res, message = 'Internal Server Error')=> error=>
    error instanceof ValidationError ? res.status(422).json({ errors: error.errors })
  : (console.error(error), res.status(500).send(message))


// utils

const initialAccountNumber = 0
const getNewAccountNumber = ()=> User
  .find({}, { accountNumber: true, _id: false })
  .sort({ accountNumber: -1 })
  .limit(1)
  .then(xs=> !xs || !xs.length
    ? initialAccountNumber
    : xs[0].accountNumber + 1)


// route

router.post('/register', [
  check('email').isEmail(),
  check('password').isLength({ min: 4 })
], (req, res)=> checkValidation(req, res)
  .then(getNewAccountNumber)
  .then(newAccountNumber=> {
    const { email, password } = req.body
    return new User({
      email,
      password,
      accountNumber: newAccountNumber,
      amount: 0,
    })
  })
  .then(user=> user.save())
  .then(successResponse(req, res, 'User successfully added'))
  .catch(errorResponse(req, res, 'Error registering new user'))
)

Anyhow, I would prefer to do this as one transaction, if possible by using existing db-build-in solutions (eg. the _id is already "guaranteed" to be unique, the accountNumber using this solution not as much).

Upvotes: 1

Titus
Titus

Reputation: 22474

You can do something like this:

const getLastAccountNumber = function() {
  return User.find({}, { accountNumber: 1, _id: 0 })
    .sort({ accountNumber: -1 })
    .limit(1)
    .then(function(doc) {
      if (!doc) throw new Error("Error?");
      return doc[0].accountNumber;
    });
};

router.post(
  "/register",
  [check("email").isEmail(), check("password").isLength({ min: 4 })],
  function(req, res) {
    getLastAccountNumber().then((accountNumber) => {
      const errors = validationResult(req);
      if (!errors.isEmpty()) {
        return res.status(422).json({ errors: errors.array() });
      }
      const { email, password } = req.body;
      const amount = 0;
      accountNumber++;
      const user = new User({
        email,
        password,
        accountNumber,
        amount
      });
      user.save(function(err) {
        if (err) {
          console.log(err);
          res.status(500).send("Error registering new user");
        } else {
          res.status(200).send("User successfully added");
        }
      });
    });
  }
);

Upvotes: 0

Related Questions