19
votes
\$\begingroup\$

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;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Don't emulate classes. This is ECMAScript. \$\endgroup\$ – Raynos Jan 26 '11 at 23:40
  • \$\begingroup\$ A while back I took a crack at making my own class definition system and if I recall my goals were similar to the things you accomplish herein. Yours looks more concise in various ways and JS development in general has come a long way since then, but I'll see if I can dig it up and find any nuggets of cleverness that might apply as well here. All in all, I find the latest revision to be pretty succinct for what it is, though admittedly I haven't given it a full on review. Still, I like what I see :) \$\endgroup\$ – TheXenocide Feb 10 '11 at 23:32
11
votes
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Hm renaming wrap to bind seems indeed like a good idea. the ext.extend expects another Class instance, maybe an ext instanceof clas would be a better choice here. Adding comments might help too, my main problem is that it's hard to keep them short and simple, just as it is hard naming things. I'll try to clean it up and update the question then. \$\endgroup\$ – Ivo Wetzel Jan 22 '11 at 15:20
  • \$\begingroup\$ Looks like you're on the right path :) I would suggest adding descriptions of parameters for each function, and inline comments for each bit of cleverness such as use of regular expression: "if (/^\$/.test(i))". \$\endgroup\$ – Eric Bréchemier Jan 27 '11 at 13:17
1
vote
\$\begingroup\$

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;
}
\$\endgroup\$
  • 5
    \$\begingroup\$ Uh, that's exactly what I didn't want :D Someone with a lot less knowledge about JS posting an answer. First of is("Function",ext) is not typeof. typeof is completely broken, never use it for getting the type of an object. arguments is a special variable inside a functions scope. Starting method names with uppercase goes against all common practice in JS land. hasOwnProperty only returns true or false so ! is completely ok in this case. Braces on next line can introduce subtle bugs. Check out: bit.ly/JSGarden for more infos. \$\endgroup\$ – Ivo Wetzel Jan 20 '11 at 12:17
  • \$\begingroup\$ Aside from the above, you're right on using longer variable names and that there's no point of var arg = arguments[i]. PS: Your code fails all tests :) \$\endgroup\$ – Ivo Wetzel Jan 20 '11 at 12:17
  • 5
    \$\begingroup\$ == true is in no way strict typing. == does do type coercion. If you really wanted strict comparison of types, you would have used ===. Also, it might be a good idea to check the source for left out methods before guessing around what they do, not that I'm ranting here, but I suppose that people who review code here actually put some effort into it when the links are provided. Concerning the use of uppercase, most of the big JS projects that I've seen (and also most of my code) uses uppercase only for user defined Types. \$\endgroup\$ – Ivo Wetzel Jan 20 '11 at 14:00
  • 2
    \$\begingroup\$ @RobertPitt there is actually a reason not to use upper-case for regular function names: this is a convention that indicates that the function is a constructor, i.e. it is intended to be used with the keyword new. For example, new Date() vs getDate(). \$\endgroup\$ – Eric Bréchemier Jan 22 '11 at 10:46
  • 1
    \$\begingroup\$ I would definitely not "remove unnecessary comments" and I strongly disagree that "comments should only be at the head of an entity". Comments should be as close as possible to the thing that is commented (principle of proximity, which helps to understand the association) if only because it's best to keep the comment within the same screen as commented code. \$\endgroup\$ – Eric Bréchemier Jan 22 '11 at 10:51

Not the answer you're looking for? Browse other questions tagged or ask your own question.