16
\$\begingroup\$

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; }
<div id="sidebar">
		<div id="main"></div>
		<div id="tab"></div>
		<br class="clear">
	</div>

\$\endgroup\$
  • 4
    \$\begingroup\$ I'd recommend that you always start with jslint.com which would have caught your non-declared variable and the braceless control statement referenced in the Answer. \$\endgroup\$ – citadelgrad Jan 27 '11 at 0:26
  • \$\begingroup\$ Try Google's own Closure Compiler from Google code.google.com/closure/compiler, which is a tool for making JavaScript download and run faster. It parses your JavaScript, analyzes it, removes dead code and rewrites and minimizes what's left. It also checks syntax, variable references, and types, and warns about common JavaScript pitfalls. There's a web version too: closure-compiler.appspot.com and a Linter. \$\endgroup\$ – David d C e Freitas Mar 21 '11 at 13:06
12
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the suggestions! Updated my question with the new [and improved] code. Also, thanks for the link to that performance page. I used the chaining tip for adding/removing the classes in addition to the "jQuery object as a variable" thing. Is it ok to take out the function {...} that was right after the 500 in .animate()? It works, but I don't know if that means I should leave it out. Anyways - thanks again! :) \$\endgroup\$ – Andrew Jan 22 '11 at 2:57
  • \$\begingroup\$ @Andrew: The key difference is that the function {...} part runs only after the animation has completed. .addClass() isn't something added to the 'fx' queue, so the behavior will be different. \$\endgroup\$ – PleaseStand Jan 22 '11 at 3:03
  • \$\begingroup\$ @Andrew: Your new version of the code looks a lot better, although there are still some unnecessary trailing commas I had not noticed. I don't know what the best way to handle the different versions is. Perhaps you could give some insight in this Meta question. \$\endgroup\$ – PleaseStand Jan 22 '11 at 3:15
  • \$\begingroup\$ Fixed the commas (which cleared up 4 more lines.) Updated the code in the question. Also, that Meta question brings up a good point. Something like that would've helped here. \$\endgroup\$ – Andrew Jan 22 '11 at 3:30
6
\$\begingroup\$

idealmachine's advice is pretty sound. One more minor criticism, using if's to set boolean values is a bit redundant.

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

You can rewrite that a bit more elegantly as

isCtrl = e.which != 17;
\$\endgroup\$
4
\$\begingroup\$

You don't need to wrap toggleMenu in a function, you can just use

$('#tab').click(toggleMenu);

If you don't include the parenthesis, it's taken to mean a reference to the function rather than a function invocation.

It's a bad idea to try and manually keep track of keyboard modifiers. For instance, if your website looses focus while the key is held down, it will think it's still down when you return focus, even if it isn't.

The keyboard event has a ctrlKey property which will be true if the control key is being held down.

It's also probably a good idea to put the key code into a variable which describes which key it is, as having to remember what 97 maps to can be tricky.

$(document).keydown(function (e) {
    if (e.ctrlKey && e.which === aKey) {
        toggleMenu();
        return false;
    }
});

Lastly, it might be a good idea to extract the offset into variable.

var menuSize = '210px';
var $main = $("#main"); 

function toggleMenu() {
    if(!$main.hasClass('out')) {
        $main.animate({'margin-left': '+=' + menuSize}, 500).addClass('out');                         
    } else {
        $main.animate({'margin-left': '-=' + menuSize}, 500).removeClass('out');
    }
}
\$\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.