current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

I am making a web app that will have a "my files" area and to do so I am writing my own html and jQuery to control the opening and closing of each folder (or ul li). I want to make sure that this web app is as efficient as I can make it as it may have to deal with a large amount of hierarchy.

Can anyone suggest a way that my Jquery could be more efficient? Many thanks

<!doctype html>
<html>
    <head>
        <meta charset="UTF-8" />
        <meta name="viewport" content="width=device-width" />
        <title>Title</title>
        <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
        <style>
            li {
                cursor: pointer;
                line-height: 20px;
                list-style: none;
            }
            li ul {
                display: none;    
            }
            .active {

            }
        </style>
    </head>
    <body>
        <div class="tree-wrapper">
            <ul class="tree">
                <li>
                    <a href="#">Animals</a>
                    <ul>
                        <li>
                            <a href="#">Birds</a>
                        </li>
                        <li>
                            <a href="#">Mammals</a>
                            <ul>
                                <li>Elephant</li>
                                <li>Mouse</li>
                            </ul>
                        </li>
                        <li>
                            <a href="#">Reptiles</a>
                        </li>
                    </ul>
                </li>
                <li><a href="#">Plants
                        <ul>
                            <li>
                                <a href="#">Flowers</a>
                                <ul>
                                    <li><a href="#">Rose</a></li>
                                    <li><a href="#">Tulip</a></li>
                                </ul>
                            </li>
                            <li><a href="#">Trees</a></li>
                        </ul>
                    </a>
                </li>
            </ul>
        </div>
        <script>
            var folderToggle = $(".tree li > a");   

            $(function () {
                folderToggle.click(function(e) {
                    e.preventDefault();
                    $(this).find("+ ul").toggle("slow");
                    $(this).toggleClass("active");
                });
            });
        </script>
    </body>
</html>

JSFIDDLE

share|improve this question

1 Answer 1

up vote 3 down vote accepted

Step 1: Fix your markup. You have the "plants"-list nested inside the link, which is invalid

Step 2: Fix your JS. You correctly add the click handler after the document's ready, but you set the folderToggle variable when the script's run, regardless of whether the document's been loaded. It still works because you have the script at the end of your page, but still. Keep everything inside the $(...) - it'll also keep the folderToggle var out of the global scope.

Step 2: Cache jQuery objects. Don't write $(this) twice.

Step 3: Don't use find() for this. While I can't speak to an exact performance difference, it's more precise to use next(), as the nested list will be the next element/sibling relative to the link.

$(function () {
    $(".tree li > a").click(function(e) {
        var link = $(this);
        e.preventDefault();
        link.next().toggle("slow");
        link.toggleClass("active");
    });
});

Step 4: If you're going to be adding more stuff to the tree via ajax or something, you'll probably want to rewrite the click handler to handle clicks for links added later:

$(".tree").on("click", "li > a", function (event) {
    ...
});

Step 5: You'll probably want to add a class to the "branch" links in the tree. Otherwise "leaf" links will also attempt to open a nested list, which doesn't make much sense, as there won't be one.

Step 6: Use OL (ordered list) unless your lists are actually "unordered". But if they're file lists, they're probably ordered somehow. Minor thing, but semantics matter.

All that said, there's nothing terribly inefficient in your code. If anything's going to lag, it's the toggle animation, but that can't really be sped up, as it's a single function call to jQuery. It's either there or not there.

share|improve this answer
    
Thank you for your help, very helpful – 2ne May 29 '13 at 13:52
2  
Its also a slight performance increase to use 'return false' rather than 'e.preventDefault' (jsperf showed a 6% benefit, not earth shattering, but still) – atmd May 29 '13 at 16:02
    
@atmd Nice find! – Flambino May 29 '13 at 21:26

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.