4
\$\begingroup\$

I find myself using a lot of .find functions within what I'm developing. The reason being that I need to run this same function on a TON of different characters eventually, and the easiest way I could find to do this was make appropriate sections classes and wrap each character in their respective ID.

Is using .find a ton (to only target the classes within the #characterId) considered bad practice?

JS Fiddle Example: http://jsfiddle.net/hgwNU/6/

Example Script:

(x is the internal javascript object, where y is the DOM ID for what to target)

function updateDom(x,y){
    if(x.pc < 1 || x.speed >= 4){
        y.find('.addSpeed').attr('disabled', 'disabled');
    }
    if(x.speed <= 1){
        y.find('.remSpeed').attr('disabled', 'disabled');
    }
    if(x.speed > 1){
        y.find('.remSpeed').removeAttr('disabled');
    }
    if(x.speed < 4 && x.pc > 0){
        y.find('.addSpeed').removeAttr('disabled');
    }
    y.find(":checkbox").each(function() {
        if($(this).data('pc') > x.pc && $(this).prop('checked')==false){
            $(this).attr('disabled', 'disabled');
        } else if ($(this).data('pc') <= x.pc) {
            $(this).removeAttr('disabled');
        }
    });
    for (var key in x) {
        y.find('#' + key).text(x[key]);
    };
}

Called with:

updatePlayer(bulk,$('#bulkUi'));

JS Fiddle Example: http://jsfiddle.net/hgwNU/6/

I suppose I'm just curious if I'm off to a good start for how to manipulate both the GUI and the internal object effectively, easily, quickly and repeatedly. Looking for best practices if anybody has any feedback thus far.

\$\endgroup\$
6
\$\begingroup\$

Personally instead of using .find I prefer passing context to the jQuery selector like this;

$('.remSpeed', y);

Which will find all instances of .remSpeed that are within the element y.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Doesn't this only work for direct descendents? \$\endgroup\$ – Nicholas Hazel Apr 7 '14 at 15:57
  • \$\begingroup\$ @NicholasHazel Nope. \$\endgroup\$ – Jivings Apr 7 '14 at 18:34
  • \$\begingroup\$ Thanks for the answer :-) Is there a performance increase doing it this way? \$\endgroup\$ – Nicholas Hazel Apr 7 '14 at 18:37
  • \$\begingroup\$ It appears the find() method is slightly faster. But I still get around 600k operations per second for the context selector, so hardly noticeable. I use context because I find it easier to read and understand, rather than for performance reasons. \$\endgroup\$ – Jivings Apr 7 '14 at 18:42
4
\$\begingroup\$

The most immediate problems:

  • Use better variable names than x and y. It's almost unreadable this way. If one of them is the player data object and the other one a jQuery DOM object, why not name them player and dom respectively.

  • Values of HTML id attribute should be unique across the whole page. When you have several player boxes on one page, you're better off using classes than ID-s.

If you're planning to place a ton of these characters on a page, you might be better of using event delegation, instead of binding the event handlers on each and every element - see the jQuery.on() function reference.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Regarding the id attribute, that was a mistake I corrected right after posting this. Good point for the parameter names. Just tried to make it quick and dirty :-P. And this paragraph seems to indicate that doing it this way is ideal...? \$\endgroup\$ – Nicholas Hazel Apr 5 '14 at 18:20
4
\$\begingroup\$

It is much faster to look up items by id (I assume a hash table because sometimes ids are defacto duplicate) than searching for a class in a collection (which is what you force the browser to do when you select by class). This is specially so in old browsers like some old versions of IE.

Also, you should use else's (removing in turn redundant if's), and group the if's :

I would place the simpler if's at the top of the code (those who only depend on variable):

if(x.speed > 1){
    y.find('.remSpeed').removeAttr('disabled');
}
else{
    y.find('.remSpeed').attr('disabled', 'disabled');
}

in fact i would separate the find from the actually work on the attribute:

var remSpeed = y.find('.remSpeed');
if(x.speed > 1){
    remSpeed('disabled');
}
else{
    remSpeed('disabled', 'disabled');
}


var addSpeed = y.find('.addSpeed');
if(x.pc < 1 || x.speed >= 4){
    addSpeed.attr('disabled', 'disabled');
}
else{
    addSpeed.removeAttr('disabled');
}

You could also simplify the other set of conditionals:

    if($(this).data('pc') > x.pc){
        if ($(this).prop('checked')==false)
        {
            $(this).attr('disabled', 'disabled');
        }
    }
    else
    {
        $(this).removeAttr('disabled');
    }
|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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