rvagg/node-levelup — pull request #49 by ralphtheninja (2013-01-03 00:23:19+00:00)
Title: use util.inherits() from node core

I'm going to attempt some refactoring on parts of the code. This is what I'm best at. I've been a maintenance coder for many years and we all know it's important to polish the code until it shines. Not everyone might agree with my opinions though, but I thought that I'd give it a try anyway and see what you guys think. Perhaps we can all learn something in the end :)

I think we should use util.inherits( ) from node core instead of the non standard __proto__.__proto__ and for two reasons. First is clarity. It's much more clear what's going on in the code when you see a call to inherits( ). The second reason is symmetry. We should create our inheritance chains the same way through all layers. We also get the _super property that can be useful to have.

Since we are not using __proto__.__proto__ it means we can no longer type:

ReadStream.prototype = { }

since it would kill the prototype object that is created by the call to Object.create( ) in util.inherits( ). Some people might use this as an argument because it factors out "ReadStream.prototype" which I can agree on to some extent. But as the object literal and the methods grow larger, the more page downs and page ups you need to see what prototype object literal you are reading right now and it's not clear exactly what methods you are looking at.

Compare the following. In this snippet it's unclear in what context these methods are defined

, pipe: function (dest) {
if (typeof dest.add == 'function' && this._options.type == 'fstream') {
this._dataEvent = 'entry'
this.on('entry', function (data) {
var entry = new BufferStream()
entry.path = data.key.toString()
entry.type = 'File'
entry.props = {
type: 'File'
, path: data.key.toString()
}
entry.once('data', process.nextTick.bind(null, entry.end.bind(entry)))
entry.pause()
if (dest.add(entry) === false) {
this.pause()
}
entry.write(data.value)
}.bind(this))
}
return Stream.prototype.pipe.apply(this, arguments)
}

, _read: function () {
if (this._status == 'ready') {
this._status = 'reading'
this._iterator.next(this._cleanup.bind(this), this._onData.bind(this))
}
}

Here it's immediately clear what methods we are defining.

ReadStream.prototype.pipe = function (dest) {
if (typeof dest.add == 'function' && this._options.type == 'fstream') {
this._dataEvent = 'entry'
this.on('entry', function (data) {
var entry = new BufferStream()
entry.path = data.key.toString()
entry.type = 'File'
entry.props = {
type: 'File'
, path: data.key.toString()
}
entry.once('data', process.nextTick.bind(null, entry.end.bind(entry)))
entry.pause()
if (dest.add(entry) === false) {
this.pause()
}
entry.write(data.value)
}.bind(this))
}
return Stream.prototype.pipe.apply(this, arguments)
}

ReadStream.prototype._read = function () {
if (this._status == 'ready') {
this._status = 'reading'
this._iterator.next(this._cleanup.bind(this), this._onData.bind(this))
}
}

Another argument for this is scope. We lose one level of indentation. Would be nice to hear your thoughts on this. There is probably something I haven't thought of :)

Cheers
/Magnus