Add conditional in jquery handler or inside the function?

Question

A simple favourites system. I wrote it like this:

$(".favourite").click(function(){
  // It's active; deactivate it
  if ($(this).hasClass('active')) {
    changeFavourites('remove', -1);
    }
  // It's not active; activate it
  else {
    changeFavourites('add', 1);
    }
  });

However that was some old code and I'm refactoring it. Would this code be better?

// Handle clicking on favourites to add or remove them
// Cannot use .click() ( http://stackoverflow.com/a/12673849/938236 )
$(document).on('click', ".favourite.active", function(){
  changeFavourites('remove', -1);
  });

// :not() http://stackoverflow.com/a/520271/938236
$(document).on('click', ".favourite:not(.active)", function(){
  changeFavourites('add', 1);
  });

The main issue I see is that I'm embedding a conditional within the selector instead of making it an obvious if. I'm concerned the readability of the code will decrease and the future me will not be happy when I come back to this code. If I'm not mistaken, I think the second code could be called polymorphism.

You're also creating 2 eventhandlers in place of 1. I prefer the way you wrote it originally, but that's my subjective opinion. By the way, I'd refactor the original to eliminate the variables `action` and `updatecount` -- just call `changeFavourites('remove', -1);` as in your refactored example — yitwail, Mar 22 '14 at 19:55
Is there any disadvantage by creating two eventhandlers in place of 1? The code was all following the if, that's why I had the action set in that way; corrected now. — Francisco Presencia, Mar 22 '14 at 20:21
I wouldn't call it a disadvantage, just unnecessary. 200 eventhandlers instead of 100 *would* be a disadvantage. — yitwail, Mar 22 '14 at 20:26
Another argument for one eventhandler is, you're repeating the `".favourite.active"` selector, in effect. So, if you were to change the classes 'favourite' or 'active' at some point, you have to change it in 2 places in the javascript, instead of 1. And one of the goals of refactoring code is to reduce duplicated code. — yitwail, Mar 22 '14 at 20:32
@LShetty It means the update to the count that needs to be performed. If I add a favourite, the count increments in 1. If I delete a favourite, the favourite count decreases in 1. However it was obvious when I had named variables and not so much now ... — Francisco Presencia, Mar 22 '14 at 20:36
@yitwail Would even 200 eventhandlers make a difference in modern computing? (I have around ~10, just asking). About refactoring; first all the code of the function was in each of the if/else. Now in a separate function. I'm trying to reduce it as much as possible, however I think it's more about dependencies than duplication; the question could be, is it okay to depend on the class `.favourite` or should this dependency be eliminated. Would it be better a `var favouriteMatch = ".favourite"` to decouple html from js? I'm not so sure... — Francisco Presencia, Mar 22 '14 at 20:40
In mobile browsers, the number of eventhandlers *might* make a difference. Introducing a variable doesn't really decouple js from html, the variable assignment statement still references the html element class. Anyway, if you're the only programmer who will ever read this code, then your opinion is the only one that matters. — yitwail, Mar 22 '14 at 22:23

score 1 · Answer 1 · answered Mar 22 '14 at 20:28

This would be my preference:

$(document).on('click', ".favourite", function(){
    var active = $(this).hasClass('active');
    changeFavourites(
        active ? 'remove' : 'add', 
        active ? -1 : 1
    );
});

That's the conditional (ternary) operator -- [boolean] ? [value-if-true] : [value-if-false]

score 1 · Answer 2 · answered Mar 22 '14 at 20:46

And here is my version:

$(document).on('click', '.favourite', function() {
    isActive = $(this).hasClass('active');
    //v1
    var method = isActive ? 'remove' : 'add';
    var flag = isActive ? -1 : 1;
    changeFavourites(method, flag);

    //v2 - bit silly
    var params = isActive ? ['remove', -1] : ['add', 1];
    changeFavourites(params[0], params[1]);
});

score 0 · Accepted Answer · answered Mar 27 '14 at 08:30

While both answers are good enough, I still found them lacking a little bit (as my initial code). After much thinking and trying, something I can afford while learning, I decided to go with this code. I am using the two handlers, but also I'm adding another layer of abstraction, saveFavourites(); that will handle keyboard shortcuts and when the user gets to the element pressing tab and clicks enter when arrived to the upvote/downvote:

// Handle clicking on favourites to add or remove them
$(document).on('click', ".favourite:not(.active)", function(){
  saveFavourites('add');
  });

$(document).on('click', ".favourite.active", function(){
  saveFavourites('remove');
  });

Add conditional in jquery handler or inside the function?

3 Answers3