jQuery menu: hide and show on click

Question

I am new to jQuery and currently practicing. I wrote a little piece of code to show and hide a navigation on click and change the text of the button when the navigation is opened and closed.

I am fresh in this area, therefore I would very much value getting some feedback about the code. For example, are there any ways to do it better, with less code or any other area of the matter? I would like a code review to learn and practice (the second if is especially bothering me).

// Function Hide nav
function hideNav() {
$('nav').hide();
}
// Hide Nav
hideNav();
// Function Show Nav
function showNav() {
$('nav').show();
}
// Clickevent on Button
$('button').click(function() {
if($('nav').is(':hidden')){
// Show Nav on Clickevent
showNav();
// Change Text when Nav is showing
$(this).text('Close me');
}
else
hideNav();
// Change Text back when Nav is hiding
if($('nav').is(':hidden')){
$('button').text('Open me');
}
});

Answer 1

Some general style points:

1. Indentation, it's important. Please indent one level for each set of braces moving forward.
2. Frivolous comments. You have one comment for each function. These comments simply repeat what the names of the functions already tell us. They're noise. Remove them. I find it's a good practice to write pseudocode comments and then "fill in the blanks", but they shouldn't stay once you've implemented it. Comments should always say why, rarely should they say what.

Now we can see what's really going on a little better.

function hideNav() {
 $('nav').hide();
}
hideNav();
function showNav() {
 $('nav').show();
}
$('button').click(function() {
 if($('nav').is(':hidden')){
 showNav();
 $(this).text('Close me');
 }
 else
 hideNav();
 if($('nav').is(':hidden')){
 $('button').text('Open me');
 }
});

Personally, I don't see a point to the showNav() and hideNav() functions. It seems they don't do much except save a few keystrokes, but you've already written them and they're not actually hurting anything. I guess they're ok.

This else statement really needs some braces.

 if($('nav').is(':hidden')){
 showNav();
 $(this).text('Close me');
 }
 else
 hideNav();
 if($('nav').is(':hidden')){
 $('button').text('Open me');
 }

It's really not clear when the second if($('nav').is(':hidden') is supposed to execute. I think it should look like this.

 if($('nav').is(':hidden')){
 showNav();
 $(this).text('Close me');
 } else {
 hideNav();
 }
 if($('nav').is(':hidden')){
 $.('button').text('Open me');
 }

Which brings to mind some code duplication. I would create a isNavHidden() function to simplify all this a bit.

function isNavHidden() {
 return $('nav').is(':hidden');
}

Then you could write the above snippet like this.

 if(isNavHidden()){
 showNav();
 $(this).text('Close me');
 } else {
 hideNav();
 }
 if(isNavHidden){
 $.('button').text('Open me');
 }

• Steve: Nice. That helped. And btw: I always indent my code. Its just that 4 spaces everytime... :) But thank you for your suggestions.
• RubberDuck: I'm not really a Javascript guy, but I think the convention is two spaces. I tend to use four as well. Don't forget to upvote and accept useful answers. =;)-