CODEWITHSUNDEEP
javascript
user3361761
user3361761

Reputation: 259

Javascript class methods returning 'undefined'

there is probably a quick answer to this question however, I cannot figure out what I am doing wrong. I have a class EmailList:

var EmailList = function(id, name, expire_date)
{
    var instance = this;
    var list_id = id;
    var list_name = name;
    var expire_date = expire_date;
    var is_editing = false;

    this.get_list_id = function() { return instance.list_id; }
    this.get_list_name = function() { return instance.list_name; }
    this.get_expire_date = function() { return instance.expire_date; }
    this.is_editing = function() { return instance.is_editing; }
}

I am confused as when I call any of the methods on the class such as:

console.log(list.get_list_name());
console.log(list.get_list_id());

I get 'undefined' as a result. What's going on here?

Upvotes: 0

Views: 102

Answers (2)

JLRishe
JLRishe

Reputation: 101662

Both examples in Ted Hopp's answer will get your code to work, but it looks like you may be trying to make accessors for private fields (or the JavaScript equivalent of private fields). If so, you may be better off doing this:

var EmailList = function(id, name, expire_date)
{
    var list_id = id;
    var list_name = name;
    // Clone input date so it can't be modified externally
    var expire_date = cloneDate(expireDate);
    var is_editing = false;

    this.get_list_id = function() { return list_id; }
    this.get_list_name = function() { return list_name; }
    // Clone date so it can't be modified externally
    this.get_expire_date = function() { return cloneDate(expire_date); }
    this.is_editing = function() { return is_editing; }

    function cloneDate(date) {
        return date && date.getTime && new Date(date.getTime());
    }
}

Or you can do this, and skip three of the extra variable declarations altogether:

var EmailList = function(id, name, expire_date)
{
    var is_editing = false;
    // Clone input date so it can't get modified externally
    expire_date = cloneDate(expire_date);

    this.get_list_id = function() { return id; }
    this.get_list_name = function() { return name; }
    // Clone date so it can't get modified externally
    this.get_expire_date = function() { return cloneDate(expire_date); }
    this.is_editing = function() { return is_editing; }

    function cloneDate(date) {
        return date && date.getTime && new Date(date.getTime());
    }
}

This will ensure that list_id, list_name, expire_date and is_editing cannot be modified from outside your objects.

Yet another alternative available in EcmaScript 5 is to create true readonly properties, that can't be modified by anyone:

var EmailList = function(id, name, expire_date) {
    Object.defineProperties(this, {
        list_id: { value: id, writable: false },
        list_name: { value: name, writable: false },
        expire_date: { value: cloneDate(expire_date), writable: false },
        is_editing: { value: false, writable: false });

    function cloneDate(date) {
        return date && date.getTime && new Date(date.getTime());
    }
}

I recognize that it probably doesn't make sense to make is_editing completely readonly, but hopefully this gives you some good ideas.

Upvotes: 1

Ted Hopp
Ted Hopp

Reputation: 234795

You aren't initializing the instance data. Instead of declaring local variables to store the arguments, try this instead:

var EmailList = function(id, name, expire_date)
{
    var instance = this;
    instance.list_id = id;
    instance.list_name = name;
    instance.expire_date = expire_date;
    instance.is_editing = false;

    this.get_list_id = function() { return instance.list_id; }
    this.get_list_name = function() { return instance.list_name; }
    this.get_expire_date = function() { return instance.expire_date; }
    this.is_editing = function() { return instance.is_editing; }
}

A better approach, by the way, is to eliminate the instance variable, change the instance methods to use this and use the prototype to define the methods:

var EmailList = function(id, name, expire_date)
{
    this.list_id = id;
    this.list_name = name;
    this.expire_date = expire_date;
    this.is_editing = false;
};

EmailList.prototype = {
    get_list_id : function() { return this.list_id; },
    get_list_name : function() { return this.list_name; },
    get_expire_date : function() { return this.expire_date; },
    is_editing : function() { return this.is_editing; }
};

That way you aren't creating separate member function objects for each instance of EmailList.

Upvotes: 4

Related Questions