[mattmanser]

Having now thought about it a bit more, you could actually write the code so you don't have to baby the promise at all in normal code. So the promises didn't increase the complexity of the code really, the lack of abstraction is.

I'm thinking of something like the below as a dbHelper class.

Note I'm passing the error messages into the deferred reject method rather than using console logging. I'm not sure if the q API supports that as you don't, but if it doesn't that seems like a bit of an odd design decision by the library authors, how else are you supposed to pass errors back? Unless I'm misunderstanding promises. If you want to have console logging of failed defers I would recommend either editing the source of Q or monkey patching the reject method to do it automatically.

It's also worth noting I am deliberately not checking rows length, etc. You should let exceptions do what they're supposed to do as something very serious has gone wrong if rows.length == 0 or rows[0] is null or rows[0]['count'] doesn't exist. A SELECT COUNT should always return 1 row and if you've named the column it should definitely be there.

    //this is totally untested code to show the idea of 
    //how you should abstract the boilerplate from your code
    var dbHelper = function() {
        var db = require('./db'),
            Q  = require('q');

        var dbHelperInner = {
            query : function(query, onComplete) {
                var dfd = Q.defer();
                
                try {
                    var cmd = new DbCommand(dfd);
                    cmd.execute(query, onComplete);
                } catch(e) {
                    dfd.reject(e);
                }
                
                return dfd.promise;
            }
        }

        function DbCommand(dfd) {
            if (!db.canRead()) {
                throw "db unavailable";
            }
            
            this.dfd = dfd;
        }
        
        DbCommand.prototype.execute = function(query, onComplete) {
            var self = this;
            dbConnectionPool.acquire(function(acquireError, connection) {
                if (acquireError) {
                    self.dfd.reject(acquireError);
                    return;
                }
                connection.query(query, function(queryError, rows) {
                    if (queryError) {
                        self.dfd.reject(queryError);
                    } else {
                        try {
                            self.dfd.resolve(
                                onComplete(rows));
                        } catch (e) {
                            self.dfd.reject(e);
                        }
                    }

                    dbConnectionPool.release(connection);
                });
            });
        }
        
        return dbHelperInner;
    }();


    //and then you could use it like this
    //which is almost identical to the code I posited above
    function playlistCount(){
        return dbHelper.query('SELECT COUNT(id) AS count FROM Playlists;', function(rows) {                   
            return parseInt(rows[0]['count']);
        });
    }

[fruchtose]

First of all, thank you for taking the time to read over my code. I don't get enough of this.

You're right that 28 lines is pretty ridiculous, but it's because I never abstracted out that code. The playlist code is some of the ugliest in that project, because I got pretty lazy with it. I know it's terrible. The console logging stuff is part of that.

Q actually allows exceptions to cause promise rejections, which is both a nifty feature and a potential curse (e.g. throwing an exception before releasing a resource).

I like the changes you propose (not considering testing), but with slightly different implementation. the DbCommand should be creating its own deferreds, rather than accepting one as a parameter. This kind of promise handling is best left up to DbCommand to implement, rather than the caller. In Q it's easy to chain promises, like so:

    Q.fcall(someFunction).then(function(result) {
      return functionThatReturnsAPromise();
    }).then(function(secondResult) {
      console.log(secondResult);
    }).done();

Also, in a proper redesign, Q's denodeify function can change the whole flow of the execute function entirely:

    DbCommand.prototype.execute = function(query, onComplete) {
      var self = this;
      Q.denodeify(dbConnectionPool.acquire).then(function(connection) {
        self.connection = connection;
        return Q.denodeify(connection.query);
      }).then(function(rows) {
        return onComplete(rows);
      }).finally(function() {
        self.connection && dbConnectionPool.release(self.connection);
      }).done();
    };

Q's denodeify call works with Node.js callbacks which follow the convention that the error is the first argument, and the result all others. denodeify then converts the error into an argument for any calls to Q.fail. Any uncaught errors will be thrown after done() is called.

However, I am not sold on the idea of creating a prototype for DB commands. There's no state that needs to be held, and the code is abstract enough without introducing a prototype.

Again, thanks for the code review. The playlist DB code needs a lot of refactoring, since right now there's too much repetition I've been too lazy to fix. If you want to talk some more, feel free to email me.