4
\$\begingroup\$

I've just finished building my slider and currently trying to refactor some redundant if/else code. Here is a preview of the code that I have built. Inside the event handlers. I have some if/else that are redundant, the purpose of the condition is to detect if slider reaches the last slide it will hide the next button and if the slider goes to the first slide. It will hide the previous button.

The slider I have built has no looping function.

    var programSliderContainer = $('.programs-slider'),
        programSliderWidth     = $('.programs-slider').outerWidth(),
        sliderContainer        = $('.programs-slider .view-programs-description .view-content'),
        totalSlides            = $('.programs-slider .view-programs-description .view-content .views-row').length,
        slides                 = $('.programs-slider .view-programs-description .view-content .views-row'),
        position               = 0,
        move                   = 0;


    //Reset Slider Position when resizing
    $('.programs-slider .view-programs-description .view-content').css({'right' : 0 });

    //Apply the Maximum width based on the total number of slides
    sliderContainer.width(programSliderWidth * totalSlides);

    //Apply the width of the slides based on the width of programSliderWidth
    slides.outerWidth(programSliderWidth);


    //Add Class active on Start
    $('.program-controls .program-list ul li:first-child').addClass('active');


    //Hide Previous Arrow on Start
    if( position == 0 ) {
        $('.arrow-controls .prev').hide();
    }

    $('.program-controls .program-list ul li').on('click', function() {
        position = $(this).index();
        $('.programs-slider .view-programs-description .view-content').css({'right' : position*programSliderWidth });

        $('.program-controls .program-list ul li.active').removeClass('active');
        $(this).addClass('active');


        //Show Hide Directional Navigation based on Slider Position
        if( position == 0 ) {
            $('.arrow-controls .prev').hide(); 
        }

        else {
            $('.arrow-controls .prev').show(); 
        }

        if( position == totalSlides - 1  ) {
            $('.arrow-controls .next').hide(); 
        }

        else {
            $('.arrow-controls .next').show(); 
        }

    });


    $('.arrow-controls .prev').on('click', function() {
        if( position > 0 ) {
            position--;
            $('.programs-slider .view-programs-description .view-content').css({'right' : position*programSliderWidth });

            $('.program-controls .program-list ul li.active').removeClass('active');
        }

        if( position == 0 ) {
            $('.arrow-controls .prev').hide(); 
        }

        else {
            $('.arrow-controls .prev').show(); 
        }


        if( position == totalSlides - 1 ) {
            $('.arrow-controls .next').hide(); 
        }

        else {
            $('.arrow-controls .next').show(); 
        }

    });

    $('.arrow-controls .next').on('click', function() {
        if( position < totalSlides - 1 ) {
            position++;
            $('.programs-slider .view-programs-description .view-content').css({'right' : position*programSliderWidth });

            $('.program-controls .program-list ul li.active').removeClass('active');
        }


        if( position == 0 ) {
            $('.arrow-controls .prev').hide(); 
        }

        else {
            $('.arrow-controls .prev').show(); 
        }


        if( position == totalSlides - 1 ) {
            $('.arrow-controls .next').hide(); 
        }

        else {
            $('.arrow-controls .next').show(); 
        }

    });

}
\$\endgroup\$
3
\$\begingroup\$

To reduce if/else you might want to introduce a separate function:

function showIf(selector, condition) {
    if (condition === true)
        $(selector).show();
    else
        $(selector).hide();
}

In this way you can simply write:

showIf('.arrow-controls .prev', position === 0);
showIf('.arrow-controls .next', position === totalSlides - 1);

Which, BTW, is what .toggle() does (see Jack's answer). I'd go a step further. You use those conditions extensively: I'd introduce some separate functions:

showIf('.arrow-controls .prev', isFirstSlideSelected());
showIf('.arrow-controls .next', isLastSlideSelected());

Note that if you want you might change showIf() to accept a function as condition and simply write:

showIf('.arrow-controls .prev', isFirstSlideSelected);

Few minor considerations.

You might want to use let instead of var, see also Is there any reason to use the “var” keyword in ES6?.

You may repeat var/let in each line, multiple declarations are sometimes misleading. Not to mention that when debugging and going through the code step by step that code you can inspect each declaration alone. See also Multiple var statements in JavaScript, not superfluous.

Do not align your declarations, visual appealing isn't important when you want to update one of those lines or you introduce a longer variable name...

If you reuse the same selector multiple times (you do) you should introduce a const for each of them, it will make changes in your HTML code much easier to handle.

Comments: your comments actually repeat what you already have in code, they do not explain anything more. Usually when I see/write this type of comments I feel I'm missing the opportunity to extract a separate function. Will you need to write "Show Hide Directional Navigation based on Slider Position" as comment if you call a function updateNavigationControlsVisibility()? Don't be afraid of reasonably long names, your minifier will do the job for you.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

You can use the toggle() function: http://api.jquery.com/toggle/#toggle-display

Before:

if (position == 0) {
    $('.arrow-controls .prev').hide(); 
} else {
    $('.arrow-controls .prev').show(); 
}

After:

$('.arrow-controls .prev').toggle(position != 0);
| improve this answer | |
\$\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.