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 .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>
share|improve this question
    
Hey everyone, Sorry I didn't check this post for a few days. Thanks for all of the answers and comments. –  Kris Hollenbeck Nov 5 '12 at 14:26
add comment

4 Answers 4

up vote 10 down vote accepted

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

share|improve this answer
1  
slowclap this was the answer I said japanFour should wait for! –  jsanc623 Nov 5 '12 at 2:41
add comment

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.

share|improve this answer
    
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? –  James Khoury Nov 5 '12 at 3:52
1  
@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. –  Corbin Nov 5 '12 at 4:02
    
@JamesKhoury Have updated it to be more specific :). –  Corbin Nov 7 '12 at 1:24
add comment

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;
    }
});
share|improve this answer
1  
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. –  ANeves Nov 3 '12 at 18:57
    
@ANeves: Sorry, was 4:30am here whenI wrote it. I'll correct it now. –  jsanc623 Nov 3 '12 at 20:59
    
@ANeves I'll take a +1 now that the code works :) and again, my apologies for writing incorrect code...never code sans sleep. –  jsanc623 Nov 3 '12 at 21:13
2  
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. –  Corbin Nov 5 '12 at 1:47
add comment

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.

share|improve this answer
1  
The ClickManager seems way overkill. jsfiddle.net/8C5wR/5 –  Corbin Nov 4 '12 at 6:56
2  
@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. –  Robert Smith Nov 4 '12 at 7:35
1  
@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. –  Robert Smith Nov 4 '12 at 7:39
1  
@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.) –  Corbin Nov 4 '12 at 21:00
2  
@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. –  Corbin Nov 5 '12 at 1:45
show 12 more comments

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.