jQuery notifications plugin

Question

Any suggestions on how I could improve it?

jsFiddle

(function($){
    $.extend({
        notify: function(options, duration) {
            var
            // Default options object
            defaults = {
                inline: false,
                href: '',
                html: '',
                onStart: function(){},
                onShow: function(){},
                onComplete: function(){},
                onCleanup: function(){},
                onClosed: function(){}
            },
            options = $.extend(defaults, options),

            // Elements & Clones
            notifications_active = ($('ul#notification_area').length ? true : false),
            container = (notifications_active ? $('ul#notification_area') : $('<ul>    </ul>').attr('id', 'notification_area')),
            wrapper = $('<li></li>').addClass('notification'),

            // Useful variables
            clone,
            get_clone,
            close,
            start_countdown,
            element,
            north,
            south,
            east,
            west,
            content,
            add_notification;

            options.onStart.call(this);

            if(!notifications_active){
                $('body').append(container);
            }

            get_clone = function(){
                if(options.href){
                    if(options.inline){
                        return $(options.href).clone();
                    }
                    else {
                    return $('<iframe></iframe>').attr('src',     options.href).css({width: '100%', height: '100%'});
                    }
                }
                else if(options.html){
                    return $(options.html);
                }
            }

            close = function(){
                options.onCleanup.call(this);
                wrapper.fadeOut('fast', function(){
                    $(this).remove();
                    options.onClosed();
                })
            }

            start_countdown = function(){
                setTimeout(function(){
                    close();
                }, duration);         
            }

            element = function(tag, cl, id){
                var el = document.createElement(tag);

                if(cl){
                    el.className = cl;
                }

                if(id){
                    el.id = id;
                }

                return $(el);
            }

            add_notification = function(){
                wrapper.append(
                    element('div', 'notify_top').append(
                        element('div', "notify_nw"),
                        north = element('div', "notify_n"),
                        element('div', "notify_ne")
                    ),
                    element('div', 'notify_center').append(
                       east = element('div', "notify_w"),
                        content = element('div', 'notify_content').append(clone),
                        west = element('div', "notify_e")
                    ),
                    element('div', 'notify_bottom').append(
                        element('div', "notify_se"),
                        south = element('div', "notify_s"),
                        element('div', "notify_sw")
                    )
                );

                wrapper.css("visibility", "hidden").appendTo(container);

                if(options.close){
                    var close_elem = $('<span></span>').addClass('cl').html(options.close);
                    content.append(close_elem);
                }

                var anim_length = 0 - parseInt(wrapper.outerHeight());
                wrapper.css('marginBottom', anim_length);
                wrapper_height = wrapper.height();

                north.width(parseInt(wrapper.width())-40);
                south.width(parseInt(wrapper.width())-40);
                east.height(parseInt(content.height()));
                west.height(parseInt(content.height()));

                options.onShow.call(this);

                wrapper.animate({marginBottom: 0}, 'fast', function(){
                    wrapper.hide().css('visibility', 'visible').fadeIn('fast');
                    if(duration){
                        start_countdown();
                    }

                    if(!options.close){
                        wrapper.bind('click', function(){
                            close();
                        })
                    }
                    else {
                        close_elem.bind('click', function(){
                            close();
                        })
                    }

                    options.onComplete.call(this);
                });
            }

            clone = get_clone();
            add_notification();   
        }
    });
})(jQuery);

Answer 1

Some suggestions:

• rename onClosed to onClose

• don't declare that group of useful variables at once, declare them as you go. Or, declare once but use a var statement. Some of them are used only in add_notification function, so move them there.

• break these lines

  container = (notifications_active ? $('ul#notification_area') : $('<ul></ul>').attr('id', 'notification_area'));
  ...
  if(!notifications_active){
      $('body').append(container);
  }
  

into these (you remove one conditional statement and it's easier to follow):

if (notifications_active) {
    container = $('ul#notification_area');
} else {
    container = $('<ul></ul>').attr('id', 'notification_area'));
    $('body').append(container);
}

• rename the function element to something more explicit and simplify it. You never use the id argument. It could be something like:

  createElement = function(tag, cl){
      return $(document.createElement(tag)).addClass(cl);
  }
  

you can use this helper in other places where you create elements, for example in this block:

if(options.close){
    var close_elem = $('<span></span>').addClass('cl').html(options.close);
    content.append(close_elem);
}

• parseInt should always be used with the radix, parseInt(999, 10) for decimal numbers

• remove this useless line

  wrapper_height = wrapper.height();
  

• it seems you don't pass a close option, so all lines that check options.close are useless or don't do what you expect. If users can provide this value (for instance, if you mention in the documentation, they could try), then set a default value.

• these last lines are obscure

  clone = get_clone();
  add_notification();   
  

it turns out that add_notification uses clone, so it would be better to pass it as a parameter to make it explicit. As a general rule, it's better to pass parameters than putting everything in the same scope and rely on them having the expected value all the time. You could have passed parameters to start_countdown, get_clone and they would be more generic functions.

• a more subjective topic, you use underscore to separate words in variables, but the options you accept are in camel case, I would prefer using the same convention for both (and would prefer camel case).