1
\$\begingroup\$

I have some code which saves a mongoose model from the submitted form data. However the user might also upload an image along with the form, which needs to be saved on the server (moved from the buffer).

Here is my first attempt:

// Save a new job to the database.
exports.save = function(req, res, next) {
  // If the request contains a file, generate a file name for it.
  if (req.file) req.body.logoImage = generateFileName(req.file.mimetype)

  // Save the job to the database.
  Job.create(req.body)
    .then(job => {
      // Move the file from the buffer to the uploads folder.
      if (job.logoImage) {
        moveFile(req.file.buffer, job.logoImage).then(() => {
          res.json({ status: 'Uploaded job successfully.' })
        })
      } else {
        res.json({ status: 'Uploaded job successfully.' })
      }
    })
    .catch(error => next(error))
}

I would like to avoid having to write duplicate code for sending the response.

The problem is that moveFile return a Promise so simply doing this won't work, because the response will be sent before the file is moved (and moving it might fail)!

if (job.logoImage) {
    moveFile(req.file.buffer, job.logoImage)
}
res.json({ status: 'Uploaded job successfully.' })

To get around this, I came up with the following code:

// Save a new job to the database.
exports.save = function(req, res, next) {
  // If the request contains a file, generate a file name for it.
  if (req.file) req.body.logoImage = generateFileName(req.file.mimetype)

  // Save the job to the database.
  Job.create(req.body)
    .then(job => {
      // Move the file from the buffer to the uploads folder.
      if (job.logoImage) {
        return moveFile(req.file.buffer, job.logoImage)
      }
      return Promise.resolve()
    })
    .then(() => {
      res.json({ status: 'Uploaded job successfully.' })
    })
    .catch(error => next(error))
}

Would there be a better way maybe?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! What task does this code accomplish? Please tell us, and also make that the title of the question via edit. Maybe you missed the placeholder on the title element: "State the task that your code accomplishes. Make your title distinctive.". Also from How to Ask: "State what your code does in your title, not your main concerns about it.". \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 3 '18 at 23:48
1
\$\begingroup\$

Your modified code is better, but you can simplify this method even more. If you don't return anything from a function in a promise, it will be resolved with no value. There's no need to do return Promise.resolve().

.catch(error => next(error)) is equivalent to .catch(next).

Code Tells You How, Comments Tell You Why. Don't clutter your code with comments that tell you nothing more than the code they are describing. "If the request contains a file, generate a file name for it." tells me nothing more than if (req.file) req.body.logoImage = generateFileName(req.file.mimetype)

Depending on your framework, async functions might be able to simplify this logic. If you are using Express, this won't apply. However, if you are using a framework which lets you return a promise in middleware, async functions are the way to go. If next(error) is equivalent to returning a rejected promise, you can just do this:

// Save a new job to the database.
exports.save = async function(req, res) {
  if (req.file) {
    req.body.logoImage = generateFileName(req.file.mimetype)
  }

  const job = await Job.create(req.body)
  if (job.logoImage) {
    await moveFile(req.file.buffer, job.logoImage)
  }

  res.json({ status: 'Uploaded job successfully.' })
}

If Job.create doesn't use req.body.logoImage and generateFileName never returns a falsy value, you can further simplify this logic by combining the if statements.

// Save a new job to the database.
exports.save = async function(req, res) {
  const job = await Job.create(req.body)
  if (req.file) {
    await moveFile(req.file.buffer, generateFileName(req.file.mimetype))
  }

  res.json({ status: 'Uploaded job successfully.' })
}

If async functions won't work for your use case (I wouldn't want to make the exported function return a promise if it is left dangling), I would go with this logic (assuming generateFileName can return a falsy value):

// Save a new job to the database.
exports.save = function(req, res, next) {
  if (req.file) req.body.logoImage = generateFileName(req.file.mimetype)

  Job.create(req.body)
    .then(job => {
      if (job.logoImage) {
        return moveFile(req.file.buffer, job.logoImage)
      }
    })
    .then(() => {
      res.json({ status: 'Uploaded job successfully.' })
    })
    .catch(next)
}
\$\endgroup\$

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.