2
\$\begingroup\$

Is this the correct way to block requests in Express?

app.js

app.use("/", indexRouter);

app.use(function(req, res, next) {
  next(createError(404));
});

app.use(function(err, req, res, next) {
  res.locals.message = err.message;
  res.locals.error = req.app.get("env") === "development" ? err : {};
  console.log(err);
  res.status(err.status || 500);
  res.json({ error: err.message });
});

controller

exports.fetch_seat = (req, res, next) => {
  const errors = validationResult(req);
  if (!errors.isEmpty()) {
    return res.status(422).json({ error: true, message: errors.array() });
  }

  Seat.getSeatByNumber(req.params.seatNumber, (err, seat) => {
    if (err) res.json(err);
    else res.json(seat);
  });
};

model

Seat.findSeat = function(key, value) {
  return new Promise((resolve, reject) => {
    const seat = file.find(r => r[key] === value);
    if (!seat) {
      reject({
        error: true,
        message: "Seat not found",
        status: 404
      });
    }
    resolve(seat);
  });
};

Seat.getSeatByNumber = function(seatNumber, result, next) {
  try {
    this.findSeat("seatNumber", seatNumber)
      .then(seat => {
        console.log(seat);
        if (seat !== undefined && Object.keys(seat).length > 0) {
          return result(null, seat);
        } else {
          return result({
            error: true,
            message: "Seat not found",
            status: 404
          });
        }
      })
      .catch(error => {
        console.log(error);
        console.log("1st catch");
        return result(error, null);
      });
  } catch (error) {
    console.log("outer catch");
    console.log(error);
    return result(error);
  }
};

I have changed the functions within my model to promises. do i need it to be promises from the controller?

\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Aug 23 at 10:31
2
\$\begingroup\$

If possible, always whitelist acceptable sources instead of trying to blacklist harmful ones. Also, don't use try-catch for control flow, use it for what it's designed for: error handling.

All that being said, you aren't passing the promise out of the model, because you wait for the response with .then, so no you wouldn't have to make the controller methods promises as well, but why wouldn't you? JS is single-threaded, so always default to asynchronous operations to be non-blocking.

\$\endgroup\$
  • \$\begingroup\$ I have added an update. not sure what i should do about the try catch. do i not need it? I have not been able to log anything in the "outer catch" \$\endgroup\$ – shorif2000 Aug 23 at 9:28
  • \$\begingroup\$ As the only place where an error can come is the promise, the outer catch it will never reach. You just need the Promise.catch. \$\endgroup\$ – Mario Santini Aug 23 at 14:33

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.