Cleaning up class creation / extension

Question

I was wondering for quite some time how to clean up the below code without blowing it up any further. The extension of the classes is the main concern here, it looks a bit too much like magic. That's mainly because it has to handle all the different cases, and I haven't figured out a way to reduce the code in a meaningful fashion here.

Maybe I'm paranoid and the code is just fine, but I'd still love to get some feedback on it.

The original code and test cases are here.

function is(type, obj) {
    return Object.prototype.toString.call(obj).slice(8, -1) === type;
}
function copy(val) { /* ...make shallow copy */ }
function wrap(caller, obj) {
    obj = obj || Function.call;
    return function() {
        return obj.apply(caller, arguments);
    };
}

function Class(ctor) {
    // ...default ctor stuff here....
    function clas(args) { /* ...actual instance ctor stuff... */}

    var proto = {};
    clas.init = wrap(ctor);

    // extend needs to be reduced in width, it easily goes over 80 columns
    // without some ugly if statements
    clas.extend = function(ext) {
        if (is('Function', ext)) {
            return ext.extend(proto); // holy closure!
        }

        for (var e in ext) {
            if (!ext.hasOwnProperty(e)) {
                continue; // a bit ugly imo, but it helps to prevent the indentation
                          // from blowing up
            }

            // this needs some refactoring, it creates bound and unbound
            var val = ext[e], func = is('Function', val);
            if (/^\$/.test(e)) { // statics
                proto[e] = copy(val);
                clas[e] = clas.prototype[e] = func ? wrap(clas, val) : val;

            } else if (func) {
                clas[e] = wrap(proto[e] = clas.prototype[e] = val);
            }
        }
        return clas;
    };

    // this could also need some clean up I suppose
    for (var i = ctor.hasOwnProperty('init') ? 0 : 1,
             l = arguments.length; i < l; i++) {

        var arg = arguments[i];
        is('Object', arg) ? clas.extend(arg) : arg.extend(clas);
    }
    return clas;
}

Answer 1

I would suggest to add comments (Javadoc-style, but probably best if much lighter) to describe the intent of each method. Even the most simple ones. I found it especially useful in JavaScript to describe:

• the type and range of arguments expected
• which arguments are optional and what are the default values
• the type of the result value, if any
• what is the result when provided arguments do not match expectations

Otherwise I agree with your inline comment "this needs some refactoring, it creates bound and unbound", corresponding code should be extracted into a separate function, which will also reduce nesting, which seems to be one of your worries.

Regarding the code itself, I would rename wrap() to bind() to match the bind() function added in ECMAScript 5 and I would rename l to length in the for loop as letter l is easily confused with number 1.

I have doubts about this portion of the code:

if (is('Function', ext)) {
    return ext.extend(proto); // holy closure!
}

In my understanding: you check first that ext is a funciton before calling the extend() method, but:

• extend() is not defined in JavaScript, it is one of your custom methods, so there is no guarantee that you will find this property on the function. You should probably add a check.
• I do not understand the intent: an inline comment would help :)

All in all, I think it's fine that the code is a bit hairy because adding support for classes in JavaScript is no simple matter, but a lot more inline comments (up to one comment per line of code for the most complex stuff) would improve the code readability immensely.

Answer 2

A few tips:

• Make sure your using good indentation
• Do not create a function that runs a native method
  • so: if(is("Function",ext)) becomes if(typeof ext == "Function")
• remove unnecessary comments and comments should only be at the head of an entity.
• Do not shorten your variables as it causes issues with the latter developers
• use stricter typecasting:
  • if (!ext.hasOwnProperty(e)) becomes if(ext.hasOwnProperty(e) == false)
• Keep your conditions in the for loops on one line
• There's is not point in reassigning a value from an array becuase you want to send it to a function
  • var arg = arguments[i]; gets removed and arguments[i] is sent to the function

Taking into account the above your class would look like so:

function Class(ClassBase)
{
    /*
        * Default Constructor, used to do XXX with YYY
    */
    var Arguments = args || [];

    function __Class(Arguments)
    {

    }

    var Prototype = {};
    __Class.Initialize= Wrap(ClassBase);


    /*
        * extend needs to be reduced in width, it easily goes over 80 columns
        * without some ugly if statements
    */

    __Class.Extend = function(ExtendableEntity)
    {
        if (typeof ExtendableEntity == "function")
        {
            return ExtendableEntity.extend(Prototype);
        }

        for (var Entity in ExtendableEntity)
        {
            if (ext.hasOwnProperty(Entity) == true)
            {
                var Value = ext[Entity]
                var _Function = (typeof Value == "function");

                if (/^\$/.test(Entity)) 
                {
                    Prototype[Entity] = Copy(Value);
                    __Class[Entity] = __Class.Prototype[Entity] = function ? Wrap(__Class, Value) : Value;
                }else 
                {
                    __Class[Entity] = Wrap(Prototype[Entity] = __Class.Prototype[Entity] = Value);
                }
            }
        }
        return __Class;
    }

    for (var i = ClassBase.hasOwnProperty('Initialize') ? 0 : 1, l = Arguments.length; i < l; i++)
    {
        (typeof Arguments[i] == 'object') ? __Class.Extend(Arguments[i]) : Arguments[i].Extend(__Class);
    }
    return __Class;
}