Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm relatively new to JavaScript OOP and would like to know the best practice when doing a simple object such as a Model.

Here's a piece of code representing class named "Child".

What I'm asking for is:

  1. Especially about the usage of this (and the weak reference I'm doing which I name self)
  2. The global shape of the code (methods in prototype etc)
  3. And what you would advise between declare a class as function, or with JSON semantic ?

var Child = function( json ) {
    var self = this;
    // Properties
    this.id = 0;
    this.firstname = "";
    this.moneyCurrent = 0;
    this.moneyDue = 0;
    this.missionsPlay = 0;
    this.missionsWait = 0;
    this.missionsStop = 0;

    this.missions = [];

    this.initialize( json );
}

Child.prototype = {
    initialize: function( json ) {
        var self = this;
        self.id = json.id_child;
        self.firstname = json.firstname;
        self.moneyCurrent = parseFloat(json.already_paid) + parseFloat(json.to_pay) - parseFloat(json.spent_money);
        self.moneyDue = parseFloat(json.to_pay);
        self.missionsPlay = json.nb_missions.in_progress;
        self.missionsWait = json.nb_missions.waiting;
        self.missionsStop = json.nb_missions.completed;
    },

    getMissions: function( type, callback ) {
        var self = this;
        session.requestServer( "parent_getChildMissionList", {
            sid: session.sid,
            id_child: self.id,
            status: type ? type : "",
            strict_status: false,
            page: 0
        }, function( data ){
            var missions = [];
            $(data.missionList).each(function(){
                self.missions.push(new Mission(this));
                missions.push(self.missions[self.missions.length-1]);
            });
            callback( missions );
        });
    },
};

Edit for question 3

Classic way to declare a class:

var obj = function() { 
    var attribut = "foo";
    this.metho = function(parameter1, parametre2) { 
        alert("parameters: " + parameter1 + ", " + parameter2); 
    } 
} 

JSON way to declare a class:

var obj = { 
    attribut: "foo", 
    method: function(parameter1, parameter2) { 
        alert("parameters: " + parameter1 + ", " + parameter2); 
    } 
} 
share|improve this question
3  
As far as I can see, your code is perfectly on topic. For more information you might want to check the help center –  Vogel612 Apr 9 at 12:40
add comment

3 Answers 3

up vote 5 down vote accepted

For your questions:

  • The usage of this : In my mind, unless you are using closures, do not use self = this, so only keep it in getMissions
  • Functions in prototype <- Good stuff
  • Function <> JSON, is an odd question. You want to use constructors, and those are functions.

Other than that,

  • You are using lowerCamelCase, that is good. Though firstname -> firstName
  • initialize: function( json ) does not really get a JSON string, instead it gets an object, so I would not name that parameter json
  • The function to deal with returned missions seems a bit clumsy, I would either assign each new mission to a variable and push it to both arrays:

    function( data ){
        var missions = [], mission;
        $(data.missionList).each(function(){
            mission = new Mission(this)
            self.missions.push( mission );
            missions.push( mission );
        });
        callback( missions );
    }
    

    or, I would only push to missions and then concatenate missions into self.missions

    function( data ){
        var missions = [];
        $(data.missionList).each(function(){
            missions.push(new Mission(this));
        });
        self.missions = self.missions.concat( missions );
        callback( missions );
    }
    

All in all, nice code, very maintainable and self explanatory.

share|improve this answer
    
Thank you for your answer, it's wath I searched as an answer, especially the "Other than that" really usefull :) I edited my question to try to be clearer on my 3th question (sometime I lack to express myself). I read it in a course but they don't say which one could be better and why. I wonder about it. –  Anc Ainu Apr 9 at 13:08
    
Singletons -> 'JSON Way', if you are going to have many objects of the same type -> constructor functions –  konijn Apr 9 at 13:24
    
In which way JSON Way is less good for reusable class than constructor functions ? –  Anc Ainu Apr 9 at 13:27
    
I would use map instead of each in those last 2 examples –  megawac Apr 9 at 13:28
    
+1 for self = this seeing that convention used improperly bothers me way more than it should. –  pllee Apr 9 at 21:53
add comment

You don't actually need the self variable in most of your code (the constructor doesn't use it at all). You can just use this directly in most cases.

