It works. But is it best practice?

Question

When I start to learn a new language, I always feel like I'm not doing it the practical, standard way. So here's a question regarding jQuery and if what I'm doing is acceptable.

I have 3 images.

<img src="del.png" class="delete user" />
<img src="del.png" class="delete event" />
<img src="del.png" class="delete recipe" />

Then I have jQuery detect a click event on $('.delete').

$('.delete').click(function() {
    if( $(this).hasClass('user') ) {
        action = 'user'
    }

    if( $(this).hasClass('event') ) {
        action = 'event'
    }

   if( $(this).hasClass('recipe') ) {
        action = 'recipe'
    }

   // I then use ajax, send the action as $_POST to the PHP script
   // and it tells PHP what to delete.

   // Doing it this way, I don't have to use 3 onclick functions and 3 ajax functions.


});

Answer 1

I'd say there's nothing wrong with that way of doing what you're doing. It's actually very clean and readable to use dual classes in a dispatcher function like that.

Answer 2

I agree with @Tommi, you are keeping yourself from writing more code than you need, and also consolidating your ajax call to a single place instead of having 3 roughly identical ajax calls (or writing an ajax function and calling it from 3 separate locations).

If this works for you and is not confusing, I don't think there's anything wrong with it. In fact, there are a number of ways to do what you're trying to do in an ugly and inelegant way, but this isn't one of them.

Answer 3

What if an object had user and event as classes, then the action would be event rather than user?

Answer 4

This solution makes your system dependent on javascript to work. A more robust solution is to build something that is at least functional without javascript, and then on load use javascript to modify the behavior of the buttons to do something more ajax-y.

It requires more code to do it that way and doesn't look as elegant. But if you're looking for the "best" solution, you may want to weigh in and determine whether handling the "no javascript" corner-case is a priority or not.

Which ever route you decide, you should at least consciously make a decision one way or another instead of letting it surprise you later on.

Answer 5

You might like to create a separate function for your AJAX, and pass action into it.

As your code stands, it will allow you to change the actions dynamically and have multiple actions for one img. I would document whether this is intentional.

Answer 6

This code is fine seen isolatedly. I would however suggest using the onClick event on the <a> directly, instead of relying on jquery adding functionality to your elements.

While jquery does an acceptable job of it, you end up with code that is hard to read, because the end result does not display the fact that there actually is an eventhandler associated with the onClick event. I personally prefer to be able to see what code is actually is being executed, using tools like Firebug to examine the page I'm being served.

This is ofc a non-issue if you are the only person to ever read your code.

Answer 7

If I were tempted to do something like this I would generalize things a bit more, like this

function getType(elem)
{
    var m = /\btype:([a-zA-Z_]+)\b/.exec(elem.className);
    return m ? m[1] : null;
}

function deleteType(type)
{
    // delete code instead of alert
    alert("Deleting type " + type);
}

$(function()
{
    $(".delete").click(function(ev)
    {
     var type = getType(this);
     if (type)
     {
      deleteType(type);
     }
     return false;
    });
});

used on elements like

<a href="#" class="delete type:foo">delete foo</a>
<a href="#" class="delete type:bar xxx">delete bar</a>
<a href="#" class="delete">do nothing</a>

No additional code to add for new types.

Answer 8

Assuming you have a LI structure (or something similar):

<ul>
    <li>Some text <a href="delete.php?do=user">delete</a></li>
    <li>Some text <a href="delete.php?do=event">delete</a></li>
    <li>Some text <a href="delete.php?do=recipe">delete</a></li>
</ul>

you can do this way

$('.delete').click(function(){
    var t=$(this);
    jQuery.ajax({
     type: "GET",
     url: t.attr('href'),
     cache: false,
     success: function(data){
      t.parent().remove(); // you remove the whole li tag
     }
    });
});

Of course, if you need to trigger different function on ajax success, then your ways I think is the best pick.

Answer 9

On reading your code, I had the same urge of refactoring the code into something shorter specially that there is repetition in code structure.

The following solution gets the list of classes of the div that has been clicked. It then retrieves the last class listed.

$('.delete').click(function() {

   // Your 'action' class type must always be the last in the list
   action = $(this).attr('class').split(' ').slice(-1); 

   // Remaining logic goes here...

});