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 1 down vote favorite

Here's a working version of my code - http://jsfiddle.net/sambenson/g3qsa/#base
How can this be improved upon?

$(function(){
    var box = {},
        island = {},
        canvas = {};

    box.el = $('#box');
    canvas.el = $('#arrow');

    function go(el){
        if(box.el.css('visibility') == 'hidden'){
            reset_name();
            $('.island').addClass('inactive');

            box.top = box.el.offset().top;
            box.corner = {};
            box.corner.left = box.el.offset().left;
            box.corner.right = box.corner.left + box.el.width();

            island.el = el;
            island.top = island.el.offset().top;
            island.left = island.el.offset().left;

            island.el.removeClass('inactive').addClass('active');

            canvas.width = island.left - box.corner.left;
            canvas.height = island.top - box.top;

            canvas.el.css({
                top: box.top,
                left: box.corner.left
            }).attr({
                width: canvas.width,
                height: canvas.height    
            });

            var context = canvas.el[0].getContext('2d');

            context.beginPath();
            context.moveTo(0, 0);
            context.lineTo(box.el.width(), 0);
            context.lineTo(canvas.width, canvas.height);
            context.closePath();

            context.fillStyle = "#eaeaea";
            context.fill();

            box.el.css('visibility', 'visible');
            canvas.el.show();
        }
        else {
            close();
        }
    }

    function reset_name(){
        $('#name_holder').text('').css({
            top: -9999,
            left: -9999
        })
    }

    function close(){
        $('.island').removeClass('inactive active');
        box.el.css('visibility', 'hidden');
        canvas.el[0].width = canvas.el[0].width;
        canvas.el.hide();
    }

    $('.island').hover(function(){
        if(box.el.css('visibility') == 'hidden'){
            var t = $(this),
                name = t.text(),
                holder = $('#name_holder');
                top = t.offset().top - 60;
                left = t.offset().left - 50;

            holder.css({
                top: top,
                left: left
            }).text(name);
        }
    }, function(){ reset_name(); })
    .click(function(){ go($(this)); })

    $('.close').click(function(){
       close();
    })


})

Could it be made faster?
Could it be made more efficient?

share|improve this question

migrated from stackoverflow.com Jul 14 '11 at 13:34

This question came from our site for professional and enthusiast programmers.

2 Answers 2

up vote 2 down vote accepted

Along with @kingjv's suggestions, you can improve the performance of your jQuery selections by making some simply tweaks to the selectors.

Consider if I have the following html:

<div id="container">
    <div class="cell">Cell 1</div>
    <div class="cell">Cell 2</div>
</div>

If you I am using a class selector, such as $(".cell") then the selector engine has to do a little more than it needs to. In this example, the selector engine has to walk the entire DOM to match the correct elements. We can improve this a couple of ways:

  • Specify a tag in the selector, e.g. $("div.cell"). This allows the selector engine to use the native getElementsByTagName method to shrink the number of comparisons the engine has to do to match.

  • Specify a root object to start searching from. E.g., if I've previously cache my selector var root = $("#container"), I can reuse that selector: $("div.cell", root) that way we don't walk the entire DOM for matches.

These are small performance improvements, you'll appreciate them more if you are doing a lot of DOM manipulation and lookup. More simplistic applications won't have such an apparent performance improvement.

share|improve this answer
up vote 2 down vote

My comments are more just on style as opposed to performance:

Instead of using :

if(box.el.css('visibility') == 'hidden'){

it's more readable to use :

if(box.el.is(":hidden")){

And similarly, instead of using :

box.el.css('visibility', 'visible');

you can use :

box.el.hide();

A note on this one though, these are not exactly equivalent because hide actually sets display: none. In this case though I believe that's fine.

And one final comment, it's a good practice to name variables which contain jQuery objects with a $ at the beginning. This allows you to know that the variable is already a jQuery object without going back in the code to check and avoids accidentally creating jQuery objects out of objects that were already jQuery objects.

So something like this:

var t = $(this),
    name = t.text()

would become this:

var $t = $(this),
    name = $t.text()
share|improve this answer
    
Thanks for your review. With regards to your first point: I had to use visibility: hidden instead of .hide() etc because the jQuery needs to be able to measure the width of the box whilst it's hidden. Your $variable_name suggestion is great though! – Sam Jul 14 '11 at 14:05
    
Ah okay, I looked to see if you had visibility for a reason but missed that. Can still use :hidden though I believe. – James Montagne Jul 14 '11 at 14:08

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.