3
\$\begingroup\$

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 = new Person();
p.set({a:25,b:40});
\$\endgroup\$

migrated from stackoverflow.com Dec 7 '11 at 13:51

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ Code like that is an anti pattern. It should be avoided by removing it. \$\endgroup\$ – Raynos Dec 9 '11 at 6:12
  • 1
    \$\begingroup\$ Can you be please more specific. Very similar approach you can see in prototype.js or backbone.js. Extendable is also jquery object. Maybe there is more coupling on "on_change" object, but setter are placed inside the object. Code is dry and readable in my opinion. \$\endgroup\$ – Zdeněk Mlčoch Dec 10 '11 at 11:01
  • \$\begingroup\$ I mean using setters and adding listeners to them. It's an anti pattern and should be avoided. There are a view use-cases for reactive programming but those are edge cases. \$\endgroup\$ – Raynos Dec 10 '11 at 12:49
  • \$\begingroup\$ Two moths ago, I've started using Dart. There are native setters and getters. Look at solution of Dart team: dartlang.org/articles/idiomatic-dart/… \$\endgroup\$ – Zdeněk Mlčoch Jan 28 '14 at 22:36
3
\$\begingroup\$

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.

  1. Providing onChange is old skool, real events use addEventListener.
  2. 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.
  3. 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 );
    
\$\endgroup\$
  • 1
    \$\begingroup\$ One nitpick: o is never a good name. \$\endgroup\$ – James Khoury Jan 28 '14 at 1:58
  • \$\begingroup\$ I would upgrade that nit-pick to one GobSmacker: o is never a good name !! \$\endgroup\$ – rolfl Jan 28 '14 at 2:04
  • \$\begingroup\$ Disagreed : codinghorror.com/blog/2008/07/spartan-programming.html 'o' like 'i' and 's' are very often good names. I did provide an alternative for the faint of heart ;) \$\endgroup\$ – konijn Jan 28 '14 at 2:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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