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 23 hours ago
    
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 23 hours ago
    
@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 23 hours ago
    
Because if you apply changes to the prototype of an instance of a JavaScript object, they only apply to that instance. To apply those two functions to EVERY instance of a Person object, you need to assign them to the root Person object definition. I'm not sure if this would count as an instance. Most likely, it'd be only to the Person objects that belong to myApp.model objects. So if you have Person somewhere else, then those objects won't have those functions on them. –  krillgar 22 hours ago
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 20 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 23 hours ago
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 23 hours ago
    
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 22 hours ago
    
@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 22 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 22 hours ago
    
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 22 hours ago
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 20 hours ago
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 19 hours ago
    
@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 4 hours ago
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.