HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Proper prototypal programming with Node.JS

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withnodeprogrammingprototypalproper

Problem

Practicing Prototypal Code

I'm focusing on practicing a very prototypal structure with my code (non library oriented) to begin learning to reuse large portions of my application infrastructures where applicable in future projects. With this in mind, I don't have experience with prototypal programming before now, and would like to ensure that I'm not misusing the strategy.

Code Summary

The following server side JavaScript code has the purpose of handling a large number of Socket.io events in a modulated, prototypal way. My goal is code that is readable, but also reusable. I feel as if I may be sacrificing the clarity of my code in order to utilize a prototype for the handlers; it eliminates the inclusion of duplicate code in each handler, most of which helps with debug logging, but it may make the code less readable. I'm interested in learning whether or not this is an unavoidable aspect of using prototypes, or if there are ways that I may use prototypes without diminishing readability.

Review

Requesting general improvements, best practices, code readability, etc.

Most importantly, I'm interested in feedback on the comparison between the prototypal and non-prototypal event handling, and whether or not I'm using that strategy correctly.

Commenting

Another thing I wouldn't mind reviews to touch on is my style of commenting. I prefer to let the code speak for itself, but do like to keep each section and sub-section titled. I use all-caps on my in-line (sub-section) comments because I want them to stand out clearly when scrolling through.

Perhaps these practices should be changed. If so, please say so. Also, I notice that in reviews where large sections of code are involved, people will comment a paragraph of explanation / functionality description of that section. Is that ever a good practice?

Non-Prototypal Handlers

control_center.js

```
/ Server Control Center /

//REQUIRE CUSTOM MODULES

var debug = new(require("custom_modules/debug"))("Contr

Solution

The Basics

As you said, I think as each handler grows in size and the base handler adds more common features, the prototypes will pay off in spades.

Honestly, the biggest readability issue for me is the require-and-instantiate style you're using. Is this common? I haven't seen it in any of the code for the tools I mentioned in my comment above.

Compare

this.debug = new(require("custom_modules/debug"))(handlerName);


with

var Debug = require("custom_modules/debug");

...

this.debug = new Debug(handlerName);


There are a couple bugs, likely from writing code in the browser. :)

  • The handler constructor should not accept a client parameter. It should be moved to each handler's callback function instead.



  • The callbacks must refer to debug as this.debug.



A Little Style

I have a few other issues with the code in general:

-
Testing settings.debug === true seems overly-restrictive. Do you really want to disable debugging if they assign 1 instead of true?

-
I'd rather see proper JSDoc comments on properties and methods instead of "section comments".

-
handle should not instantiate a new logger instance for every client. This should be created once for each type of handler and stored in the constructor.

-
Exporting singleton instances of the handlers will make testing difficult if you need multiple instances of a single handler. Granted, they'll probably remain stateless which would allow singletons, but it just rubs me wrong. I would export the constructor instead and force clients to use new themselves.

-
Too

much

whitespace.

Use blank lines to logically group related blocks. If they are so long as to need blank lines inside, refactor.

-
While it looks really nice when assignments are all lined up on the =, it is a PITA to maintain and easy to miss when you rename a variable in a file. I find it's just not worth it in the end.

Where Are the Prototypes?

The handlers don't seem to be making use of prototypes or the prototype chain at all. The constructor exported by handler.js is merely a factory method. Nothing is assigned to its prototype to be inherited by the subclasses. Instead, it builds a new object from scratch each time. Given that both debug and handle require state about the specific handler, I don't see an easy way around this.

As it stands now, I would probably rewrite handler.js as a factory method. It still returns an object

handler.js

(function () {
    var Debug = require('custom_modules/debug');

    module.exports = function (name, callback) {
        return {
            debug: new Debug(name);
            handle: function (client) {
                this.debug.log("Initialized");
                callback(client);
            }
        };
    };
}());


Creating a new handler is pretty much the same.

login.js

(function () {
    var handler = require("custom_modules/handler");

    module.exports = handler("Login Handler", function (client) {
        client.on("login", function (data) {
            this.debug.log("Handling login...");
        });
    });
});


For these to be truly prototypal, you need to assign some properties to the constructor's prototype. But this case is very unsuited to inheritance in general. Nothing is shared by the subclasses except a single call to a per-instance logger.

Bring in the Clones Prototypes

To demonstrate how it would look using prototypes, assume that debug.js exports a function to be shared by all components, and handle will call a template method in the subclass instead of accepting a callback in the constructor. This would be more beneficial if it had to do a bunch of common setup work and provided more methods useful to the subclasses.


Warning: This code does not stand well on its own. It is way too complicated for the tiny amount of work it does now. You can alleviate the verbosity by using a bit of sugar such as Prototype or John Resig's Simple JavaScript Inheritance.

handler.js

(function () {
    var debug = require('custom_modules/debug');
    var Handler = function () {};

    Handler.prototype.debug = function (message) {
        debug(this.name + " - " + message);
    };

    Handler.prototype.handle = function (name, callback) {
        this.debug(this.name + " - Initialized");
        this.handleImpl(client);
    };

    module.exports = Handler;
}());


login.js

(function () {
    var Handler = require('custom_modules/handler');
    var LoginHandler = function () {
        this.name = "Login Handler";
    };

    LoginHandler.prototype = new Handler;

    LoginHandler.prototype.handleImpl = function (client) {
        client.on("login", function (data) {
            this.debug("Handling login...");
        });
    };

    module.exports = LoginHandler;
}());


Ick! Even with generous whitespace, this is difficult to read. Here's the same code using Resig's class.js:

handler.js

```
(function () {
var Class = requir

Code Snippets

this.debug = new(require("custom_modules/debug"))(handlerName);
var Debug = require("custom_modules/debug");

...

this.debug = new Debug(handlerName);
(function () {
    var Debug = require('custom_modules/debug');

    module.exports = function (name, callback) {
        return {
            debug: new Debug(name);
            handle: function (client) {
                this.debug.log("Initialized");
                callback(client);
            }
        };
    };
}());
(function () {
    var handler = require("custom_modules/handler");

    module.exports = handler("Login Handler", function (client) {
        client.on("login", function (data) {
            this.debug.log("Handling login...");
        });
    });
});
(function () {
    var debug = require('custom_modules/debug');
    var Handler = function () {};

    Handler.prototype.debug = function (message) {
        debug(this.name + " - " + message);
    };

    Handler.prototype.handle = function (name, callback) {
        this.debug(this.name + " - Initialized");
        this.handleImpl(client);
    };

    module.exports = Handler;
}());

Context

StackExchange Code Review Q#57429, answer score: 7

Revisions (0)

No revisions yet.