How to decouple and encapsulate long Node.js method?

Question

I have the following method, simplified:

var component = require('../../component.js')
exports.bigMethod = function (req, res) {
    var connection = new sql.Connection(process.config.sql, function (err) {
        new sql.Request(connection)
            .input('input', sql.VarChar, 'value')
            .execute('dbo.sp1')
            .then(function (data) {
                if(data[0].length === 0) {
                    res.status(500).send({ message: 'Some Error' });
                }

               // set some local variable I'll use later: localVar

                new sql.Request(connection)
                    .input('input1', sql.Int, req.query.input1)
                    .input('input2', sql.Int, req.query.input2)
                    .input('input3', sql.DateTime2, req.query.input3)
                    .input('input4', sql.DateTime2, req.query.input4)
                    .execute('dbo.sp2')
                    .then(function (recordset) {
                        json2csv( { data: recordset[0] }, function(err, data) {
                            if (err) {
                                res.status(500).json(err);
                            }
                            fs.writeFile(localVar.path, data, function (err) {
                                if (err) {
                                    res.status(500).json(err);
                                }
                                try {
                                    var email = new component.Email();
                                    email.set_callback(function (error) {
                                        if (error) {
                                            res.status(500).send({ message: 'Another error' });
                                        }
                                        res.jsonp([]);
                                    });
                                    email.send(
                                        localVar,
                                        {
                                            filename: localVar.name,
                                            path: localVar.path
                                        }
                                    );
                                } catch (e) {
                                    res.status(500).send({ message: 'Another Error' });
                                }
                            })
                        });

                    })
                    .catch(function (err) {
                        res.status(500).send({ message: 'Another Error' });
                    });
            })
            .catch(function (err) {
                res.status(500).send({ message: 'Another Error' });
            });

    });
};

As you can see, it's a long method, which is an anti-pattern. Furthermore, the call to sp2 is actually a duplication: there's another component that makes a similar call, only returning the result of json2csv directly.

Now, I'm not so sure how to go about dividing this method to fully take advantage of reusability. Should I encapsulate the database calls in functions returning Promises? Should I use some currying?

Thanks.

It cleans up pretty nicely if you just pull out that function: `var myfunc = function (recordset) {...}` and then that line of in bigMethod just becomes `.then(myfunc(recordset))` — Oscar, Mar 10 '17 at 20:33
If this is working code that you want help improving, then this post probably belongs at http://codereview.stackexchange.com with a good description for what the code is trying to do, not here. — jfriend00, Mar 10 '17 at 20:43

score 1 · Accepted Answer · edited May 23 '17 at 12:25

Dividing your code into different functions should be your top priority — that function alone is handling way too many things which could be easily divided.

You can make your code function much cleaner by dividing through callbacks and promises, as you already suspected.

Callback example:

new sql.Connection(..., handleSQLConnection);
handleSQLConnection(error) { ... }

Promise example:

doSomething
  .then((a) => doOtherStuff(a))
  .then((b) => doSmthElse(b));

doOtherStuff(a) { ... }
doSmthElse(b) { ...}

Nonetheless, refactoring is very opinionated. But you should try to avoid God functions, and instead write functions that do one thing but they do it well.

How to decouple and encapsulate long Node.js method?

1 Answers1