current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

I have this code currently written to help me create a menu. I was just wondering if there is a way to tidy this so that there is less code needed to achieve the same thing.

The code is designed to create large dropdowns which have divs inside which will contain most of the content. I want to hide parts of the menu so that the page doesn't become crowded so Its set to hide the other dropdown containers.

$(document).ready(function() {

  $("#1_open").click(function() {
    $("#background_container").slideToggle((500, 'easeInOutExpo'), function() {
      var background = $(this).css('display');
      if (background == 'none') {
        $('#1_arrow').attr('src', './resources/images/icons/right.png');
        $('#background, #colourscheme , #description , #logo , #products , #links , #contact').css('display', 'block');
      } else {
        $('#1_arrow').attr('src', './resources/images/icons/left.png');
        $('#colourscheme , #description , #logo , #products , #links , #contact').css('display', 'none');
      }
    });
  });
  $("#2_open").click(function() {
    $("#colour_container").slideToggle((500, 'easeInOutExpo'), function() {
      var colour = $(this).css('display');
      if (colour == 'none') {
        $('#2_arrow').attr('src', './resources/images/icons/right.png');
        $('#background, #description , #logo , #products , #links , #contact').css('display', 'block');
      } else {
        $('#2_arrow').attr('src', './resources/images/icons/left.png');
        $('#background, #description , #logo , #products , #links , #contact').css('display', 'none');
      }
    });
  });
  $("#3_open").click(function() {
    $("#description_container").slideToggle((500, 'easeInOutExpo'), function() {
      var description = $(this).css('display');
      if (description == 'none') {
        $('#3_arrow').attr('src', './resources/images/icons/right.png');
        $('#background, #colourscheme , #logo , #products , #links , #contact').css('display', 'block');
      } else {
        $('#3_arrow').attr('src', './resources/images/icons/left.png');
        $('#background, #colourscheme , #logo , #products , #links , #contact').css('display', 'none');
      }
    });
  });
  $("#4_open").click(function() {
    $("#logo_container").slideToggle((500, 'easeInOutExpo'), function() {
      var logo = $(this).css('display');
      if (logo == 'none') {
        $('#4_arrow').attr('src', './resources/images/icons/right.png');
        $('#background, #colourscheme , #description , #products , #links , #contact').css('display', 'block');
      } else {
        $('#4_arrow').attr('src', './resources/images/icons/left.png');
        $('#background, #colourscheme , #description , #products , #links , #contact').css('display', 'none');
      }
    });
  });
  $("#5_open").click(function() {
    $("#products_container").slideToggle((500, 'easeInOutExpo'), function() {
      var products = $(this).css('display');
      if (products == 'none') {
        $('#5_arrow').attr('src', './resources/images/icons/right.png');
        $('#background, #colourscheme , #description , #logo , #links , #contact').css('display', 'block');
      } else {
        $('#5_arrow').attr('src', './resources/images/icons/left.png');
        $('#background, #colourscheme , #description , #logo , #links , #contact').css('display', 'none');
      }
    });
  });
  $("#6_open").click(function() {
    $("#contact_container").slideToggle((500, 'easeInOutExpo'), function() {
      var contact = $(this).css('display');
      if (contact == 'none') {
        $('#6_arrow').attr('src', './resources/images/icons/right.png');
        $('#background, #colourscheme , #description , #logo , #products , #links').css('display', 'block');
      } else {
        $('#6_arrow').attr('src', './resources/images/icons/left.png');
        $('#background, #colourscheme , #description , #logo , #products , #links').css('display', 'none');
      }
    });
  });
  $("#7_open").click(function() {
    $("#links_container").slideToggle((500, 'easeInOutExpo'), function() {
      var links = $(this).css('display');
      if (links == 'none') {
        $('#7_arrow').attr('src', './resources/images/icons/right.png');
        $('#background, #colourscheme , #description , #logo , #products , #contact').css('display', 'block');
      } else {
        $('#7_arrow').attr('src', './resources/images/icons/left.png');
        $('#background, #colourscheme , #description , #logo , #products , #contact').css('display', 'none');
      }
    });
  });


});
<div id="background">
  <div class="header" id="1_open">
    <img src="./resources/images/icons/right.png" class="arrow" id="1_arrow">Background</div>
  <div id="background_container" class="containers" style="display:none;"></div>
</div>

<div id="colourscheme">
  <div class="header" id="2_open">
    <img src="./resources/images/icons/right.png" class="arrow" id="2_arrow">Colour Scheme</div>
  <div id="colour_container" class="containers" style="display:none;"></div>
