1

I am new to Javascript and object-oriented programming in general. I would like to know if I am following the best practices when writing JS OOP code.

Here I made a class called _name and gave it some properties as well as an object this.details. Then I use prototyping to create methods for the class.

//define _name class (I use _ to easily recognize classes)
function _name () {
    this.firstName = '';
    this.lastName = '';
    this.middleName = '';
    this.details = {
        eyeColor: '',
        hairColor: ''
    }
}

//begin _name methods

_name.prototype.getFullName = function() {
    return this.firstName + ' ' + this.middleName + ' ' + this.lastName;
}
_name.prototype.setFirstName = function(firstName) {
    if ($.trim(firstName).length && typeof firstName != 'not_defined') {
        this.firstName = firstName;
    } else {
        alert('Please enter a valid first name.');
    }
}
_name.prototype.setLastName = function(lastName) {
    if ($.trim(lastName).length && typeof lastName != 'not_defined') {
        this.lastName = lastName;
    } else {
        alert('Please enter a valid last name.');
    }
}
_name.prototype.setMiddleName = function(middleName) {
    if ($.trim(middleName).length && typeof middleName != 'not_defined') {
        this.middleName = middleName;
    } else {
        alert('Please enter a valid middle name.');
    }
}
_name.prototype.setHairColor = function(hairColor) {
    this.details.hairColor = hairColor;
}
_name.prototype.setEyeColor = function(eyeColor) {
    this.details.eyeColor = eyeColor;
}

//end _name methods

var personOne = new _name();
personOne.setFirstName('John');
personOne.setLastName('Doe');
personOne.setMiddleName('Barry');
personOne.setEyeColor('Brown');
personOne.setHairColor('Black');
document.write(personOne.getFullName());
document.write(personOne.details.eyeColor);
document.write(personOne.details.hairColor);
Etcher
  • 92
  • 2
  • 9
  • 1
    I think you meant to put "`undefined`" at each occurrence of "`not_defined`" ... – David G Jun 28 '12 at 23:15
  • 3
    just a comment, usually constructor functions are capitalized, that's how you know they are constructors for objects so _name would be Name – Jaime Jun 28 '12 at 23:17
  • 1
    There are some general principles you may want to adhere to. For one, it might be considered a "code smell" to have `alert()` statements in the setter functions. Also where you test for `'not_defined`', as David said, you probably mean `'undefined'`—but you should also be doing the `undefined` check *before* you check the trimmed length with `$.trim(blah).length`. – Asherah Jun 28 '12 at 23:17
  • @David I tried `undefined` but it didn't seem to work. In Inspect Element the property value was `not_defined` when I used `typeof` – Etcher Jun 28 '12 at 23:18
  • @Len Yeah I just used `alert()` for an easy way to test the code. – Etcher Jun 28 '12 at 23:19
  • @Etcher What browser are you using? – David G Jun 28 '12 at 23:19
  • @David Was using Chrome. – Etcher Jun 28 '12 at 23:20
  • 1
    @Etcher: `'not_defined'` is definitely not kosher ಠ_ಠ – Asherah Jun 28 '12 at 23:20
  • @Etcher: *"I tried `undefined` but it didn't seem to work. In Inspect Element the property value was `not_defined` when I used `typeof`"* That's very, very, very strange. There is no built-in JavaScript thing called `not_defined`. There **is** a built-in JavaScript thing called `undefined`, which has the type (via `typeof`) of `"undefined"`. – T.J. Crowder Jun 28 '12 at 23:26
  • @T.J.Crowder Yeah something else must be the culprit. I replaced `not_defined` with `undefined` and removed the `$.trim(blah).length` check but it is still not triggering the `alert()` – Etcher Jun 28 '12 at 23:38
  • @Etcher: That's a different question (feel free to post it, with examples of the call). Your question here is about OOP, not the vagaries of JavaScript type coercion. :-) – T.J. Crowder Jun 28 '12 at 23:50

2 Answers2

3

In the big picture, yes, you're doing fine; nice one. :-) Consider the examples in this other answer on SO for more information and perspective.

Some pointers, none of which is OOP-related, just JavaScript-related:

  1. The overwhelming convention in JavaScript is that constructor functions (your _name) are initially-capped, e.g. Name (like Date or RegExp).

  2. Your various functions assigned in the form _name.prototype.setLastName = function() { ... } are anonymous functions assigned to properties that have names. Giving your functions proper names helps your tools help you. Some engines are smart enough to figure it out even when you don't give your functions names, but others need proper names. See the link above for examples, and/or Anonymouses anonymous.

  3. You're relying on the horror that is automatic semicolon insertion in all of your function assignments: _name.prototype.setLastName = function() { ... } should end with a semicolon after the }. Recommend learning the rules and always supplying the semicolons explicitly; when the engine has to guess, it can guess wrong, and it makes minification/compression/packing difficult when you leave them out.

  4. Rather than writing _name.prototype.xyz = ... all over the place, consider using a scoping function and caching _name.prototype to a simpler symbol, e.g.:

    (function(p) {
        p.setLastName = function() {
            // ...
        };
    
        p.xyz = function() {
            // ...
        };
    })(_name.prototype);
    

    ...or using a helper function that goes further than that. Just to cut down on keystrokes and script size.

But again, by and large, yes, you're on the right track.

More reading:

Community
  • 1
  • 1
T.J. Crowder
  • 1,031,962
  • 187
  • 1,923
  • 1,875
0

Some comments:

I use _ to easily recognize classes

The standard is to use Capitalized names for constructor functions. The underscore is used to imply privacy of variables or properties - i.e. they are public, but should not be used.

var personOne = new _name();

personOne.setFirstName('John');

personOne.set...

Usually you'd use arguments to the constructor:

function Name (first, last, middle, eye, hair) {
    this.firstName = first;
    this.lastName = last;
    this.middleName = middle;
    this.details = {
        eyeColor: eye,
        hairColor: hair
    }
}

var personOne = new Name('John', 'Doe', 'Barry', 'Brown', 'Black');

The setters will usually be called only once when initialising, so you can move their code into the construtor function. If you plan to make the properties changeable, you still could call the setter function with the validation logic from the constructor.

typeof middleName != 'not_defined'

The result of the typeof operator for value-less variables is "undefined". You also should will need to check that before trimming.

There also might be some flaws in your general code design. You use the same condition for first, middle and last name, you should really move them into a own validateString function which can be called from the single setters (DRY code). As well, a validation function should not give the user the feedback about what to enter, it should just return false for "not valid". The user interaction should be done by the module that also handles your input - not in the data structuring module.

Bergi
  • 630,263
  • 148
  • 957
  • 1,375