undefined | Better HN

0 pointsmattmanser12y ago0 comments

In other words, promises allow us to separate concerns. Document retrieval is one concern, collation another.

Other programming languages have this too. They're called a 'METHOD'. Sorry, couldn't resist.

On a serious note, look at your code in here:

https://github.com/fruchtose/muxamp/blob/master/lib/playlist...

And look at your 'playlistCount' function on line 39 (which for no apparent reason you've made a variable).

Now I can't understand how a promise there is helping your programming. In fact it seems to have vastly increased the complexity of the code and it is now completely unclear from a quick scan how the method works, what the program flow actually is.

It should be a much shorter function, literally just this:

    function playlistCount(){
        return db.query('SELECT COUNT(id) AS count FROM Playlists;', function(rows) {                   
            return parseInt(rows[0]['count']);
        });
    }

Your version is 28 lines!

28 LINES. HOW? For a simple COUNT!

Actually to be fair to promises, 50% of the problem here is that you've not abstracted your db code and unnecessarily repeat boiler plate connection code again and again and again.

But a big part of the problem is that you can't just throw an exception and let it bubble up the stack, you're constantly having to baby sit the code, and that is the promises fault.

(unrelated, but as a mini-code review though I've never used node.js but the oft repeated line of `dbConnectionPool.release(connection);` in the connection failure code is an immediate code smell to me, it looks like rain dance code, why would you have to release a failed connection? It failed, how can it be released?)

0 comments

mattmanserOP12y ago

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']);
        });
    }

fruchtose12y ago

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.

tel12y ago

Some pseudocode

    playlistCount :: PromiseDB Int
    playlistCount = withConn $ \conn -> do
                      result <- query 'SELECT COUNT(id) AS count FROM Playlists;' conn
                      return (parseInt $ get 'count' result)

Wrapping the pool handling into withConn, and the failure modes into query. This PromiseDB monad would be fairly trivial to produce in Haskell. It's also fairly easy to write it point-free as

    withConn $ 
      query 'SELECT COUNT(id) AS count FROM Playlists;' 
      >=> return . parseInt . get 'count'

j / k navigate · click thread line to collapse