Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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)?

share|improve this question

3 Answers 3

up vote 5 down vote accepted

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.

share|improve this answer
    
var1=>selector var2=>eventType –  mnhg Jan 22 '13 at 8:41
    
+1 interesting method –  Joseph the Dreamer Jan 23 '13 at 0:42

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.

share|improve this answer
    
Joseph showed how to merge multiple events. But I'm still not sure if this is better/cleaner/more readable than the native approach. –  mnhg Jan 22 '13 at 11:03
//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();
});
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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