1

I have an Object with a getter and setter where I'm intending to protect a property so that it can only be updated through a method. Let's call this object Person which has the following structure:

function Person(data) {
    var _name = data.name;

    this.name = function () {
        return _name;
    };
    this.setName = data.setName || function (newValue) {
        _name = newValue;
    };
}

I want to be able to override setName and pass different implementations to each instance of Person but I can't quite seem to get it to work. I'm certain this is a closure issue, but I just can't get my head around it.

Given the following usage scenario, what am I doing wrong?

var p1 = new Person({
    name: "Paul",
    setName: function (newValue) {
        this._name = newValue;
    }
});
var p2 = new Person({ name: "Bob" });
p1.setName("Paul (updated)");
p2.setName("Bob (updated)");

p1 never updates its value and so is always "Paul" whereas p2 does and becomes "Bob (updated)". I want to be able to create as many unique instances of Person as I want, where some of them have their own implementation of setName and others will just use the default instance.

I've tried wrapping data.setName up in a closure like this and setting my custom setName to return the value instead:

this.setName = data.setName ? function () {
    return (function (value) {
        value = data.setName();
    }(_name));
} : function (newValue) { _name = newValue; }

But I'm having no luck - I obviously just don't get closures as well as I thought! Any and all help always appreciated.

codepen.io example here

Community
  • 1
  • 1
Paul Aldred-Bann
  • 5,840
  • 4
  • 36
  • 55

4 Answers4

2

I feel this is the simplest solution so far, using bind:

function Person(data) {
    var _data = {name:data.name};

    this.name = function () {
        return _data.name;
    };
    this.setName = data.setName ? data.setName.bind(_data) : function (newValue) {
        _data.name = newValue;
    };
}

var p1 = new Person({
  name: "Paul",
  setName: function (newName) {
      this.name = newName;
  }
});
var p2 = new Person({ name: "Bob" });
p1.setName("Paul (updated)");
p2.setName("Bob (updated)");

console.log(p1.name());
console.log(p2.name());

Instead of using a primitive to store the name we use an object, whose reference we can pass as this using bind. Originally, when you wrote this._name = newValue;, this meant something quite different from what you thought, namely the object you pass the intantiate the new Person.

demo

Mosho
  • 7,099
  • 3
  • 34
  • 51
  • @PaulAldred-Bann Although you've accepted an answer, I encourage you to look at this, I think it's elegant and quite close to what you had in mind in the first place, and uses, in my opinion, a very basic core feature of JavaScript effectively. – Mosho Apr 08 '14 at 10:59
  • @Mosho I missed this one! I like how the private variable has been changed to an object and passed as scope to the custom method. However, in practice (the real application) I need to pass the `Person` as scope in cases where I `$.extend` the value passed via the params. Hence, I'm calling @Bergi's solution using `.bind`. – Paul Aldred-Bann Apr 08 '14 at 11:09
2
this._name = newValue;

_name is a private variable, not a property. It cannot be accessed on this. Have a look at Javascript: Do I need to put this.var for every variable in an object? if you're not sure about the difference.

function (value) {
    value = data.setName();
}

value here is a local variable to that function's scope, not a "reference" to the _name variable you passed in. Setting it will only put a different value in that local variable, but not change anything else.


Possible solutions:

  • Pass a setter function that has access to the _name variable to the validator:

    function Person(data) {
        var _name = data.name;
    
        this.name = function () {
            return _name;
        };
        if (data.nameValidator) 
            this.setName = function(newValue) {
                _name = data.nameValidator(newValue, _name);
            };
        else
            this.setName = function (newValue) {
                _name = newValue;
            };
    }
    var p1 = new Person({
        name: "Paul",
        nameValidator: function (newValue, oldValue) {
            return (newValue /* ... */) ? newValue : oldValue;
        }
    });
    
  • Or let the validator return the value:

    function Person(data) {
        var _name = data.name;
    
        this.name = function () {
            return _name;
        };
        this.setName = function (newValue) {
            _name = newValue;
        };
        if (data.makeNameValidator) 
            this.setName = data.makeNameValidator(this.setName);
    }
    var p1 = new Person({
        name: "Paul",
        makeNameValidator: function (nameSetter) {
            return function(newValue) {
                if (newValue) // ...
                    nameSetter(newValue);
            };
        }
    });
    
  • Or make _name a property to which the validator can write. You could also put this on the original data object, for not exposing it on the Person interface:

    function Person(data) {
        this.name = function () {
            return data.name;
        };
        if (data.setName)
            this.setName = data.setName.bind(data);
        else
            this.setName = function(newValue) {
                _name = newValue;
            };
    }
    var p1 = new Person({
        name: "Paul",
        setName: function (newValue) {
            if (newValue) // ...
                this.name = newValue;
        }
    });
    
Community
  • 1
  • 1
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • I see now that I was hugely over-complicating my design (in my head!), this is now working and I've updated the codepen, thanks! – Paul Aldred-Bann Apr 08 '14 at 10:57
  • I wouldn't say any of my designs are not complicated :-) – Bergi Apr 08 '14 at 11:00
  • Haha very true, although what I meant was in my head I imagined replacing the entire method signature - parameters, return types the works, all within a closure... and then it hit, I was thinking WAY outside the box. – Paul Aldred-Bann Apr 08 '14 at 11:02
0

U think it's a problem with this

this.setName = function (newValue) {
  if (data.setName) {
    data.setName.call(this, newValue);
  } else {
    _name = newValue;
  }
}
Jakub Matczak
  • 15,341
  • 5
  • 46
  • 64
phylax
  • 2,034
  • 14
  • 13
  • So did I, but unfortunately this doesn't work. I've updated the codepen with this however. – Paul Aldred-Bann Apr 08 '14 at 10:43
  • Sorry, _name is not accessed via `this` so this is no solution. Its a clusore problem, because only function defined within the scope of `Person` have access to `_name`. – phylax Apr 08 '14 at 10:45
0

You are setting this._name in your p1 Person, whereas you are accessing the 'internal' variable _name in your Person() function, instead of this._name. This can easily be fixed by adding this. to parts in your Person() class.

function Person(data) {
    this._name = data.name;

    this.name = function () {
        return this._name;
    };
    this.setName = data.setName || function (newValue) {
        this._name = newValue;
    };
}

Your second example, with the wrapping, is not working due to some errors you appear to have made in the setName() function. Firstly, you are not passing anything to the data.setName() function, meaning that it is receiving undefined for the first parameter. Next up, you are setting value to the returned value, whereas you should instead be setting _name to the returned value. Try this instead:

this.setName = data.setName ? function (newValue) {
    return (function (value) {
        _name = data.setName();
    }(newValue));
} : function (newValue) { _name = newValue; }
tomb
  • 1,817
  • 4
  • 21
  • 40
  • Exposing `_name` is what I'm trying to avoid - I'm aware my current design may not be suitable for what I'm trying to achieve - but that's why I'm here :o) – Paul Aldred-Bann Apr 08 '14 at 10:52
  • Try using your second idea with the second example I provided, it should work then ;) – tomb Apr 08 '14 at 10:55