12
\$\begingroup\$

I have the following .click() functions:

$('.tab').click(function(){
    $('.tab').removeClass('activeTab');
    $(this).addClass('activeTab');
});

$('.edit').click(function(){
    $(this).hide();
    $(this).next().show();
});

$('.cancel').click(function(){
    $(this).parent().hide();
    $(this).parent().prev().show();
});

Is it possible to combine these functions doing something like this?

Pseudocode:

$.click(function(){
    for $('.tab'){
        //do this
    }
    for $('.edit'){
        //do this
    }
    for $('.cancel'){
        //do this
    }
});

HTML:

<a href="javascript:void(0);" class="edit">Edit</a>
<span class="controls">
    <a href="javascript:void(0);" class="cancel">Cancel</a> |
    <a href="javascript:void(0);" class="save">Save</a> 
</span>
\$\endgroup\$
0

4 Answers 4

31
\$\begingroup\$

First off, I'd rewrite the HTML to get rid of the javascript href attributes. I'd also use a data-action attribute for the action name. You can still use class to do the styling of course, but style is not behavior, so keep those things separate (i.e. the class might be action-button primary highlighted or something, but the action would still be save):

<a href="#" data-action="edit">Edit</a>
<span class="controls">
    <a href="#" data-action="cancel">Cancel</a> |
    <a href="#" data-action="save">Save</a> 
</span>

Code-wise, what each link has in common is that they should preventDefault so the links aren't "followed" when clicked. So at the very least, we'll need this:

$("a[data-action]").on("click", function (event) {
  event.preventDefault();
});

Then of course, there are the actions/behaviors themselves. I'd suggest putting the logic for each in an object, and expanding the generic click handler like so:

var actions = {
  edit:   function (event) { ... },
  cancel: function (event) { ... },
  save:   function (event) { ... }
};

$("a[data-action]").on("click", function (event) {
  var link = $(this),
      action = link.data("action");

  event.preventDefault();

  // If there's an action with the given name, call it
  if( typeof actions[action] === "function" ) {
    actions[action].call(this, event);
  }
});

Now you have a generic click handler, and an easily extensible list of actions. The action functions themselves behave exactly like normal jQuery event handlers (i.e. this will refer to the link clicked, and the first argument will be the event).

\$\endgroup\$
1
  • 2
    \$\begingroup\$ slowclap this was the answer I said japanFour should wait for! \$\endgroup\$
    – jsanc623
    Commented Nov 5, 2012 at 2:41
5
\$\begingroup\$

To answer your question directly: yes, it's possible to combine them into one function.

Is it advisable in the way you presented though? No.

I would argue that anytime there's a function with hard coded functionality controls, something is probably wrong.

Consider this for example:

function someHandler() {
    var $this = $(this);
    if ($this.hasClass('class-a')) {
        //do something specific to a
    }
    if ($this.hasClass('class-b')) {
        //do something specific to b
    }
    if ($this.hasClass('class-c')) {
        //do something specific to c
    }
}

$(".class-a, .class-b, .class-c").click(someHandler);

The easiest way to illustrate why this is a problem is to consider what happens if you want to reuse any of the specific functionalities.

How would you reuse the functionality in the //do something specific to a segment on something that doesn't have the class class-a without copying and pasting the code? You couldn't. (Well, not cleanly anyway.)

For this reason, it's good to try to separate functionalities and their bindings:

function someHandlerA() {
    //do something specific to a
}
function someHandlerB() {
    //do something specific to b
}
function someHandlerC() {
    //do something specific to b
}
$(".class-a").click(someHandlerA);
$(".class-b").click(someHandlerB);
$(".class-c").click(someHandlerC);

Now what if you want to reuse the functionality? It's easy: $("some selector").bind("some event", someHandler);

Obviously in large applications it becomes a lot more complex than this. There are of course times when it's ok to switch functionalities based on some condition. It's typically best to keep different functionalities as atomic as possible though. It relates back to the idea of separation of concerns. Each segment of code should do only one thing, even if that one thing is just dispatching to more than one thing.

