Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Those functions are selected by the same index, I understand the index logic of what to do but I don't know how to do it well enough. Please give me a hand!

        // those are buttons that will trigger different functions according by an index
        button_up = jQuery('#videos_tabs-up-0'  );
        button_down = jQuery('#videos_tabs-down-0' );
        button_up_1 = jQuery('#videos_tabs-up-1'  );
        button_down_1 = jQuery('#videos_tabs-down-1' );
        button_up_2 = jQuery('#videos_tabs-up-2'  );
        button_down_2 = jQuery('#videos_tabs-down-2' );



        // this one stores a custom number
            videos_visible = 3;

           // there are 3 .ov_front_video div, see I select each one by its index
        videos_list = jQuery('#ov_front_videos .ov_front_video:eq(0) .item-list ul');
        videos_list_item = videos_list.find('li');
        videos_list_item_length = videos_list_item.length;
        videos_visible_limit = videos_list_item_length - videos_visible;

        videos_list_1 = jQuery('#ov_front_videos .ov_front_video:eq(1) .item-list ul');
        videos_list_item_1 = videos_list_1.find('li');
        videos_list_item_length_1 = videos_list_item_1.length;
        videos_visible_limit_1 = videos_list_item_length_1 - videos_visible;

        videos_list_2 = jQuery('#ov_front_videos .ov_front_video:eq(2) .item-list ul');
        videos_list_item_2 = videos_list_2.find('li');
        videos_list_item_length_2 = videos_list_item_2.length;
        videos_visible_limit_2 = videos_list_item_length_2 - videos_visible;


        // each .ov_front_video (videos_list, videos_list_1, videos_list_2) animates pressing any of the previous buttons,  
        // I'm setting a variable for each .ov_front_video.index
        k =1;
        k_1 =1;
        k_2 =1;




   // The next functions animates videos_list when button_up is pressed, 
   // videos_list_1 when button_up_1 is pressed, etc ...
                    /* --------------------
                click tab 0
                ------------------------ */
             button_up.click(function() {
                     current_position = videos_list.position().top;
                        current_position = current_position + 70
                     videos_list.not(':animated').stop().animate({'top':current_position}, function() {k -= 1;});

                        console.log(k);

                        if (k == 2 ) {
                 jQuery('#videos_tabs-up-0').hide();
            }
                        jQuery('#videos_tabs-down-0').show();
                });         

             button_down.click(function() {
                     current_position = videos_list.position().top;
                        current_position = current_position - 70
                     videos_list.not(':animated').stop().animate({'top':current_position}, function() {k += 1;});

                        console.log(k);

                        if (k == videos_visible_limit ) {
                 jQuery('#videos_tabs-down-0').hide();
            }
                        jQuery('#videos_tabs-up-0').show();
                });         


                /* --------------------
                click tab 1
                ------------------------ */
             button_up_1.click(function() {
                     current_position = videos_list_1.position().top;
                        current_position = current_position + 70
                     videos_list_1.not(':animated').stop().animate({'top':current_position}, function() {k_1 -= 1;});

                        console.log(k_1);

                        if (k_1 == 2 ) {
                 jQuery('#videos_tabs-up-1').hide();
            }
                        jQuery('#videos_tabs-down-1').show();
                });         

             button_down_1.click(function() {
                     current_position = videos_list_1.position().top;
                        current_position = current_position - 70
                     videos_list_1.not(':animated').stop().animate({'top':current_position}, function() {k_1 += 1;});

                        console.log(k_1);

                        if (k_1 == videos_visible_limit_1 ) {
                 jQuery('#videos_tabs-down-1').hide();
            }
                        jQuery('#videos_tabs-up-1').show();
                });     

                /* --------------------
                click tab 2
                ------------------------ */
             button_up_2.click(function() {
                     current_position = videos_list_2.position().top;
                        current_position = current_position + 70
                     videos_list_2.not(':animated').stop().animate({'top':current_position}, function() {k_2 -= 1;});

                        console.log(k_2);

                        if (k_2 == 2 ) {
                 jQuery('#videos_tabs-up-2').hide();
            }
                        jQuery('#videos_tabs-down-2').show();
                });         

             button_down_2.click(function() {
                     current_position = videos_list_2.position().top;
                        current_position = current_position - 70
                     videos_list_2.not(':animated').stop().animate({'top':current_position}, function() {k_2 += 1;});

                        console.log(k_2);

                        if (k_2 == videos_visible_limit_2 ) {
                 jQuery('#videos_tabs-down-2').hide();
            }
                        jQuery('#videos_tabs-up-2').show();
                });     
share|improve this question

1 Answer 1

up vote 1 down vote accepted

You can compact this code a lot by generalising the click listener.

In the html assign a class to each video_tab, but keep the ids as they are. For instance I assigned each video tab a video_button class.

<div id="videos_tabs-up-0" class="video_button">
  Button: #videos_tabs-up-0
</div>
<div id="videos_tabs-down-0" class="video_button">
  Button: #videos_tabs-down-0
</div>
<div id="videos_tabs-up-1" class="video_button">
  Button: #videos_tabs-up-1
</div>
<div id="videos_tabs-down-1" class="video_button">
  Button: #videos_tabs-down-1
</div>
<div id="videos_tabs-up-2" class="video_button">
  Button: #videos_tabs-up-2
</div>
<div id="videos_tabs-down-2" class="video_button">
  Button: #videos_tabs-down-2
</div>

So now in the jQuery, we can assign just one click listener, listening for clicks on .video_button.

This method handles all possible 'video_tabs-xxx' clicks: up, down, for each of the indexes 0, 1, 2.

Now, clicks can come from any of the 'video_tabs-xxx' ids, so the first thing we need to do is assign some variables based on the origin of the click.

event.target.id contains the id string of the originating element. This is very handy.

Your naming convention contained 'up' and 'down' in the id, so using string searching we can get whether this is an UP or DOWN click and store that in direction.

The index is supplied in the last character of the id name. Let's parse that as an int into the variable idx.

Your k variables seemed to be tracking whether an animation ir running or not*. A simpler way is an array of booleans, where idx will give us a key into the array.

And the offset variable is assigned + or - 70 depending on whether this is an UP or DOWN click.

So that leaves this single listener:

var UP = 0,
    DOWN = 1,
    animating = [false, false, false];

$(document).ready(function () {
    $('.video_button').click(function (event) {
        var idx = parseInt(event.target.id[event.target.id.length-1], 10),
            direction = event.target.id.indexOf('up') > -1 ? UP : DOWN,
            offset = direction === UP ? 70 : -70,
            isAnimating = direction === UP ? true : false,
            videos_list,
            current_position;

        console.log('video_button clicked: ' + event.target.id + ' idx: ' +
            idx + ' direction: ' + direction + ' offset: ' + offset +
            ' isAnimating: ' + isAnimating);

        videos_list = $('#ov_front_videos .ov_front_video:eq(' +
            idx + ') .item-list ul');

        current_position = videos_list.position().top;
        current_position = current_position + offset;
        videos_list.not(':animated').stop().animate({'top':current_position},
            function() {animating[idx] = isAnimating});

        if (direction === UP) {
            jQuery('#videos_tabs-down-' + idx).show();
            if (animating[idx]) {
                jQuery('#videos_tabs-up-' + idx).hide();
            }
        }
        else {
            jQuery('#videos_tabs-up-' + idx).show();
            if (animating[idx]) {
                jQuery('#videos_tabs-down-' + idx).hide();
            }
        }
    });
});

*: not entirely clear what your k variables were measuring, but hopefully it's straightforward to fit that logic in the single listener.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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