</div>
<div id="description">
  <div class="header" id="3_open">
    <img src="./resources/images/icons/right.png" class="arrow" id="3_arrow">Description</div>
  <div id="description_container" class="containers" style="display:none;"></div>
</div>
<div id="logo">
  <div class="header" id="4_open">
    <img src="./resources/images/icons/right.png" class="arrow" id="4_arrow">Logo and Icon</div>
  <div id="logo_container" class="containers" style="display:none;"></div>
</div>

<div id="products">
  <div class="header" id="5_open">
    <img src="./resources/images/icons/right.png" class="arrow" id="5_arrow">Products</div>
  <div id="products_container" class="containers" style="display:none;"></div>
</div>

<div id="contact">
  <div class="header" id="6_open">
    <img src="./resources/images/icons/right.png" class="arrow" id="6_arrow">Contact Information</div>
  <div id="contact_container" class="containers" style="display:none;"></div>
</div>

<div id="links">
  <div class="header" id="7_open">
    <img src="./resources/images/icons/right.png" class="arrow" id="7_arrow">Links</div>
  <div id="links_container" class="containers" style="display:none;"></div>
</div>

share|improve this question

migrated from stackoverflow.com Jul 7 at 16:50

This question came from our site for professional and enthusiast programmers.
    
have you tried using css? –  kainlite Jun 22 at 0:36

2 Answers 2

up vote 2 down vote

You are repeating yourself too much. Not DRY at all. Read about Dry Principals

This snippet here could easily be extracted into a function, that then could be reused everywhere.

var background = $(this).css('display');
  if (background == 'none') {
    $('#1_arrow').attr('src', './resources/images/icons/right.png');
    $('#background, #colourscheme , #description , #logo , #products , #links , #contact').css('display', 'block');
  } else {
    $('#1_arrow').attr('src', './resources/images/icons/left.png');
    $('#colourscheme , #description , #logo , #products , #links , #contact').css('display', 'none');
  }

You could create a function that would take as parameters everything that differ from each usage. Example-

function doStuff(selector, image1, image2, otherParam) {
    var background = $(this).css('display');
  if (background == 'none') {
    $(selector).attr('src', image1);
    $('#background, #colourscheme , #description , #logo , #products , #links , #contact').css('display', 'block');
  } else {
    $(selector).attr('src', image2);
    $('#colourscheme , #description , #logo , #products , #links , #contact').css('display', 'none');
  }
}
share|improve this answer
up vote 0 down vote

If you see a little your code, you will see that there are excessive repeated selectors, which (in most cases) means something goes wrong. Thinking how to improve it, finally I rewrote the entire code. The result (I hope it works):

$(function(){
    var targets = ['background', 'colour', 'description', 'logo', 'products', 'links', 'contact']; // containers
    var $elements = $('#background, #colourscheme, #description, #logo, #products, #links, #contact'); // jQuery elements
    var i, x; // we use `i` for the array and `x` for the event selector
    for( i = 0, x = 1; i < targets.length; i++ ){
        $('#' + x + '_open').on('click', function(){
            $('#' + targets[i] + '_container').slideToggle((500, 'easeInOutExpo'), function(){
                var visible; // will store the name of the method (show/hide)
                if( !$(this).is(':visible') ){
                    $('#' + x + '_arrow').attr('src', './resources/images/icons/right.png');
                    visible = 'show';
                }
                else{
                    $('#' + x + '_arrow').attr('src', './resources/images/icons/left.png');
                    visible = 'hide';
                }
                switch( x ){ // since the code is (almost) the same to all elements, we finally use the method which name was stored in `visible` variable
                    case 1: visible == 'show' ? $elements.show() : $elements.not('#background').hide(); break; // this case is the only one that missmatch, so we use directly the method to show/hide
                    case 2: $elements.not('#colourscheme')[visible](); break; // the rest of cases had the same syntax so it is easy to solve
                    case 3: $elements.not('#description')[visible](); break;
                    case 4: $elements.not('#logo')[visible](); break;
                    case 5: $elements.not('#products')[visible](); break;
                    case 6: $elements.not('#contact')[visible](); break;
                    case 7: $elements.not('#links')[visible](); break;
                }
            });
        });
        x++;
    }
});

The first thing was to assign the names of the targets containers into an array and the elements into a jQuery object. Since we are working with an "ordered" identifiers (#1_open, #2_open...) we can assign the onclick event with a loop easily and work into it.

Since the code is almost the same, we just need to identify our current target x and proceed at convenience. There are more ways to do it, but well, this is just another one.

share|improve this answer

Your Answer

 
discard

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