3
\$\begingroup\$

Is there a more efficient way to write this? Seems redundant to me:

    var leaves = $(".leaf");
    var trees = $(".tree");

    $(".my-class").on("click", function (e) {
        if ($(e.target).hasClass("leaf")) {
            trees.each(function() {
                $(this).attr("tabIndex", -1);
            });
            leaves.each(function() {
                $(this).attr("tabIndex", 1);
            });
        } else {
            trees.each(function() {
                $(this).attr("tabIndex", 1);
            });
            leaves.each(function() {
                $(this).attr("tabIndex", -1);
            });
        }
    });
\$\endgroup\$
3
\$\begingroup\$

.attr() can work on all jQuery matched elements at once.

Assuming that performance isn't a concern, I think that the clearest way would be to "reset" all nodes to have a tabIndex of -1 first.

var leaves = $(".leaf");
var trees = $(".tree");

$(".my-class").on("click", function(e) {
    leaves.add(trees).attr("tabIndex", -1);

    var selected = $(e.target).hasClass("leaf") ? leaves : trees;
    selected.attr("tabIndex", 1);
});
\$\endgroup\$
  • \$\begingroup\$ Apologies. I made a typo --the correction is above. The conditionals are actually different. \$\endgroup\$ – hama_hama Mar 24 '14 at 4:27
0
\$\begingroup\$
if ($(e.target).hasClass("leaf")) {
    // do x
} else {
    // do x but in a different way
}

This part makes it harder to maintain. If you change one you must change the other.

$(this).attr("tabIndex", -1);

You do not need jQuery for this. Try this.setAttribute("tabIndex", -1) or even this.tabIndex = -1.

Also, I see some problems with this code:

  1. Why are you checking the event's target in $().on("click")? Would the event's target not be this? Maybe you should change it to $(".leaf.my-class, .tree.my-class")?
  2. What if a user clicks on an element besides .leaf or .tree? The code suggests that you expect one or the other. Should $(".my-class") be changed to $(".leaf, .tree")?
  3. What if you add leaves? You will need to update leaves and trees if new leaves/trees are created.

Here is my proposed solution...

var $leaves = $(".leaf"), $trees = $(".tree"), tabIndexLeaves = -1, tabIndexTrees = 1;
$(".my-class").on("click", function (e) {
    var isLeaf = $(e.target).hasClass("leaf");
    if ((isLeaf && tabIndexLeaves !== 1) || (!isLeaf && tabIndexLeaves === -1)) {
        // Invert the tabIndex
        tabIndexLeaves = -tabIndexLeaves;
        tabIndexTrees = -tabIndexTrees;
        // Update all the leaves' and trees' tabIndexes
        $leaves.attr("tabIndex", "" + tabIndexLeaves);
        $trees.attr("tabIndex", "" + tabIndexTrees);
    }
});
\$\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.