Using a navigation menu to help keep material organized

I'm working on a personal project to keep different snippets/examples/small projects of mine organized. I want to make the most of my page width, so I decided to write a navigation menu that slides out. It works as expected, but the code is kinda big.

I provided the HTML/CSS just in case that helps, but I'm really only looking for help with the JavaScript. I don't need anyone to rewrite the whole thing.



$(document).ready(function() {
$('#tab').click(function() {
if($('#main').attr('class') != 'out') {
$('#main').animate({
'margin-left': '+=340px',
}, 500, function() {
$('#main').addClass('out');
});

} else {
$('#main').animate({
'margin-left': '-=340px',
}, 500, function() {
$('#main').removeClass('out');
});

}
});

//does the same as above, but when I press ctrl + 1. from /http://www.scottklarr.com/topic/126/how-to-create-ctrl-key-shortcuts-in-javascript/
$(document).keyup(function (e) {
if(e.which == 17) isCtrl=false;
}).keydown(function (e) {
if(e.which == 17) isCtrl=true;
if(e.which == 97 && isCtrl == true) {
if($('#main').attr('class') != 'out') {
$('#main').animate({
'margin-left': '+=340px',
}, 500, function() {
$('#main').addClass('out');
});

} else {
$('#main').animate({
'margin-left': '-=340px',
}, 500, function() {
$('#main').removeClass('out');
});

}
return false;
}
});
});
#sidebar {
top: 0;
left: 0;
position: relative;
float: left;
}

#main {
width: 320px;
padding: 10px;
float: left;
margin-left: -340px;
}

#tab {
width: 30px;
height: 120px;
padding: 10px;
float: left;
}

.clear { clear: both; }






First, it seems that isCtrl is not declared anywhere in your code. Put var isCtrl = false; as your first line inside the ready function. Otherwise, you will get a JavaScript error when the first key the user presses is not the Ctrl key. Also, == true is unnecessary within an if statement; it can be omitted.

Second, this line could be improved, as it fails if the element is a member of another class:

if($('#main').attr('class') != 'out') {

It can be rewritten using .hasClass() as:

if(!$('#main').hasClass('out')) {

In fact, you should refactor this entire block out to a separate function to avoid duplicating code:

function toggleMenu() {
    if(!$('#main').hasClass('out')) {
        // ...
    } else {
        // ...
    }
}

And put toggleMenu(); where the duplicate code was in each function. Also, if performance is a concern, you should cache the jQuery object $('#main') by declaring another variable at the beginning of the ready function. Not doing so is slow because jQuery would have to find the matched elements within the document again.

A more minor criticism is that of braceless control statements, such as:

if(e.which == 17) isCtrl=false;

Some say that it is best to always put the code within indented braces to avoid errors caused by incorrectly placed semicolons.

Finally, the indentation doesn't look right throughout the entire code. Fix that, and your code will become somewhat more readable.

To add: It also looks like you have an extra comma at the end of:

'margin-left': '+=210px',

This is syntactically valid, but it causes problems with Internet Explorer 7 and below.

StackExchange Code Review Q#148, answer score: 12