Is this jQuery Plugin for TreeView correct?

Question

Looking for a review of my first published jQuery Plugin. It's for TreeViews, very basic example demo can be seen at: Demo

Everything works fine, just looking to see if I can get some feedback on things that I may be doing wrong best practices wise and/or things I could be doing better.

(function ( $ ) {

  var methods = {
    init : function(options) {

      // Default Settings
      var settings = $.extend({
          'expandIconClass' : 'closed',
          'contractIconClass' : 'open',
          'setFocus' : undefined,
          'classPrefix' : 'goodtree_',
        }, options);

      return this.each(function() {

        // Hide all of the children Elements
        $(this).find('ul').hide();

        // Add the plus minus buttons
        $(this).find('li').each(function() {
          if($(this).children('ul').length > 0)
            $(this).prepend($('<div />', {'class': settings.classPrefix + "toggle " + settings.expandIconClass}));
        });

        // Events
        $('.' + settings.classPrefix + 'toggle').click(function() {
          $(this).parent().children('ul').toggle();
          $(this).hasClass('open') 
            ? $(this).removeClass(settings.contractIconClass).addClass(settings.expandIconClass) 
            : $(this).removeClass(settings.expandIconClass).addClass(settings.contractIconClass);
        });

        if(undefined !== settings.setFocus)
        {
          $(this).goodtree('setFocus', settings.setFocus);
        }

      });
    },

    setFocus : function(obj) {
      return this.each(function() {
        var tree_parent = this;
        $(obj).parents('ul').each(function() {
          if($(this) === this)
            return;
          else
            $(this).show();
        });
      });
    }
  }

  $.fn.goodtree = function( method ) {
    if ( methods[method] ) {
        return methods[ method ].apply( this, Array.prototype.slice.call( arguments, 1 ));
      } else if ( typeof method === 'object' || ! method ) {
        return methods.init.apply( this, arguments );
      } else {
        $.error( 'Method ' +  method + ' does not exist on jQuery.goodtree' );
      }
  };
}) ( jQuery );

Answer 1

First off it'd be good to run the code through jslint or jshint. It identifies a few issues, such as an extra comma in the default options list; that'll break the code in older IE versions. Also, it flags some things like the block-less if, which - while allowed - it's best to avoid for the sake of consistency and maintainability.

As for the code itself, here are the things I noticed:

• jQuery objects should be cached whenever possible
• The code only handles unordered lists (ULs), not ordered lists (OL) although it might as well
• The event handling is added document-wide every time the code is run. If you use the plugin twice in a document, the first tree gets double event handlers. That spells trouble.
• The setFocus function might misbehave if the tree list is itself inside a list element, as it loops through all list-type parents up to the document root.
• The classPrefix option is confusing: It's not used for the expand/contract classes, only for the toggle button's "main" class. But since that's the only one, why not just let the user define that as well? Basically, if a prefix is used only once, it's not necessary.
• Comparing against undefined is a no-no. undefined is not a reserved word, and can (in some runtimes) be defined. In this case, a simple boolean works just as well.

Here's what I arrived at

(function ($) {
  var methods = {
    init: function(options) {
      // Default Settings
      var settings = $.extend({
        expandIconClass:   'closed',
        contractIconClass: 'open',
        toggleButtonClass: 'toggle',
        setFocus:          false      // just use a boolean here
      }, options);

      return this.each(function() {
        var target = $(this);

        // walk the tree
        target.find('li').each(function() {
          var node = $(this),
              branches = node.children('ul, ol'),
              button;

          if(branches.length > 0) {
            branches.hide();
            button = $('<div />', {
              'class': settings.toggleButtonClass + " " + settings.expandIconClass,
              on: {
                click: function (event) {
                  // we already have the correct elements here
                  branches.toggle();
                  button.toggleClass(settings.expandIconClass + " " + settings.contractIconClass);
                }
              }
            });

            node.prepend(button);
          }
        });

        if(settings.setFocus === true) {
          target.goodtree('setFocus');
        }
      });
    },

    setFocus: function(element) {
      return this.each(function() {
        $(element).parents('ul, ol').each(function() {
          var ancestor = $(this);
          if( this.is(ancestor) ) { // better check
            return false; // stop the each-loop
          }
          ancestor.show();
        });
      });
    }
  };

  $.fn.goodtree = function(method) {
    if(typeof methods[method] === 'function') { // stronger conditional
      return methods[method].apply(this, Array.prototype.slice.call(arguments, 1));
    } else if(typeof method === 'object' || !method) {
      return methods.init.apply(this, arguments);
    } else {
      $.error('Method ' +  method + ' does not exist on jQuery.goodtree');
    }
  };
}(jQuery));

Here's a demo (I reused your demo markup and styling, minus the open/close icons)