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

Modification of javascript inheritance framework

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

Problem

I've modified John Resig simple javascript inheritance framework, by adding set function with setter functionality. It seems that it works. Is it written correctly? Can you see some undesired behavior?
Framework:

(function(){
var initializing = false, fnTest = /xyz/.test(function(){xyz;}) ? /\b_super\b/ : /.*/;
this.Class = function(){};
Class.extend = function(prop) {
var _super = this.prototype;
initializing = true;
var prototype = new this();
initializing = false;
for (var name in prop) {
prototype[name] = typeof prop[name] == "function" &&
typeof _super[name] == "function" && fnTest.test(prop[name]) ?
(function(name, fn){
return function() {
var tmp = this._super;
this._super = _super[name];
var ret = fn.apply(this, arguments);
// CHANGED HERE FROM this._super = tmp;
// don't like some not used properties
if(tmp)this._super = tmp;
// END OF CHANGE
return ret;
};
})(name, prop[name]) :
prop[name];
}
function Class() {
if ( !initializing && this.init )
this.init.apply(this, arguments);
}
Class.prototype = prototype;
Class.prototype.constructor = Class;
Class.extend = arguments.callee;

// CHANGED HERE
// Set method
Class.prototype.set = function(attrs) {
for (var attr in attrs) {
//if exist setter function this.on_change{key of wrap: function(key of wrap, new value)
if(this.on_change[attr])this.on_change[attr].call(this,attr,attrs[attr]);
else this[attr] = attrs[attr];
}
return this;
};
if(!Class.prototype.on_change)Class.prototype.on_change={};
// END OF CHANGE

return Class;
};
})();


Test code:

```
var Person = Class.extend({
a: 15,
b: 30,
init: function(){
//...
},
on_change:{
b: function(oldKey, set){this[oldKey] = set*2}
}
});
var p

Solution

Easy question first : Is it written correctly?

Class.prototype.set = function(attrs) {
  for (var attr in attrs) {
    //if exist setter function this.on_change{key of wrap: function(key of wrap, new value)
    if(this.on_change[attr])this.on_change[attr].call(this,attr,attrs[attr]);
    else this[attr] = attrs[attr];
  }
  return this;
};
if(!Class.prototype.on_change)Class.prototype.on_change={};


  • attrs is not a good name, it does not contain a list of attributes, but a list of name/value pairs, I would suggest the Spartan o for Object or maybe map.



  • Dropping the curly braces in your if statement is ok, dropping the newlines not so much



  • The one line of comment is not very helpful :\



  • You could use the following for initialization of on_change :



Class.prototype.on_change = Class.prototype.on_change || {};

  • on_change is not lowerCamelCase -> onChange would be more idiomatic.



I would counter suggest:

Class.prototype.set = function( map ){
  for ( var attributeName in map ) {
    var value  = map[attributeName],
        setter = this.onChange[attributeName];
    if( setter )
      setter.call( this, attributeName, value );
    else 
      this[attributeName] = value;
  }
  return this;
};
Class.prototype.onChange = Class.prototype.onChange || {};


Hard question: Can you see some undesired behavior?

Yes.

  • Providing onChange is old skool, real events use addEventListener.



  • There are now 2 ways to change a property, thru set and thru changing properties directly. The setter functions only work for set, this will introduce bugs.



-
You are counting on the setter to do the assignment, another possible source of bugs. I would do the assignment regardless, rename setter to listener and then execute the listener:

this[attributeName] = value;
if( listener )
  listener.call( this, attributeName, value );

Code Snippets

Class.prototype.set = function(attrs) {
  for (var attr in attrs) {
    //if exist setter function this.on_change{key of wrap: function(key of wrap, new value)
    if(this.on_change[attr])this.on_change[attr].call(this,attr,attrs[attr]);
    else this[attr] = attrs[attr];
  }
  return this;
};
if(!Class.prototype.on_change)Class.prototype.on_change={};
Class.prototype.set = function( map ){
  for ( var attributeName in map ) {
    var value  = map[attributeName],
        setter = this.onChange[attributeName];
    if( setter )
      setter.call( this, attributeName, value );
    else 
      this[attributeName] = value;
  }
  return this;
};
Class.prototype.onChange = Class.prototype.onChange || {};
this[attributeName] = value;
if( listener )
  listener.call( this, attributeName, value );

Context

StackExchange Code Review Q#6598, answer score: 3

Revisions (0)

No revisions yet.