Is this factory method a good approach?

Question

We are beginning to learn Node.js and we are not always sure if we do it the right way. We have a strong object oriented background so the adoption is not always easy. Right now we implemented a factory method, but we are not completely sure if our approach is good.

Factory Method:

var fs = require('fs');

module.exports.getHandler = function(type,callback) {
    if(!type) {
        throw new Error('Please specifiy a type');
    }
    var nodeModule = type.ucFirst()+'FieldHandler';
    var file = __dirname+'/'+nodeModule+'.js';
    fs.exists(file, function(exists) {
        if (exists) {
            var handler = require('./'+nodeModule);
            var instance = new handler;
            callback(instance);
        } else {
            throw new Error('Handler for "'+type+'" does not exist. ' +
                'Expect file of handler to have the name "'+file+'"');
        }
    });
};

Object which uses the factory method:

var FieldhandlerFactory = require('./fieldhandler/Factory');
...
...    
...
var handleField = function (actualField) {
    var callbackHandler = function (handler) {
        handler.setJson(actualField);
        handler.check();
        fieldString += handler.toView() + '\n';
        actual++;
        if (amountFields == actual) {
            // WE ARE READY DO SOMETHING
        }
    };
    var handler = FieldhandlerFactory.getHandler(type, callbackHandler);
}
for (var index in fields) {
    var field = fields[index];
    type = field.type;
    if (!type) {
        throw new Error('Field on index "' + index + '" has no attribute type');
    }
    var string = handleField(field);
}
...
...
...

The code works, so we are quite happy. Is this a good approach or do we stick too much to the classical OOP style? Especially with testing we face some problems. How to test the function handleField isolated? And with jasmine-node I'm not able to catch the exception in the factory method when the file is not found.

So we think there are tons of improvements. We are looking forward to learn from you and to exchange experiences.

Answer 1

When using callbacks, the first parameter is the error. So instead of throwing the error, you should pass it as the first argument. The signature of the callback should look like this function(err, all, other, arguments).

So instead of this

var fs = require('fs');

module.exports.getHandler = function(type,callback) {
    if(!type) {
        throw new Error('Please specifiy a type');
    }
    var nodeModule = type.ucFirst()+'FieldHandler';
    var file = __dirname+'/'+nodeModule+'.js';
    fs.exists(file, function(exists) {
        if (exists) {
            var handler = require('./'+nodeModule);
            var instance = new handler;
            callback(instance);
        } else {
            throw new Error('Handler for "'+type+'" does not exist. ' +
                'Expect file of handler to have the name "'+file+'"');
        }
    });
};

You should have something like

var fs = require('fs');

module.exports.getHandler = function(type,callback) {
    if(!type) {
        callback(new Error('Please specifiy a type');
    }
    var nodeModule = type.ucFirst()+'FieldHandler';
    var file = __dirname+'/'+nodeModule+'.js';
    fs.exists(file, function(exists) {
        if (exists) {
            var handler = require('./'+nodeModule);
            var instance = new handler;
            callback(null, instance);
        } else {
            callback(new Error('Handler for "'+type+'" does not exist. ' +
                'Expect file of handler to have the name "'+file+'"');
        }
    });
};

Then it should become pretty easy to test. As a bonus, you can have a look at https://github.com/cjohansen/Sinon.JS. It's a good module for mocks, stubs and spies.

Also, I realized that you are doing a synchronous for loop calling asynchronous functions. var aString = handleField(field); will never do what you are expected. handleField does not return anything. You should expect it in a callback. If you want a tip to handle the flow properly, use https://github.com/caolan/async. With this module, you will stay away of the mythical "callback-hell".

Enjoy, node.js!