ReFactor Friday - Controlling asynchronous Function Flows

Hey! Nice to have you here.

This weeks code in question was asked in the Telegram Channel JavaScript, created and maintained by The Devs Network. Make sure to check them out!

The Code

This is the original code. We will go through it, point out some patterns and anti-patterns and what can be done to fight the trouble.

At the end of this article, we will reduce these lines down from 34 to 18 lines without dependencies, but make everything more composable and re-usable.

34 Lines

app.post('/api/v1/submit/questions', function (req, res) {
    upload(req, res, function (err) {
        let qObj;
        try {
            qObj = JSON.parse(fs.readFileSync(req.file.path, 'utf8'));
            fs.unlink(req.file.path, (err) => {
                if (err) throw err;
            });
        } catch (err) {
            res.status(400).send({ message: err.message });
            return;
        }
        qObj.forEach(question => {
            new Question({
                choice_d: question.choice_d,
                choice_a: question.choice_a,
                choice_b: question.choice_b,
                choice_c: question.choice_c,
                solution: question.solution,
                statement: question.statement,
                question_id: question.id
            }).save().then((doc) => {

            }, (err) => {
                res.status(400).send({ message: err.message })
            })
        });

        if (err) {
            res.status(400).send({ message: err.message });
        }
        res.send({ message: "File is uploaded" });
    });
})

One thing we don't know is how this Question class looks like, but we know that its save() method uses a Promise (.then()).

Can you point out what is wrong and why this may cause some struggle? I immediately noticed the following:

• Everything is one function
• Multiple try/catch blocks that do the same thing
• Same names for property assignment in creating the Question object
• If we use promises, we should use them properly or use async/await
• Interrupted flow when looping without Promise.all

Some of these are niceties and we can clean up a lot, but there is one big functional flaw which we need to fix straight ahead:

Interrupted flow when looping

This is the root cause of the mentioned problem:

Error message: UnhandledPromiseRejectionWarning: Error: Can't set headers after they are sent.

Looping through the questions doesn't return to the usual flow anymore. Also, the response is potentially sent multiple times if there is more than one exception thrown. So how do we turn back to the normal flow? For this, you can use Promise.all to make sure every save is done, before we continue in the flow.

fs.unlink also does not return to the flow, but that doesn't matter that much, as it can run in the background. But debugging this will become hell quickly.

Let's clean it up with async/await. You can either use the built-in util.promisify() to obtain Promise-based fs functions. Alternatively you can use fs-extra - which has the same API like fs.

I will also await upload while we are at it, because the callback is also just used for catching errors.

// You can promisify any function with a callback as last parameter with util.promisify
const multer = require('multer')
const { promisify } = require('util')
const upload = promisify(multer({ storage: './temp' }).single('question'))

app.post('/api/v1/submit/questions', function (req, res) {
    try {
        await upload(req, res)
        let qObj = JSON.parse(await fs.readFile(req.file.path, 'utf8'))
        await fs.unlink(req.file.path)
        await Promise.all(
            qObj.map(question =>
                new Question({
                    choice_d: question.choice_d,
                    choice_a: question.choice_a,
                    choice_b: question.choice_b,
                    choice_c: question.choice_c,
                    solution: question.solution,
                    statement: question.statement,
                    question_id: question.id
                }).save()
            )
        )
        res.send({ message: "File is uploaded" })
    } catch (err) {
        res.status(400).send({ message: err.message })
    }
})

I also turned the forEach into a map - this returns an array of promises which we can await with Promise.all

A big block is already cleaned up. Let's continue.

Multiple try/catch Blocks

They always have the same catch block: sending a status(400) and error message.

If you haven't noticed, I already cleaned that up in the step above. If you only care about the result - failed or not, you only need one catch.

try {
    // ...
} catch (err) {
    if (err.code == 'ENOENT') return res.status(400).send({message: 'File not found'})
    if (err instanceof SyntaxError) return res.status(400).send({message: 'Invalid JSON'})
    // catch all other errors
    res.status(400).send({ message: err.message })
}

Same name for properties and request

The only difference we have here is the id, which is question_id: question.id. We can shorten this by a lot by using a spread operator:

Using same name for property assignment and variable name

new Question({
    ...question,
    question_id: question.id
}).save()

We could further shorten it, if only the class would use the same property name also for the id. But we are just refactoring, not making it perfect. If the extra id property becomes a problem, we can remove it:

let {id, ...questionObject} = {...question, question_id : question.id}
new Question(questionObject).save()

What is going on here? Here I create a temporary object on the right-hand side, renaming the id to question_id. On the left-hand side I extract the question and omit the id. Using the rest operator, I can control what exactly I want. The only trade-off: I need to invent a name for the new variable because I can't reassign it to the same name: question.

Everything in one Function

You should divide your app in the logic and the part that calls the logic (rest API) This way, your application becomes a lot more composable. The HTTP API is also just one way to consume the application. What if this is not the only place where you need to read from a file?

For this, we move the repeated anonymous function into its seperate function.

Here is the final cleanup

18 Lines (without dependencies)

const express = require('express')
const storage = './uploads'
const multer = require('multer')
const { promisify } = require('util')
const upload = promisify(multer({ storage }).single('question'))
const fs = require('fs-extra')

function createQuestion(question) {
    return new Question({
        ...question,
        question_id: question.id
    }).save()
}

app.post('/api/v1/submit/questions', async (req, res) => {
    try {
        await upload(req, res)
        let questions = JSON.parse(await fs.readFile(req.file.path, 'utf8'))
        await fs.unlink(req.file.path)
        await Promise.all(questions.map(createQuestion))
    } catch (err) {
        // another hidden detail - we must return here so only one response is sent
        return res.status(400).send({ message: err.message })
    }
    res.send({ message: "File is uploaded" })
})

Summary

That's it!

We reduced lines, corrected the flow in an intuitive and predictable way and the intention and responsibility of the HTTP endpoint becomes crystal clear.