3
\$\begingroup\$

I am creating a util module that I'm using to communicate with a MS-SQL database. I want each public method to return a promise. I started with a private function that executes a DB query and returns a promise. The function establishes a connection to the DB then it executes the query. My current code just lets all errors fall through to a single catch, and I'm not sure if this is a best practice or not.

I was hoping someone could answer that for me and any other suggestions about code read ability or anything.

var config = require('./mainConfig')();
var sql = require('mssql');
var connection =  new sql.Connection(config.db.sql);
var Promise = require("bluebird");





exports.getAll = function(table){

    return executeQueryStatment("select * from " + table);

};



function executeQueryStatment(query, newconnection) {

    var conn =  newconnection || connection; 
    var request = new sql.Request(conn);

    return new Promise(function(res, rej){

     conn.connect().then(function(){

       return request.query(query);

     }).then(function (data) {

         res(data);
         conn.close();
     })
     .catch(function(err) {
        // plan on handling all errors here
        // is it better to handle each error individually upstream?  
        console.log(err);
        conn.close();

    })

  });


};
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Good job on your first question. \$\endgroup\$ – SirPython Jan 11 '16 at 23:37
1
\$\begingroup\$

Having a catch at the very end is fine. This assumes that your errors weren't recovered or handled midway.

Promise.reject('foo').then(v => console.log(v), e => console.log(e)).catch(e => console.log(e));
> foo // from `then`

In the code above, the Promise failed. However, it had a chance to recover via then's reject handler. Thus, the promise returned after that is actually resolved rather than rejected, causing catch to not execute. To log those recoveries, do a console.log at the recovery site instead, and not rely on catch.

As for the rest of your code

I suggest you generate your query using template strings. That way, you avoid ugly string concatenation. Note that template strings use backticks (``).

I also suggest you just create a connection and leave it open. There is some overhead in creating a connection, you'd want to avoid that especially when you hit the DB hard.

Your MySQL driver appears to already use Promises. No need to wrap the entire thing in a Promise. Simply return the object created by the API.

// Assuming connection is a promise from a connect()
return connection.then(() => request.query(query)).catch(err => {
    console.log(err);
    throw err;
});
\$\endgroup\$
  • \$\begingroup\$ Awesome thanks for the help and advice Joseph \$\endgroup\$ – TimCodes Jan 12 '16 at 17:15

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.