Binding events in constructor

Question

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 = $('<div class="person">' +
                          '<p class="name">' + data.name + '</p>' +
                          '<p class="age">' + data.age + '</p>' +
                      '</div>');


    // 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/.

Answer 1

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 = $(
        '<div class="person"><p class="name"></p><p class="age"></p></div>'
    );

    // 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 < persons.length; i++) {
    persons[i].render($('#person-container'));
}

.person {
    border: 1px solid black;
    margin-bottom: 5px;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.0/jquery.min.js"></script>
<div id="person-container"></div>