Simple JS/jQuery notification system

Question

I'm new to front-end development and now I'm learning JavaScript with jQuery. I couldn't understand JavaScript's classes and objects and jQuery plugins, but after reading some articles and examples I wrote simple notifications system.

(function ($) {
    var notification = function (title, msg, duration) {
        var self = this;

        this.hide = function () {
            self.element.fadeOut(1000, function () {
                self.element.remove();
            })
        }

        this.element = $('<div class="notification"><h3>' + title + '</h3><div class="notification_body">' + msg + '<br/><i>(double click to hide)</i></div></div>');
        this.element.dblclick(self.hide);

        if (duration !== undefined) {
            setTimeout(function () {
                self.hide();
            }, duration * 1000);
        }

        $('#notifications_bar').prepend(this.element.hide().fadeIn(1000));
    };
    $.extend({
        notify:function (title, msg, duration) {
            return new notification(title, msg, duration);
        }
    });

Example:

$(function() {
    $.notify('Title', 'Message'); // sticky note
    $.notify('Title', 'Message', 2); // 2-seconds notification
});

The most terrible were closures and this keyword. I couldn't understand how to use them, so I want to know if there in my code above 'bad-practice' parts.

Answer 1

First of, for someone learning jQuery and JavaScript, you've done a great job of avoiding the pitfalls of most new developers to JavaScript;

1. You're not passing strings to setTimeout, which a lot of people do.
2. You've got a good grasp on closures (no matter how long they took you to learn :)).
3. You're using strict equals (===) rather than equals.

... so, if you want me to be really, really picky;

1. I don't see much point in adding your code to the jQuery namespace. It would work just as well to be added to your own namespace. A lot of people fall into the habit of defining everything on $, and are scared of declaring your own namespace; don;t be:

   var ME = {};
   
   ME.notify = function (title, msg, duration) {
       return new notification(title, msg, duration);
   };
   

2. It's a code convention to use a capital letter for constructors (e.g. functions you need to call new on); change function notification to function Notification.

3. You could make use of prototypical inheritance. As it stands, you're defining and adding a hide function on every instance of a notification you create. Obviously this has a negligible memory impact. Instead, use prototypical inheritance, and declare the function once;

   function Notification (blah, blah, blah) {
      // blah blah blah
   } 
   
   Notification.prototype.hide = function () {
       var self = this;
   
       this.element.fadeOut(1000, function () {
           self.element.remove();
       });
   }
   

   ... you'd then have to change your double click handler to use the anonymous function approach I recommended in my answer, as this will no longer be the correct this.

4. You've got a potential XSS exploit. For example, given the message:

   <script>alert(document.cookie);</script>
   

   This will be injected, and evaluated in your page. You can fix this by setting the h3 and p using text() explicitly;

   this.element = $('<div class="notification"><h3></h3><div class="notification_body"><span class="text"></span><br/><i>(double click to hide)</i></div></div>');
   
   this.element.find('h3').text(title);
   this.element.find('span.text').text(message);
   

... but again, I want to re-iterate that I'm being very, very picky. What you've got is well written, functioning JavaScript.