The only exception is in the session.requestServer callback, where you do need a way to reference the object context - and there it's perfectly fine to use self. Alternatively, you can use .bind() to derive a function that's bound to the right context (but if you're targeting browsers know that support for bind is spotty).

More generally, I'd skip the initialize method. It depends on what you intend of course, but having the initialize function there means that you can re-initialize a Child instance whenever you want, because it'll have a publicly accessible initialize function that anyone can call. But initializing an instance should be the responsibility of the constructor function, unless there's some good reason to do otherwise. In this case, however, I don't think there is.

In other words, simply put everything in the constructor function, and remove the initialize function.

I end up with this; the "classic" way to define a class (i.e. construtor+prototype) in javascript.

function Child(json) { // equivalent to `var Child = function ...`; just for illustration
  // properties - with defaults
  this.id = json.id_child || 0; // null might be a better default for the ID
  this.firstname = json.firstname || "";
  this.moneyDue = parseFloat(json.to_pay) || 0;
  this.moneyCurrent = parseFloat(json.already_paid) + this.moneyDue - parseFloat(json.spent_money) || 0;
  this.missionsPlay = json.nb_missions.in_progress || 0;
  this.missionsWait = json.nb_missions.waiting || 0;
  this.missionsStop = json.nb_missions.completed || 0;

  this.missions = [];
}

Child.prototype = {
  getMissions: function (type, callback) {
    var self = this;

    // I assume `session` is a global or closed-over variable?
    session.requestServer( "parent_getChildMissionList", {
      sid:      session.sid,
      id_child: this.id,     // we can still use `this` here
      status:   type || "",
      strict_status: false,
      page: 0
    }, function (data) {
      var missions = [],
      mission;
      $(data.missionList).each(function () {
        mission = new Mission(this); // easier than having to go through missions[missions.length-1]
        self.missions.push(mission); // This is where we need to use the `self` var
        missions.push(mission);
      });
      callback(missions);
    });
  } // removed a stray comma on this line - some JS interpreters choke on them, some don't
};

Now, you could conceivably just use the JSON more directly. Perhaps have a factory-like method that takes in the "raw" JSON, and add a few things or builds a new object literal from it. That'd look something like:

function buildChild(json) { // note lowercase naming; this isn't a constructor
  var child = {
    id: json.id_child || 0,
    firstname: json.firstname || "",
    moneyDue: parseFloat(json.to_pay) || 0,
    moneyCurrent: parseFloat(json.already_paid) + this.moneyDue - parseFloat(json.spent_money) || 0,
    missionsPlay: json.nb_missions.in_progress || 0,
    missionsWait: json.nb_missions.waiting || 0,
    missionsStop: json.nb_missions.completed || 0,
    missions: [],
  };

  child.getMissions = function (type, callback) {
    session.requestServer( "parent_getChildMissionList", {
      sid:      session.sid,
      id_child: child.id,     // now we use `child` instead of this
      status:   type || "",
      strict_status: false,
      page: 0
    }, function (data) {
      var missions = [],
      mission;
      $(data.missionList).each(function () {
        mission = new Mission(this);
        child.missions.push(mission); // same here
        missions.push(mission);
      });
      callback(missions);
    });
  };

  return child;
}
share|improve this answer
add comment

Or if you want to avoid classical style but still use prototypes for performance or manageability...

var Child = {
  // defaults values for inherited properties
  id: 0,
  firstname = "",
  moneyCurrent = 0,
  moneyDue = 0,
  missionsPlay = 0,
  missionsWait = 0,
  missionsStop = 0,
  missions = [],

  // factory method which creates new instances and prototype factories
  create: function(options) {
    var obj = Object.create(this),
        proto = obj,
        inits = [];

    // fetch all defined init methods in the prototype chain
    while (proto && proto !== Object.prototype) {
      if (proto.hasOwnProperty("init")) inits.push(proto.init);
      proto = proto.getPrototypeOf();
    }

    // execute inits
    inits.reverse.forEach(function(init) {init.call(this, options);});
  },

  // constructor for your "class"
  init: function(options) {
    // validate input
    var money = parseFloat(options.already_paid) + parseFloat(options.to_pay)
            - parseFloat(options.spent_money),
        due = parseFloat(options.to_pay),
        play = parseInt(options.nb_missions.in_progress),
        wait = parseInt(options.nb_missions.waiting),
        stop = parseInt(options.nb_missions.completed);

    // use validated input or fallback to inherited values
    this.id = options.id_child || this.id;
    this.firstname = options.firstname || this.firstname;
    this.moneyCurrent = isNaN(money) ? this.moneyCurrent : money;
    this.moneyDue = isNaN(due) ? this.moneyDue : due;
    this.missionsPlay = isNaN(play) ? this.missionsPlay : play;
    this.missionsWait = isNaN(wait) ? this.missionsWait : wait;
    this.missionsStop = isNaN(stop) ? this.missionsStop : stop;

    // ensure we have our own copy of the missions array
    this.missions = this.missions.slice(0);
  },

  getMissions: function(type, callback) {
    // ...
  }      
};

So now creating a new child with Child.create will setup the appropriate prototype chain. You could then create a manageable inheritance hierarchy.

var c, FooChild = Child.create();

c = FooChild.create();
FooChild.foo = "FOO!";
assert(c.foo == "FOO!");
share|improve this answer
    
Really interesting –  Anc Ainu Apr 10 at 7:25
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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