3
\$\begingroup\$

I have the following JavaScript function:

function registerEvents() {
    var isTapholding = false;
    $(document).delegate('.add-control .add-image', 'tap', function (evt) {
        handleAddControlEvent(this);
    });
    $(document).delegate('.delete-image', 'tap', function (evt) {
        evt.stopPropagation();
        handleDeleteControlEvent(this);
    });
    $(document).delegate('.edit-control', 'tap', function (evt) {
        if (isTapholding)
            return;
        handleEditControlEvent(this);
    });
    $(document).delegate('.edit-control', 'vmouseup', function (evt) {
        setTimeout(function () {
            isTapholding = false;
        }, 100);
    });
    $(document).delegate('.add-control .insert-image', 'tap', function (evt) {
        handleInsertImageEvent(this);
    });
    setTimeout(function () {
        $(document).delegate('.edit-control', 'taphold', function (evt) {
            evt.stopPropagation();
            isTapholding = true;
            handleEditControlTapHoldEvent(this);
        });
    }, 100);
}

How I can make this DRY (Don't Repeat Yourself)?

\$\endgroup\$
7
\$\begingroup\$

You can refactor this to follow DRY by using the Strategy Pattern.

function registerEvents(){  
    var delegationStrategies = [
        {var1: '.add-control .add-image',
         var2: 'tap',
         func: function (evt) {
             handleAddControlEvent(this);
         }
        },
        {var1: '.delete-image',
         var2: 'tap',
         func: function (evt) {
             evt.stopPropagation();
             handleDeleteControlEvent(this);
         }
        },
        // Add more strategies here.
    ];

    for (var i=0; i< delegationStrategies.length; i++){
        strategy = delegationStrategies[i];
        $(document).delegate(strategy.var1, strategy.var2, strategy.func);
    }
}

You may need to add a special case for the timeout.

The question is whether you foresee the need to add further delegation handlers in the future. If so, then it is a matter of adding another entry to the strategy.

\$\endgroup\$
1
  • \$\begingroup\$ var1=>selector var2=>eventType \$\endgroup\$ Jan 22 '13 at 8:41
3
\$\begingroup\$
//protect your code in a namespace
//here's a simple modular "shielding" which is not immediately useful with the current code
(function (ns) {

  //so in your namespace, we place your register event
  ns.registerEvents = function () {

    var isTapholding = false   //comma separated declaration notation
      , $document = $document  //cache the delegate parent
      ;                        //I suggest a closer parent/nearest common parent rather than document

    //.delegate() is already superseded by .on()
    //it also accepts an event-handler map for the chosen selector
    $document
      .on({
        tap : function (e) {
          //always use {} for those one-liners to avoid readability issues
          if(isTapholding){return;}
          handleEditControlEvent(this);
        },
        vmouseup : function (e) {
          isTapholding = false;
        },
        taphold : function (e) {
          //i'd have to note that .delegate(), .live() and .on() use delegation
          //propagation depends on how, when and where the handlers were attached
          //see this answer for further clarification: http://stackoverflow.com/a/10672682/575527
          e.stopPropagation();
          isTapholding = true;
          handleEditControlTapHoldEvent(this);
        }
      },'.edit-control')
      //for the others, since I can't factor anything else, they stay, but we chain it instead
      .on('tap', '.add-control .insert-image', function (e) {
        handleInsertImageEvent(this);
      });
      .on('tap', '.add-control .add-image', function (e) {
        handleAddControlEvent(this);
      });
      .on('tap', '.delete-image', function (e) {
        evt.stopPropagation();
        handleDeleteControlEvent(this);
      });
  }
}(this.MyCodez = this.MyCodez || {}));

//instead of firing a timer to delay registration until DOM loads
//why not use the .ready() or in this case it's shorthand
$(function () {
  MyCodez.registerEvents();
});
\$\endgroup\$
2
\$\begingroup\$

One big takeaway here to be DRY and more efficient is that you're making several calls to $(document) that could be reduced. Not only is it DRYer to only call it once, but even caching the result of $(document) to a variable would improve things:

$(document)
    .delegate('.add-control .add-image', 'tap', function (evt) {
        handleAddControlEvent(this);
    })
    .delegate('.delete-image', 'tap', function (evt) {
        evt.stopPropagation();
        handleDeleteControlEvent(this);
    })
    .delegate('.edit-control', 'tap', function (evt) {
        if (isTapholding)
            return;
        handleEditControlEvent(this);
    })
    .delegate('.edit-control', 'vmouseup', function (evt) {
        setTimeout(function () {
            isTapholding = false;
        }, 100);
    })
    .delegate('.add-control .insert-image', 'tap', function (evt) {
        handleInsertImageEvent(this);
    });
\$\endgroup\$
1
\$\begingroup\$

All calls have different parameters and different bodies. If there is no fancy JQuery trick, there is nothing you can do about this, and even if such a trick exist I have the feeling that this will result in a switch for every call.

\$\endgroup\$
1
  • \$\begingroup\$ Joseph showed how to merge multiple events. But I'm still not sure if this is better/cleaner/more readable than the native approach. \$\endgroup\$ Jan 22 '13 at 11:03

Not the answer you're looking for? Browse other questions tagged or ask your own question.