0
\$\begingroup\$
$('#step-holder a').click(function (e) {
    e.preventDefault();
    $('#step-holder div').each(function () {
        if ($(this).attr('class') == 'step-dark-left') {
            $(this).removeClass('step-dark-left');
            $(this).addClass('step-light-left');
        }
        if ($(this).attr('class') == 'step-dark-right') {
            $(this).removeClass('step-dark-right');
            $(this).addClass('step-light-right');
        }
        if ($(this).attr('class') == 'step-no') {
            $(this).removeClass('step-no');
            $(this).addClass('step-no-off');
        }
        if ($(this).attr('class') == 'step-dark-round') {
            $(this).removeClass('step-dark-round');
            $(this).addClass('step-light-round');
        }
    });
    if ($(this).parent().attr('class') == 'step-light-left') $(this).parent().attr('class', 'step-dark-left');
    if ($(this).parent().next().attr('class') == 'step-light-right') $(this).parent().next().attr('class', 'step-dark-right');
    if ($(this).parent().next().attr('class') == 'step-dark-round') $(this).parent().next().attr('class', 'step-light-round');
    if ($(this).parent().next().attr('class') == 'step-light-round') $(this).parent().next().attr('class', 'step-dark-round');
    $(this).parent().prev().attr('class', 'step-no');
});
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

One quick refactoring jumps out at me. Start out your each with:

$('#step-holder div').each(function () {
    var $this = $(this);

Every time you call it (possibly 12 times?) its performing a lookup ... you should look up that object once, store the result, and then reuse it.

I'm going to assume each of these objects has only one class -- otherwise your .attr('class') test would fail (by the way, use .hasClass('x') to test so if it has multiple classes you can still tell if it has that class). Based on that assumption, you can shorten the first few if statements like this:

$('#step-holder div').each(function () {
    var $this = $(this);

    if( $this.hasClass('step-dark-left') ) {
        $this.attr('class','step-left-light');
    }
    else if( $this.hasClass('step-dark-right') ) {
        $this.attr('class','step-light-right');
    } 
    ... etc ...
});

I also suggest you use if ... else if ... else if ... etc because only one of those can be true -- no reason to evaluate them all. Again, this assumption is based on your code, and I'm reading a lot into your class test, when it could in fact be an error on your part.

\$\endgroup\$
2
  • \$\begingroup\$ thx erik, i have applied your changes ( beside the linked if's, i dont like the visual look of it ) \$\endgroup\$ Jul 17, 2011 at 8:43
  • \$\begingroup\$ ... it has nothing to do with visual look. You're executing branches needlessly. If only one of those conditions can be true then multiple IF statements are not the best way of handling it. \$\endgroup\$
    – Erik
    Jul 18, 2011 at 13:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.