\$\endgroup\$
3
  • \$\begingroup\$ I agree with the TL;DR part. Separate handlers for separate ideas but the rest of this is not really an answer. Why are switch statements typically a bad sign? How can this be improved using this idea that the handlers can stay separate? \$\endgroup\$ Commented Nov 5, 2012 at 3:52
  • 1
    \$\begingroup\$ @JamesKhoury I should have been a lot more specific in my answer, I suppose. A switch that controls the functionality of a function is typically a bad sign (or a switch-fallthrough type construct like his example uses -- not sure now why I read that as a switch). It means that potentially disjoint functionalities are being crammed into the same place for convenience. (Or, in some situations, because responsibilities are not placed in the right segments.) I don't have time now, but I will update my answer later. \$\endgroup\$
    – Corbin
    Commented Nov 5, 2012 at 4:02
  • \$\begingroup\$ @JamesKhoury Have updated it to be more specific :). \$\endgroup\$
    – Corbin
    Commented Nov 7, 2012 at 1:24
4
\$\begingroup\$

I think you're looking for something along the lines of:

$('.tab, .edit, .cancel').click(function(){
    switch(this.className){
        case 'tab' : {
            $('.tab').removeClass('activeTab');
            $(this).addClass('activeTab');
        }break;
        case 'edit' : {
            $(this).hide();
            $(this).next().show();
        } break;
        case 'cancel' : {
            $(this).parent().hide();
            $(this).parent().prev().show();
        } break;
    }
});
\$\endgroup\$
4
  • 1
    \$\begingroup\$ I fixed your selector. But -1: your code still does not work. You can't switch on the element and expect that the comparison will magically check the class of the element. \$\endgroup\$
    – ANeves
    Commented Nov 3, 2012 at 18:57
  • \$\begingroup\$ @ANeves: Sorry, was 4:30am here whenI wrote it. I'll correct it now. \$\endgroup\$
    – jsanc623
    Commented Nov 3, 2012 at 20:59
  • \$\begingroup\$ @ANeves I'll take a +1 now that the code works :) and again, my apologies for writing incorrect code...never code sans sleep. \$\endgroup\$
    – jsanc623
    Commented Nov 3, 2012 at 21:13
  • 2
    \$\begingroup\$ This would fail if an element had more than one class. For example: <div class="cancel button"><div>. The className of that is cancel button which obviously does not equal cancel. \$\endgroup\$
    – Corbin
    Commented Nov 5, 2012 at 1:47
0
\$\begingroup\$

I'm not sure about this but I think you can use your own manager in order to keep spaghetti code to a minimum.

Demo: http://jsfiddle.net/8C5wR/1/

function ClickManager(element) {
 this.el = $(element);
 this.check = function(){
   if (this.el.hasClass('class1')) {
    console.log('class 1 detected'); 
   }

   if (this.el.hasClass('class2')) {
    console.log('class 2 detected'); 
   }

   if (this.el.hasClass('class3')){
    console.log('class 3 detected'); 
   }
   };
}

(function(){
    $('.class1, .class2, .class3').on('click', function(){
      manager = new ClickManager(this);
      manager.check();
    });
})();​

Maybe others can weigh in to point out possible fallouts of this.

\$\endgroup\$
17
  • 1
    \$\begingroup\$ The ClickManager seems way overkill. jsfiddle.net/8C5wR/5 \$\endgroup\$
    – Corbin
    Commented Nov 4, 2012 at 6:56
  • 2
    \$\begingroup\$ @Corbin That's exactly what I want to avoid. Try to use that approach in a serious application and you will end up dealing with messy code. \$\endgroup\$
    – r_31415
    Commented Nov 4, 2012 at 7:35
  • 1
    \$\begingroup\$ @ANeves As I said before, we have to consider that something like this is going to be used in something with a good deal of functionality, so we need a manager. Although, I would like to know if there are alternatives that work in complex code. \$\endgroup\$
    – r_31415
    Commented Nov 4, 2012 at 7:39
  • 1
    \$\begingroup\$ @RobertSmith So rather than managing three handlers, you'd rather manager three handlers and their wrapper? It's just adding complexity. What if you want to use the handlers independently? What if you want to only bind two of them at some point? (Oh, by the way, part of the confusion might be the jsfiddle I posted. I disagree with the code in that fiddle. I think the proper way is as I said in my answer: leave them separate. I just think if you aren't going to separate them, there's no reason to add yet another coupling.) \$\endgroup\$
    – Corbin
    Commented Nov 4, 2012 at 21:00
  • 2
    \$\begingroup\$ @RobertSmith It sounds to me like you're jumping the gun with making classes. These handlers don't hold state, so they don't particularly need a class-style context associated with them. To each his own though, I suppose. \$\endgroup\$
    – Corbin
    Commented Nov 5, 2012 at 1:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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