HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Binding events in constructor

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
eventsconstructorbinding

Problem

I am trying to get familiar with OOP JavaScript, in particular working with the prototype pattern and would love some pointers/suggestions on how to improve my code.

I think I have understood the basics well, and have been able create class instances successfully.

I am however, a little unsure on how to work with events when it comes to representing these instances with a UI. Until now, I have been binding my events in the constructor before the element gets added to the DOM, but am sure this could be improved:

function Person(data) {
    var self = this;
    self.data = data;
    self.$element = $('' +
                          '' + data.name + '' +
                          '' + data.age + '' +
                      '');

    // bind events
    self.$element.find('.name').click(function () {
        self.speakName();
    });

    self.$element.find('.age').click(function () {
        self.speakAge();
    });

    // render element
    $('.person-container').append(self.$element);
}

Person.prototype.speakName = function () {
    alert(this.data.name);
};

Person.prototype.speakAge = function () {
    alert(this.data.age);
};


Ideally, I'd like to be able to build up a relatively complex UI of 'people', with various controls on each person to perform actions related to that person only. Apologies for the contrived example, I'm just trying to get the concept right in my head before doing anything more real-world.

I've also included a working jsfiddle incase this is easier to work with: http://jsfiddle.net/z59q2rab/.

Solution

It feels like you have muddled the model, view, and controller. With a name like Person, I would expect this object to act as a model. Constructing a person shouldn't also make its UI. At the least, make the UI rendering a separate explicit step.

A second potential problem is that concatenating strings into HTML without escaping can lead to JavaScript injection vulnerabilities (also known as cross-site scripting). I recommend using $.text() to fill in the text.



function Person(data) {
this.name = data.name;
this.age = data.age;
}

Person.prototype.speakName = function() {
alert(this.name);
};

Person.prototype.speakAge = function() {
alert(this.age);
};

Person.prototype.render = function($container) {
var $element = $(
''
);

// Fill text and bind events
var self = this;
$element.find('.name')
.text(self.name)
.click(function () {
self.speakName();
});

$element.find('.age')
.text(self.age)
.click(function () {
self.speakAge();
});

// render element
$container.append($element);
};

//////////////////////////////////////////////////////////////////////

var persons = [
new Person({
name: 'Tom',
age: 30
}),
new Person({
name: 'Dick',
age: 25
}),
new Person({
name: 'Harry',
age: 45
})
];

for (var i = 0; i
.person {
border: 1px solid black;
margin-bottom: 5px;
}

`

Context

StackExchange Code Review Q#80686, answer score: 4

Revisions (0)

No revisions yet.