Efficient way of using if and else.

[doughavant]

Hi there! I'm pretty new to Javascript and I was wondering if there was a better method to handle the situation. I'm making a website that uses Javascript to hide and show content via DIV boxes.

It's working great but the only issue is that I think there might be a better way to make it more efficient as the more 'pages' / 'DIV boxes' involved, the longer the code. Here's the current code I have:

function myself() {
	if($('#myself').css('display') == 'block') {
		$('#myself').hide();
		$('#banner').fadeIn();
	}
	else if($('#about').css('display') == 'block') {
		$('#about').hide();
		$('#myself').fadeIn();
	}
	else if($('#animation').css('display') == 'block') {
		$('#animation').hide();
		$('#myself').fadeIn();
	}
	else {
		$('#banner').hide();
		$('#myself').fadeIn();
	}
}

function about() {
	if($('#about').css('display') == 'block') {
		$('#about').hide();
		$('#banner').fadeIn();
	}
	else if($('#myself').css('display') == 'block') {
		$('#myself').hide();
		$('#about').fadeIn();
	}
	else if($('#animation').css('display') == 'block') {
		$('#animation').hide();
		$('#about').fadeIn();
	}
	else {
		$('#banner').hide();
		$('#about').fadeIn();
	}
	
}

function animation() {
	if($('#animation').css('display') == 'block') {
		$('#animation').hide();
		$('#banner').fadeIn();
	}
	else if($('#about').css('display') == 'block') {
		$('#about').hide();
		$('#animation').fadeIn();
	}
		else if($('#myself').css('display') == 'block') {
		$('#myself').hide();
		$('#animation').fadeIn();
	}
	else {
		$('#banner').hide();
		$('#animation').fadeIn();
	}

[sunfighter]

one or two else if's is OK by me but longer lists get the SWITCH Statement:
http://www.w3schools.com/js/js_switch.asp

[Airblader]

@ sunfighter

A switch statement isn't that easy to use in this particular case because he's not just iffing over one variable.

@ doughavant

First things first: Learn to cache your jQuery variables! Google is your friend. This is a standard improvement known to have a dramatic effect on performance (in particular mobile devices, but the bigger the site, the more important it is for PCs too).

As for the question itself, I agree that this if/else stuff doesn't look too great (working on a big corporate software everyday I'm getting used to it, though – unfortunately). What you want to do is unify those functions as they all pretty much do the same. This solution is a bit tricky – does it do what you want it to do? I'm asking because it's not 100% equivalent to your code, but if it's supposed to do what I think it is, it should (hopefully) work. You can see it being put to use here: http://jsfiddle.net/K8pTU/

function showPage( page ) {
    var matchedItself = $( '#myself, #about, #animation, #banner' )
        .filter(function () {
            return $( this ).css( 'display' ) === 'block';
        })
        .first()
        .hide()
        .is( $( '#' + page ) );

    if( matchedItself ) {
        $( '#banner' ).fadeIn();
    } else {
        $( '#' + page ).fadeIn();
    }
}

As you can see, it avoids if/else/switch constructs almost completely and rather utilizes some (still fairly simple) "jQuery magic". Reading it carefully should make it easy to understand, but if you need further explanations on how it works, just ask.

[doughavant]

Originally Posted by Airblader

@ sunfighter

A switch statement isn't that easy to use in this particular case because he's not just iffing over one variable.

@ doughavant

First things first: Learn to cache your jQuery variables! Google is your friend. This is a standard improvement known to have a dramatic effect on performance (in particular mobile devices, but the bigger the site, the more important it is for PCs too).

As for the question itself, I agree that this if/else stuff doesn't look too great (working on a big corporate software everyday I'm getting used to it, though – unfortunately). What you want to do is unify those functions as they all pretty much do the same. This solution is a bit tricky – does it do what you want it to do? I'm asking because it's not 100% equivalent to your code, but if it's supposed to do what I think it is, it should (hopefully) work. You can see it being put to use here: http://jsfiddle.net/K8pTU/

function showPage( page ) {
    var matchedItself = $( '#myself, #about, #animation, #banner' )
        .filter(function () {
            return $( this ).css( 'display' ) === 'block';
        })
        .first()
        .hide()
        .is( $( '#' + page ) );

    if( matchedItself ) {
        $( '#banner' ).fadeIn();
    } else {
        $( '#' + page ).fadeIn();
    }
}

As you can see, it avoids if/else/switch constructs almost completely and rather utilizes some (still fairly simple) "jQuery magic". Reading it carefully should make it easy to understand, but if you need further explanations on how it works, just ask.

Hey thanks for the great tip! I had trouble understanding your code as a whole but parts of it actually helped me out! Here's what I came up with:

function show(selector) {
  if ($(selector).is(":visible")) {
    $(selector).hide();
    $("#banner").fadeIn();
  } else {
    $("#banner,.hide").hide();
    $(selector).fadeIn();
  }
}

function myself() {
	show("#myself");
}

function about() {
	show("#about");
}

function animation() {
	show("#animation");
}

What do you think? It answered my question but the thing is, is this the most practical method? As I do plan to add new content, I somehow think this is still flawed. I'm still trying to understand your code completely (sorry! total college CS newb here :<)

[Airblader]

Your code is still bad because you still add a new function for every page (I just assume they're pages). Although you could get rid of all those functions (animation(), myself(), …) and simply call show(...) directly?

If your code does, what you want it to do, it's fine. I'd just suggest you use what I just wrote in the paragraph above.