Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
  • 2 commits
  • 4 files changed
  • 0 commit comments
  • 1 contributor
Commits on Mar 20, 2012
Felix Geisendörfer felixge WIP: Initial code review comments
Pushing this now before hopping on the wrap up call, didn't get through
all code yet.
67813f2
Felix Geisendörfer felixge Finish up review a9da54f
45 lib/client.js
View
@@ -25,11 +25,29 @@ var EventEmitter = require('events').EventEmitter;
var util = require('util');
var VoltConnection = require('./connection');
+// @REVIEW: Use a named constructor instead:
+//
+// function VoltClient() { ... }
+//
+// This gives better error messages and also lets you see objects of this type
+// when doing heap profiling for memory leaks.
VoltClient = function (configuration) {
EventEmitter.call(this);
this.config = configuration;
+ // @REVIEW: I suppose these bind's are made so the 'this' context is
+ // preserved when those functions are passed along as arguments somewhere.
+ // This is generally done on an as-needed basis inside the individual
+ // functions and I'd recommend to not do it in a constructor as it obfuscates
+ // the actual object properties.
+ // If you really want to do this, at least make a utility module that does
+ // this in a generic way. Then you could do something like bindThis(this)
+ // in the constructor which will go over all methods found on the thix object
+ // and bind them. (again, I don't recommend that, but it would aid readability)
+ //
+ // Also, it seems like some of these bindings are entirely uneeded for the
+ // code to work.
this.connect = this.connect.bind(this);
this._getConnections = this.connect.bind(this);
this.call = this.call.bind(this);
@@ -43,6 +61,9 @@ VoltClient = function (configuration) {
util.inherits(VoltClient, EventEmitter);
+// @REVIEW: The function below mixes tabs and spaces. The general convention in
+// the node community is to use 2 spaces for indention. Doing so will make it
+// easier to attract contributors.
VoltClient.prototype.connect = function (callback) {
var self = this;
@@ -55,6 +76,16 @@ VoltClient.prototype.connect = function (callback) {
self._connections.push(this);
connectionCount--;
if ( connectionCount < 1) {
+ // @REVIEW Callbacks in node.js should always reserve the first
+ // argument for a potential error object. This convention is
+ // used by the node core and should be considered absolutely
+ // essential when trying to write idomatic node.js code. I will
+ // only highlight it once here, but it applies to all callbacks,
+ // always.
+ //
+ // So this code should be changed to:
+ //
+ // callback(null, connectionResults);
callback(connectionResults);
}
});
@@ -84,6 +115,18 @@ VoltClient.prototype._getConnection = function (username, password, callback) {
return connection;
}
+// @REVIEW:
+// a) 'call' is an unfortunate method name in JavaScript, as call is also
+// a special function that is attached to all functions (along with apply and
+// bind). See: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Function/call
+// I recommend using 'invoke' or similar as a method name.
+//
+// b) Providing 2 callbacks is very unusual for node libraries. In this case
+// the idomatic way to model the back-pressure API would be to take inspiration
+// from node's stream interface. So this function would return 'false' to
+// indiciate that the caller should stop / slow down. Once the client is ready
+// to do more work, it would emit a 'drain' event, see:
+// http://nodejs.org/docs/v0.6.12/api/all.html#all_event_drain
VoltClient.prototype.call = function(query, readCallback, writeCallback ) {
var con = this._getConnection();
if ( con != null ) {
@@ -119,4 +162,4 @@ VoltClient.prototype._displayConnectionArrayStats = function(array) {
}
}
-module.exports = VoltClient;
+module.exports = VoltClient;
14 lib/configuration.js
View
@@ -22,6 +22,18 @@
*/
VoltConfiguration = function() {}
+
+// @REVIEW: This is a strange way to declare object properties. It will also
+// cause unexpected results if the properties hold objects ({} or []) as those
+// would be identical for all instances of the object.
+//
+// I would recommend this as idomatic node:
+//
+// function VoltConfiguration(options) {
+// this.host = options.host || 'localhost';
+// this.port = options.port || 21212;
+// ...
+// }
VoltConfiguration.prototype = Object.create({
host: 'localhost',
port: 21212,
@@ -31,4 +43,4 @@ VoltConfiguration.prototype = Object.create({
queryTimeout: 50000
});
-module.exports = VoltConfiguration;
+module.exports = VoltConfiguration;
80 lib/connection.js
View
@@ -32,9 +32,14 @@ var util = require('util');
var Message = require('./message').Message;
var MESSAGE_TYPE = require('./message').MESSAGE_TYPE;
+// @REVIEW: Unused variables, it doesn't seem like these are used anywhere.
var VoltProcedure = require('./query');
var VoltQuery = require('./query');
+// @REVIEW: I generally recommend declaring only a single class per file
+// as that generally makes it much easier to navigate a code base. That being
+// said, since this is a purely internal collaborator I think this way is also
+// acceptable.
VoltMessageManager = function() {}
VoltMessageManager.prototype = Object.create({
uid: null,
@@ -46,6 +51,16 @@ VoltMessageManager.prototype = Object.create({
index: -1
});
+
+// @REVIEW: Those 3 methods below seem rather overkill. The same can be
+// accomplished using:
+//
+// if (vmm.readCallback) vmm.readCallback().
+//
+// Also, if the caller doesn't really care if the vmm has a callback, you
+// could also initialize readCallback / writeCallback to an empty function,
+// so it is always safe to call it. (I would not be worried about performance
+// here, calling an empty method should be very fast)
VoltMessageManager.prototype.hasReadCallback = function() {
return this._isValid(this.readCallback);
}
@@ -72,6 +87,9 @@ VoltConnection = function(configuration) {
this._manageOustandingQueries = this._manageOustandingQueries.bind(this);
this.socket = new Socket();
+ // @REVIEW: Why was this done? If anything, I would expect this to decrease
+ // throughput. However, in my testing it didn't have much of an impact either
+ // way, so I'm curious.
this.socket.setNoDelay();
this.socket.on('error', this.onError);
this.socket.on('data', this.onRead);
@@ -81,6 +99,10 @@ VoltConnection = function(configuration) {
this.isLoggedIn = false;
this.connectionCallback = null;
+ // @REVIEW: As already mentioned in the call, JS does not have associative
+ // arrays. Arrays can be used as objects, but it's considered a bad practice
+ // and sends v8 and other engines into a slow case. So do:
+ // this.calls = {};
this._calls = [];
this._callIndex = [];
this._id = 0;
@@ -90,6 +112,11 @@ VoltConnection = function(configuration) {
this.invocations = 0;
this.validConnection = true;
this.blocked = false;
+
+ // @REVIEW: I would recommend against setting up timers (or doing other
+ // things like I/O) inside of a constructor as those will be very annoying
+ // for unit testing. In this particular case I'd set up the timer when
+ // establishing the connection.
setInterval(this._manageOustandingQueries,60000);
}
@@ -114,6 +141,15 @@ VoltConnection.prototype.call = function(query, readCallback, writeCallback) {
var uid = this._getUID();
query.setUID(uid);
+ // @REVIEW: I'd probably allow for the properties below to be passed in
+ // the constructor like this:
+ //
+ // new VoltMessageManager({
+ // readCallback: readCallback,
+ // writeCallback: writeCallback,
+ // message: query.getMessage(),
+ // uid: uid,
+ // });
var vmm = new VoltMessageManager();
vmm.readCallback = readCallback;
vmm.writeCallback = writeCallback;
@@ -137,6 +173,7 @@ VoltConnection.prototype._zeros = function(num) {
return arr;
}
+// @REVIEW: The `callback` parameter does not seem to be used.
VoltConnection.prototype.close = function(callback) {
this.socket.end();
}
@@ -146,6 +183,9 @@ VoltConnection.prototype.onConnect = function(results) {
var service = this.config.service;
var sha1 = crypto.createHash('sha1');
sha1.update(this.config.password);
+ // @REVIEW: 'binary' is deprecated. Unfortunately `digest()` does not
+ // support producing buffers yet, so I guess this is ok for now. Using
+ // 'base64' would be more future-proof for now so.
var password = new Buffer(sha1.digest('binary'), 'binary');
var message = this._getLoginMessage(password);
@@ -159,7 +199,15 @@ VoltConnection.prototype.onConnect = function(results) {
VoltConnection.prototype.onError = function(results) {
results.error = true;
+ // @REVIEW: You probably don't want to produce debug output on stdout
+ // unless the user of the client asks for it.
console.log('error', results);
+
+ // @REVIEW: This method seems to be invokved for socket errors. Afaik those
+ // can occur even after the connection has been established (for example
+ // on problems with the shutdown of socket), so you may not want to
+ // delegate them to the connectionCallback. Instead you may want to consider
+ // re-emitting them as 'error' events.
this.connectionCallback(results);
}
@@ -167,6 +215,10 @@ VoltConnection.prototype.onRead = function(buffer) {
var results = null;
if(this.isLoggedIn == false) {
+ // @REVIEW This assumes that the login result message always arrives
+ // in full. It seems to be a reasonable assumption considering that
+ // the message will be small, but afaik there could be scenarios where
+ // even a small message is split up into pieces.
results = this._decodeLoginResult(buffer);
this.connectionCallback(results);
this.isLoggedIn = true;
@@ -175,6 +227,11 @@ VoltConnection.prototype.onRead = function(buffer) {
} else {
var overflow = this._overflow;
+ // @REVIEW Creating a new buffer for every incoming buffer as well
+ // as all the slice() / copy() calls below could become expensive.
+ // However, in my profiling none of this or the parser code was showing
+ // up as using much CPU, so until other bottlenecks have been plugged
+ // this is probably ok.
var data = new Buffer(overflow.length + buffer.length);
var length;
@@ -189,6 +246,10 @@ VoltConnection.prototype.onRead = function(buffer) {
results = this._decodeQueryResult(msg);
var vmm = this._calls[results.uid];
+
+ // @REVIEW Checking if (vmm) would be easier here, no need to be
+ // this explicit. (In JS objects will always be true-ish when type
+ // juggling kicks in)
if(vmm != null && typeof vmm != 'undefined') {
this.sendCounter--;
if ( vmm.hasReadCallback() == true ) {
@@ -202,6 +263,8 @@ VoltConnection.prototype.onRead = function(buffer) {
delete this._calls[results.uid];
} else {
+ // @REVIEW Should this ever happen? If it is a critical exception,
+ // you may want to emit it as an 'error' event.
console.log('vmm was not valid');
}
}
@@ -211,6 +274,11 @@ VoltConnection.prototype.onRead = function(buffer) {
VoltConnection.prototype._send = function(vmm, track) {
var result = true;
+
+ // @REVIEW: This try...catch block is a little far-reaching, you probably
+ // just want it around the socket.write() call. (Which IMO should not throw
+ // exceptions in the node core, but fixing that will be a little tough
+ // as we're trying to not change the node api as much anymore these days)
try {
if ( track == true) {
this._calls[vmm.uid] = vmm;
@@ -228,6 +296,12 @@ VoltConnection.prototype._send = function(vmm, track) {
this.validConnection = false;
result = false;
}
+
+ // @REVIEW: Not sure what `result` is meant to represent. As it is right
+ // now, it just indicates if there was an exception. However,
+ // socket.write() also does some queueing of it's own, so just because
+ // there was no exception, it doesn't mean that message has been passed
+ // off to write(2) / kernel land.
return result;
}
@@ -235,6 +309,8 @@ VoltConnection.prototype._send = function(vmm, track) {
VoltConnection.prototype._invokeWriteCallback = function(vmm) {
// only allow more writes if the queue has not breached a limit
+ // @REVIEW: I'm not sure where this `5000` value comes from. Is it an
+ // estimate?
if(this.sendCounter < 5000) {
this.blocked = false;
if ( vmm!= null && vmm.hasWriteCallback() ) {
@@ -265,6 +341,7 @@ VoltConnection.prototype._decodeQueryResult = function(buffer) {
VoltConnection.prototype._manageOustandingQueries = function() {
var tmpCallIndex = [];
+ // @REVIEW: Idiomatic JS for this is `Date.now()`
var time = (new Date()).getTime();
var uid = null;
while( uid = this._callIndex.pop()) {
@@ -288,6 +365,9 @@ VoltConnection.prototype._checkQueryTimeout = function(vmm, time) {
this.sendCounter--;
if ( vmm.hasReadCallback() == true ) {
vmm.readCallback({error: true,
+ // @REVIEW magic constant, probably should reference a
+ // variable holding this value to make things more readable.
+ // Also no reason to not use a string directly here.
status: -6,
statusString: 'Query timed out before server responded'});
}
13 lib/message.js
View
@@ -83,7 +83,17 @@ LoginMessage = function(buffer) {
util.inherits(LoginMessage, Message);
+// @REVIEW Doesn't seem like this aliasing is helping much here, as it is only
+// used once below.
var lm = LoginMessage.prototype;
+
+// @REVIEW toString() is expected to return a string in JS. For returning a JSON
+// object like this, one would use toJSON, see:
+//
+// https://developer.mozilla.org/en/JSON#toJSON()_method
+//
+// If you are using this as a way to improve console.log() output, you can
+// also provide an inspect() method which console.log() will use.
lm.toString = function() {
return {
length : this.length,
@@ -156,6 +166,9 @@ qm.toString = function() {
};
}
+// @REVIEW I would recommend using string constants rather than numbers here.
+// That's unless those values are used by the protocol itself (like it seems
+// to be the case for PRESENT below). In that case this approach is fine.
var MESSAGE_TYPE = {
UNDEFINED: -1,
LOGIN: 1,

No commit comments for this range

Something went wrong with that request. Please try again.