Singleton model for modifying future .chart elements

Question

I have written a singleton module which is to be used for some future modifications on .chart elements. Modification functions, such as colorArgumentButtons() are called recursively so that I don't have to iterate over all .chart elements in all functions.

Is my use of _ for private "class vars" proper? Otherwise they may conflict with my local vars.

Am I using the revealing module pattern correctly and is my namespace implementation okay? Feel free to comment on bad design choices!

var SuperHappy = (function (_parent, $) {

    var _my = _parent.Chart = _parent.Chart || {};

    // default settings
    var _settings = {
        autoFind: true,
        chartSelector: '.chart'
    };

    // jQuery vars
    var _$charts = $();

    _my.init = function (options) {
        /// <summary>
        /// Initializes the module.
        /// </summary>
        /// <param name="options">Custom JSON options</param>

        _settings = $.extend(_settings, options);

        if (_settings.autoFind) {
            _my.findCharts();
        }
    };

    var _applyFunctionToAll = function (func, $charts) {
        /// <summary>
        /// Applies the supplied function for each chart element in the collection.
        /// </summary>
        /// <param name="func">Function to be applied</param>
        /// <param name="$charts">Optional chart collection to apply function on</param>

        if (typeof $charts === 'undefined' && _$charts !== $()) {
            $charts = _$charts;
        }

        if ($charts.length === 1) {
            return false;
        }

        $charts.each(function () { // can this be converted to a for loop?
            func($(this));
        });

        return true;
    };

    _my.colorArgumentButtons = function (color, $chart) {
        /// <summary>
        /// Colors all argument buttons.
        /// </summary>
        /// <param name="color">Color to be used</param>
        /// <param name="$chart"></param>

        if (_applyFunctionToAll(_my.colorArgumentButtons.bind(null, color), $chart)) {
            return;
        }

        $chart.find('button').css('background-color', color);
    };

    _my.findCharts = function (selector) {
        /// <summary>
        /// Finds all chart elements.
        /// </summary>
        /// <param name="selector">Optional CSS selector for finding chart elements</param>

        selector = typeof selector !== 'undefined' ? selector : _settings.chartSelector;
        _$charts = $(selector);
    };

    return _parent;

})(SuperHappy || {}, jQuery);

jQuery(document).ready(function ($) {
    SuperHappy.Chart.init();
    SuperHappy.Chart.colorArgumentButtons('#f00');
});

http://jsfiddle.net/JrwRN/

Answer 1

I think you are overdoing the underscores, you will find that conflicts will be rare.

Furthermore, I can understand that you want to centralize the iteration over the chart elements, but why would you want to do that recursively, it does not make much sense to me.

If you drop underscores and opt out of recursive iteration, your iterator becomes simpler:

  //Applies the supplied function for each chart element in the collection.
  var applyFunction = function ( f )
  {
    $charts.each(function (){ f($(this)); });
  };

This could be converted to a for loop as per your comment

  //Applies the supplied function for each chart element in the collection.
  var applyFunction = function ( f )
  {
    for( var i = 0, length = $charts.length; i < length ; i++ ){
      f( $charts[i] );
    }
  };

Then, colorArgumentButtons becomes simpler as well:

  // Colors all argument buttons.
  my.colorArgumentButtons = function (color)
  {
    applyFunction( function( $chart )
    {
      $chart.find('button').css('background-color', color);
    });
  };

You will notice that I reduced the comments to a one line comment. When you have almost as much lines of comment as code, then you are doing it wrong.

A minor side note on

selector = typeof selector !== 'undefined' ? selector : settings.chartSelector;

this could simply be

selector = selector || settings.chartSelector;

All in all, I refactored the code into this ( untested ):

var SuperHappy = (function (parent, $) {

  var my = parent.Chart = parent.Chart || {},
      // default settings
      settings = {
        autoFind: true,
        chartSelector: '.chart'
      },
      // jQuery vars
      $charts = $();

  //Applies the supplied function for each chart element in the collection.
  var applyFunction = function ( f )
  {
    $charts.each(function (){ func($(this)); });
  };

  my.init = function (options) 
  {
    settings = $.extend(settings, options);
    if (settings.autoFind) {
      my.findCharts();
    }
  };

  /// Colors all argument buttons.
  my.colorArgumentButtons = function (color)
  {
    applyFunction( function( $chart )
    {
      $chart.find('button').css('background-color', color);
    });
  };

  // Finds all chart elements.
  my.findCharts = function (selector)
  {
    selector = selector || settings.chartSelector;
    $charts = $(selector);
  };

  return parent;

})(SuperHappy || {}, jQuery);

jQuery(document).ready(function ($) {
    SuperHappy.Chart.init();
    SuperHappy.Chart.colorArgumentButtons('#f00');
});