Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

My app has a feature that when you click the "new data" button, HTML is loaded to the page via ajax. Because I am using ajax, all events I want to add has to be bound using the on() method.

There are 4 on('click') functions that are bound to 4 different HTML elements. and they look like this:

// save data
$('#container').on( {
    click: function() {
       // save code here            
    }
}, "a.save_data" );

// cancel data
$('#container').on( {
    click: function() {
       // cancel code here            
    }
}, "a.cancel_data" );

// edit data
$('#container').on( {
    click: function() {
        // edit code here            
    }
}, "a.edit_data" );

// delete data
$('#container').on( {
    click: function() {
        // delete code here
    }
}, "a.delete_data" );

Each of these functions share multiple jquery wrappers to select certain elements within the HTML. Like this:

$(this).closest('div.content_wrap');
$(this).closest('div.Content');

(please note, that the reason I am using selectors such as $(this).closest() is because the user has the ability to add the same HTML multiple times on to the page.)

First, is there a better way to organize all of the on() functions? Maybe combine them into one object OR create a function?

Second, because all 4 functions use the same jquery wrappers, is there any way to declare them in a variable globally somewhere, so i don't keep retyping them. I wasn't able to figure out how declare variables with $(this) and apply it to more than one function.

To me, how I am doing it doesn't seem very dry and I was wondering if someone can help me better organize.

Thanks a lot

EDIT

This is what I came up with

var elements = ["a.save_data", "a.cancel_data", "a.delete_data", "a.edit_data"];

$.each(elements, function(i, el)
{   
    $('#container').on('click', el, function()
    {   
        // Variables to be used in all if statements below
        var var1 = value
            var2 = value

        // Save Data
        if (el == "a.save_data") {
            // save code
        }

        // Cancel Data
        else if (el == "a.cancel_data") {
            // cancel code
        }

        // Edit Data
        else if (el == "a.qEdit") {
            // edit code
        }

        // Delete Data
        else if (el == "a.qDelete") {
            // delete code
        }

        return false; 
    });
});

EDIT

var handlers = {
    test: function(f, n, t) {
        f.hide();
        n.hide();
        t.text("hidden");
    }
}


$('button').on('click', function() {

    var selector = $(this).prop('id'),
        first = $(this).prev(),
        next = $(this).next(),
        top = $(this).closest('div.top');

    handlers[selector](first, next, top);
});

This code executed exactly the same as:

var handlers = {
    test: function(e, f, n, t) {
        f.hide();
        n.hide();
        t.text("hidden");
    }
}

$('button').on('click', function(event) {

    var selector = $(this).prop('id'),
        first = $(this).prev(),
        next = $(this).next(),
        top = $(this).closest('div.top');

    handlers[selector].call(this, event, first, next, top);
});
share|improve this question
2  
See this question/answer (/shameless self-promotion) – Flambino Oct 20 '12 at 19:41
1  
Hey thanks a lot, that was informative...I was able to follow, but wasn't really able to apply it to what I was doing, but I did find a way to set it up so I don't have so much repetitive code...It's under EDIT in my post if you have any additional feedback...Thanks – elgarcon Oct 21 '12 at 7:44
add comment (requires an account with 50 reputation)

2 Answers

up vote 1 down vote accepted

Here's how I'd probably do it (basically, it's the same as in the answer I linked to in the comments, I'm just adding it here for clarity)

var handlers = {
  save_data:   function (event, content, wrap) {
    // save code
  },

  cancel_data: function (event, content, wrap) {
    // cancel code
  },

  edit_data:   function (event, content, wrap) {
    // edit code
  },

  delete_date: function (event, content, wrap) {
    // delete code
  }
};

$('#container a').on('click', function (event) {
  var klass = this.className,
      content,
      wrap;

  if(typeof handlers[klass] === "function") {
    content = $(this).closest("div.Content"),
    wrap    = $(this).closest("div.content_wrap");
    handlers[klass].call(this, event, content, wrap);
  }

  // you want to always "absorb" the click event, you can
  // do so here or in the if-block above
  //   event.preventDefault();
});

You can write the handler functions just like you'd write them in your orginal code (i.e. this will refer to the right element, etc.). However, the click event handler will pass along the the content and wrap arguments, so you don't have to have code to select those elements in every handler function.

Of course, if your links have multiple classes, the className trick won't work. That's why I'd advocate (as I do in the answer I linked to) using a data-* attribute if possible.

share|improve this answer
Hey thanks for this again, much appreciated...I am actually returning back to this and I had a question...See my code above, I added a new edit...It's just a simple test...I did exactly what you have except I took out the event object, the if (typeof...) statement and the call() method...I assume the if statement was just for security sake, but I can't figure out why you need the call function, I was able to run the code without it and get the same results. Also, why did you add the event object? – elgarcon Oct 23 '12 at 6:14
Also, I tested this with data being pulled in via ajax and I noticed that without declaring the element I am clicking within the on() arguments, the on() functionality didn't work, but once I replaced it with live(), it did work...Which I know has been deprecated... – elgarcon Oct 23 '12 at 6:45
@elgarcon The typeof check is there to make sure that the handler actually exists and is a function. If you try handlers["xyz"](), and handlers["xyz"] doesn't exist or isn't a function, you'll get an error and JS just stops executing. The point of call and passing the event obj, is so the handler functions can be exactly the same as if you added them to the elements the normal way: this will be the element the event occurred on, and the 1st argument is the event obj. – Flambino Oct 23 '12 at 8:03
@elgarcon Not sure what you mean by the on vs live stuff. But if things stop working when you remove things, well, don't remove things. jQuery's on function works, so the problem must be on your end. Don't know what else to tell ya. – Flambino Oct 23 '12 at 8:11
add comment (requires an account with 50 reputation)
up vote 1 down vote

Perhaps something like this? 

function manipulateData($action){
    $('#container').on( {
        click: function() {
            switch(action){
                case "save" : saveData();
                    break;
                case "delete" : deleteData();
            }
        }
    }, "a." + action + "_data" );
}

Also, see Flambinos comment

share|improve this answer
1  
Hey thanks for taking the time to help...I came up with something similar and posted it in my post under EDIT...My thinking there is, because I am binding 4 different functions to 4 different elements, I just wanted a way to cut back on the repetitive code. What I have done allows me to use the same variables for all events and also allows to write 'click' and 'return false' only once ...Any improvements to this that you know of? – elgarcon Oct 21 '12 at 7:39
add comment (requires an account with 50 reputation)

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.