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

my constructor is doing work. Is this a code smell in this example?

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

Problem

I have created a queue in javascript. This queue also has blocking take(). As you all probably know this is not really true, because javascript does not have threads. It just wait until element has been added to queue. You can view to source-code at https://gist.github.com/2467432. I try to write testable code. As you can see from my code below my constructor does real work?

var EventEmitter = require('events').EventEmitter,
    Queue = module.exports = function Queue() {
    /*  Constructor is doing work..?? 
    */
    var _emitter    = this._emitter     = new EventEmitter(),
        _awaiting   = this._awaiting    = [],
        _queue      = this._queue       = [];

    _emitter.on('offer', function (data) {
        _awaiting.shift()(data);
    });

};


My question is if this a code smell? I think this class(object) is a leaf of application graph(value-object), so maybe it is not such a big deal. This way I think I can keep my API clean(easy to use). I think my class is still easy to test as you can hopefully see from my test class.


Value-objects, these tend to
have lots of getters / setters and are very easy to construct are
never mocked, and probably don't need an interface. (Example:
LinkedList, Map, User, EmailAddress, Email, CreditCard, etc...). (2)
Service-objects which do the interesting work, their constructors ask
for lots of other objects for colaboration, are good candidates for
mocking, tend to have an interface and tend to have multiple
implementations (Example: MailServer, CreditCardProcessor,
UserAthenticator, AddressValidator).

If you think it is a code smell I would like to know how you would fix this(but still keep the tests pass without any modification if possible?)

Solution

Personally the only issue I see with the code in terms of your constructor doing real work or testability is the instantiation of an EventEmitter object. Doing this in the constructor prevents you from mocking out or injecting a different class/instance for testing. You could use Dependency Injection as a simply way to resolve this:

var EventEmitter = require('events').EventEmitter,
    Queue = module.exports = function Queue( emitter ) {
    /*  Constructor is doing work..?? 
    */
    var _emitter    = this._emitter     = emitter,
        _awaiting   = this._awaiting    = [],
        _queue      = this._queue       = [];

    _emitter.on('offer', function (data) {
        _awaiting.shift()(data);
    });

};

Code Snippets

var EventEmitter = require('events').EventEmitter,
    Queue = module.exports = function Queue( emitter ) {
    /*  Constructor is doing work..?? 
    */
    var _emitter    = this._emitter     = emitter,
        _awaiting   = this._awaiting    = [],
        _queue      = this._queue       = [];

    _emitter.on('offer', function (data) {
        _awaiting.shift()(data);
    });

};

Context

StackExchange Code Review Q#11098, answer score: 2

Revisions (0)

No revisions yet.