7 ms·

I can speak your comment, since my side project is a Node.js server that talks to several APIs, a database, and N web clients. Like you, I've worked more than a

• mattmanser 13y ago
  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.js 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?)
  • mattmanser 13y 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']); }); }