current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 4 down vote favorite

Is this a good way to create a class with a constructor and namespace in object literal style?

// root namespace
var myApp = myApp || {};

// sub namespace
myApp.model = {
    // Constructor
    Person: function (name) {
        this.name = name;
    }
};


myApp.model.Person.prototype = {
    sayName: function () {
        alert(this.name);
    },
    sayHi: function () {
        alert("Hi, " + this.name);
    }
};

var p1 = new myApp.model.Person("CK");
p1.sayName();
p1.sayHi();

JSHint shows no error and the full source code is here.

share|improve this question
    
I may be incorrect, but wouldn't your myApp.model.Person.prototype only add those functions to the Person objects that are a part of your myApp.model, and not all Person objects? If that is intended, that's fine. I could just see that becoming an issue later on. Do you have a separate Person object? –  krillgar Sep 9 at 13:05
    
I am from Java background. I want to make myApp.model as my package, the fully qualified class name I want to make is myApp.model.Person. This class shall have a constructor that take String name as parameter, and have 2 methods sayName and sayHi. It is simple to be achieved in Java, but I am not sure how to make it right in Javascript. –  CK Lee Sep 9 at 13:15
    
@krillgar, there is no other Person object. Why it could be an issue if I define my qualified class name as myApp.model.Person? –  CK Lee Sep 9 at 13:59
1  
@krillgar: if you make a 2nd instance of myApp.model.Person (with different constructor arg), it'll work ok: jsfiddle.net/vj4ujz0L/3 –  Shivan Dragon Sep 9 at 16:25
1  
@krillgar You needn't be concerned. JavaScript doesn't care where the Person constructor is stored, whether it's myApp.model.Person or anywhere else. All Person instances which share that constructor will have access to the functions attached to the constructor's prototype. –  Cory 22 hours ago

3 Answers 3

up vote 3 down vote accepted

This looks good to me. From a once over;

  • Consider using 'use strict'
  • You cannot have 'private' functions or properties, that should be fine
  • Naming is fine. As I personally dislike namespaces I would probably go for a 1 character namespace for myApp.
  • Indenting is fine
  • 0 comments, you might want to consider one-liner comments separating the different sections of your code.
  • Somewhat to krillgar's point, instanceof checks will have to check against myApp.model.Person with your approach, this should be fine as well

All in all, JavaScript is not Java. I would not go so far as to say that namespaces are not idiomatic, but I would invite you read this.

share|improve this answer
    
If the listed code is under Person.js file, I would like to add another class myApp.model.Facility at another Facility.js file. How should I define the sub namespace block? This may worth for another question, but hope you can answer as well. –  CK Lee Sep 9 at 13:32
1  
@CK by combining your first line with my answer, ie myApp.model = myApp.model || {}; followed by myApp.model.Person = /*...*/ rather than myApp.model = { Person /*...*/ –  James Thorpe Sep 9 at 13:36
    
I disagree re: comments - comments should only be added to code that is hard to understand without the comment. Assuming that this code will be read by people who know javascript, no comments are needed in this code. –  TV's Frank Sep 9 at 14:48
    
@TV'sFrank It's an opinion, I would invite you to look at CK Lee's answer, the comments make it more readable in my mind. –  konijn Sep 9 at 14:54
    
@CKLee You should look up "javascript module pattern". You'll find a pattern for isolating your new variables from the global namespace, and within that context you can safely alias your namespaces to something convenient like this: var Person = myApp.model.Person. –  Cory 20 hours ago
up vote 4 down vote

I would suggest splitting the addition of the methods as follows:

myApp.model.Person.prototype.sayName = function () {
    alert(this.name);
};

myApp.model.Person.prototype.sayHi = function () {
    alert("Hi, " + this.name);
};

This way you can define additional methods in different places (if needed), as you're not replacing the entire prototype in one go.

share|improve this answer
    
In Java, normally we define all methods under public class Person { ... } block in a same file. Should it be good to put all related methods in the same place in Javascript as well? –  CK Lee Sep 9 at 14:03
    
Yes, it would be normal to group them together for ease of maintenance, but nothing actually stopping you in the language from putting them wherever you want –  James Thorpe Sep 9 at 14:48
1  
I might be wrong, but I don't understand how the OP's solution doesn't allow you to later on add extra functions to the prototype (something like this: jsfiddle.net/vj4ujz0L/6) –  Shivan Dragon Sep 9 at 16:30
1  
ShivanDragon is right. But I'd still recommend following James Thorpe's advice and adding your new methods to the prototype that's already there, since that approach works nicely with inheritance models. Oh, and please never add methods to a single prototype at multiple different places (unless your project is very small or you have a very clear set of conventions to which everyone agrees). –  Cory Sep 9 at 17:48
    
@ShivanDragon Yes, you can add them to the prototype, so long as you do so after you've already done the original assignment. If you add each method individually, it can make refactoring easier as you don't need to ensure they're added after the initial assignment –  James Thorpe Sep 10 at 8:38
up vote 2 down vote

Based on comment from konijin, here is the updated version

// root namespace
var myApp = myApp || {};

// sub namespace
myApp.model = myApp.model || {};

// constructor
myApp.model.Person = function (name) {
    this.name = name;
}

myApp.model.Person.prototype = {
    sayName: function () {
        alert(this.name);
    },
    sayHi: function () {
        alert("Hi, " + this.name);
    }
};

var p1 = new myApp.model.Person("CK");
p1.sayName();
p1.sayHi();

alert(p1 instanceof myApp.model.Person); // true
alert(p1 instanceof Object); // true
alert(p1 instanceof Person); // Uncaught ReferenceError: Person is not defined

http://jsfiddle.net/cklee75/1d6jgx4k/3/

share|improve this answer

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.