patternjavascriptModerate
Cleaning up class creation / extension
Viewed 0 times
cleaningclassextensioncreation
Problem
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;
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;
Solution
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:
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:
In my understanding: you check first that ext is a funciton before calling the extend() method, but:
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.
- 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.
Code Snippets
if (is('Function', ext)) {
return ext.extend(proto); // holy closure!
}Context
StackExchange Code Review Q#85, answer score: 11
Revisions (0)
No revisions yet.