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.

I've written some jQuery code that works, but I'm a novice in jQuery and I want to know how to improve my code if possible.

I know I have 3 near identical blocks of code. If I create a function, the code will be the same. I'm looking for more accurate code and am trying to reduce my code into fewer lines.

JSFiddle

    $('#click-me-idiomas').click(function () {
        var idiomasVisible = $("#menu-idiomas").is(":visible");
        var coleccionesVisible = $("#menu-colecciones").is(":visible");
        var nosotrosVisible = $("#menu-nosotros").is(":visible");
        if (coleccionesVisible || nosotrosVisible) {
            if (coleccionesVisible) {
                $('#click-me-colecciones').removeClass('active');
                $('#click-me-idiomas').addClass('active');
                $("#menu-colecciones").slideUp('slow', function () {
                    $('#menu-idiomas').slideToggle('slow');
                });
            }
            if (nosotrosVisible) {
                $('#click-me-nosotros').removeClass('active');
                $('#click-me-idiomas').addClass('active');
                $("#menu-nosotros").slideUp('slow', function () {
                    $('#menu-idiomas').slideToggle('slow');
                });
            }
        } else {
            if (idiomasVisible) {
                $("#menu-idiomas").slideToggle('slow', function () {
                    $('#click-me-idiomas').removeClass('active');
                });
            } else {
                $('#click-me-idiomas').addClass('active');
                $("#menu-idiomas").slideToggle('slow');
            }
        }

    });

    $('#click-me-colecciones').click(function () {
        var idiomasVisible = $("#menu-idiomas").is(":visible");
        var coleccionesVisible = $("#menu-colecciones").is(":visible");
        var nosotrosVisible = $("#menu-nosotros").is(":visible");
        if (idiomasVisible || nosotrosVisible) {
            if (idiomasVisible) {
                $('#click-me-idiomas').removeClass('active');
                $('#click-me-colecciones').addClass('active');
                $("#menu-idiomas").slideUp('slow', function () {
                    $('#menu-colecciones').slideToggle('slow');
                });
            }
            if (nosotrosVisible) {
                $('#click-me-nosotros').removeClass('active');
                $('#click-me-colecciones').addClass('active');
                $("#menu-nosotros").slideUp('slow', function () {
                    $('#menu-colecciones').slideToggle('slow');
                });
            }
        } else {
            if (coleccionesVisible) {
                $("#menu-colecciones").slideToggle('slow', function () {
                    $('#click-me-colecciones').removeClass('active');
                });
            } else {
                $('#click-me-colecciones').addClass('active');
                $("#menu-colecciones").slideToggle('slow');
            }
        }

    });

    $('#click-me-nosotros').click(function () {
        var idiomasVisible = $("#menu-idiomas").is(":visible");
        var coleccionesVisible = $("#menu-colecciones").is(":visible");
        var nosotrosVisible = $("#menu-nosotros").is(":visible");
        if (idiomasVisible || coleccionesVisible) {
            if (idiomasVisible) {
                $('#click-me-idiomas').removeClass('active');
                $('#click-me-nosotros').addClass('active');
                $("#menu-idiomas").slideUp('slow', function () {
                    $('#menu-nosotros').slideToggle('slow');
                });
            }
            if (coleccionesVisible) {
                $('#click-me-colecciones').removeClass('active');
                $('#click-me-nosotros').addClass('active');
                $("#menu-colecciones").slideUp('slow', function () {
                    $('#menu-nosotros').slideToggle('slow');
                });
            }
        } else {
            if (nosotrosVisible) {
                $("#menu-nosotros").slideToggle('slow', function () {
                    $('#click-me-nosotros').removeClass('active');
                });
            } else {
                $('#click-me-nosotros').addClass('active');
                $("#menu-nosotros").slideToggle('slow');
            }
        }

    });
share|improve this question
1  
You have 3 near identical blocks of code. You should create one common function that you can pass a few parameters to and then just call that common function from each of your event handlers. –  jfriend00 Jul 3 at 9:22
1  
@jfriend00 post that as an answer... you'd get upvotes for sure. just flesh it a little. –  Vogel612 Jul 3 at 9:36
    
Since the language thing has been pointed out in an answer, I've rolled back the edit (while retaining and fixing your added explanation). –  Jamal Jul 10 at 12:46
add comment

2 Answers

I've written in my way. You can see it with this link http://jsfiddle.net/968pA/8/ .

HTML :

<header id="masthead" class="container site-header" role="banner">
  <nav role="navigation">
    <ul class="menu">
      <li><a href="#" id="click-me-languages" data-toggle="menu-languages">LANGUAGES</a></li>
      <li><a href="#" id="click-me-books" data-toggle="menu-books">BOOKS</a></li>
      <li><a href="#" id="click-me-about" data-toggle="menu-about">ABOUT US</a></li>
    </ul>
  </nav>
</header>
<div class="container">
  <div class="menu-list" id="menu-languages">
    <div class="row clearfix">
      <div class="col-3">
        <ul>
          <li><a href="">English</a></li>
          <li><a href="">Spanish</a></li>
          <li><a href="">Français</a></li>
          <li><a href="">Deutsch</a></li>
        </ul>
      </div>
    </div>
  </div>
  <div class="menu-list" id="menu-books">
    <div class="row clearfix">
      <div class="col-3">
        <ul>
          <li><a href="">Book #1</a></li>
          <li><a href="">Book #2</a></li>
          <li><a href="">Book #3</a></li>
          <li><a href="">Book #4</a></li>
          <li><a href="">Book #5</a></li>
        </ul>
      </div>
    </div>
  </div>
  <div class="menu-list" id="menu-about">
    <div class="row clearfix">
      <div class="col-3">
        <ul>
          <li><a href="">About us</a></li>
          <li><a href="">Contact</a></li>
          <li><a href="">Blog</a></li>
        </ul>
      </div>
    </div>
  </div>
</div>

CSS (same as original) :

* { 
    -webkit-box-sizing: border-box; /* Safari/Chrome, other WebKit */
    -moz-box-sizing: border-box;    /* Firefox, other Gecko */
    box-sizing: border-box;         /* Opera/IE 8+ */
}
body {margin:0; padding:0;}
a {text-decoration:none;}
.container { padding-left:15px; padding-right:15px; margin:0 auto; max-width:1170px; }
.col-3 { width:285px; }

.col-3 {
    float:left;
    position: relative;
    min-height: 1px;
    padding-right: 15px;
    padding-left: 15px;
}

.site-header { background-color:#00AF85; }
.site-header li { display:inline; font-family:'Open Sans', sans-serif; font-weight:700; line-height:70px; padding-right:18px; }
.site-header a { color:#fff; }
.site-header a:hover,
.site-header a.active { color:#403F41; }

#menu-languages,
#menu-books,
#menu-about { background-color:#fff; display:none; font-family:'Open Sans', sans-serif; font-size:16px; font-weight:700; text-transform:uppercase; }

#menu-languages .col-12, #menu-books .col-12, #menu-about .col-12 { height:50px; }

#menu-languages li, #menu-books li, #menu-about li { padding-bottom:5px; padding-top:5px; }
#menu-languages a, #menu-books a, #menu-about a { line-height:20px; }

.clearfix:after {
  content: "";
  display: table;
  clear: both;
}

JS :

function getActiveLink() {
    var $activeLink = $('#masthead').find('a.active:first');
    return ($activeLink.length > 0)? $activeLink: null;
}

function getActiveMenu() {
    var $activeMenu = $('.menu-list:visible:first');
    return ($activeMenu.length > 0)? $activeMenu: null;
}

$('#click-me-languages, #click-me-books, #click-me-about').click(function (event) {
    var $thisLink = $(event.currentTarget);
    var menu = $thisLink.data('toggle');
    var $thisMenu = $('#' + menu);
    var $activeLink = getActiveLink();
    var $activeMenu = getActiveMenu();
    if ($thisMenu.is($activeMenu)) {
        $activeLink.removeClass('active');
        $activeMenu.slideUp('slow');
    }else if ($activeMenu === null) {
        $thisLink.addClass('active');
        $thisMenu.slideDown('slow');        
    }else {
        $activeMenu.slideUp('slow', function() {
            $activeLink.removeClass('active');
            $thisLink.addClass('active');
            $thisMenu.slideDown('slow');
        });
    }
});

I tried to remove duplicated code and merge the events to one, So I need to get menu and link that being active, then I can do whatever to the menu and the link. My code isn't the best but I think you can learn from it. I added class to div menus and added data-toggle to links so that I can get active link and active menu easier.

share|improve this answer
    
Links can go away or temporarily fail. Please add the code to your answer. –  ckuhn203 Jul 10 at 11:49
    
I like it, but: there doesn't seem to be much point in adding the data-toggle. It could be renamed to something more meaningful (data-menu-id?), or it could be removed altogether. In the latter case, replace with var menu = $thisLink.attr("id").substr("click-me-".length);. In fact, the menu variable is used only once, and it isn't a complicated calculation, nor does it really add clarity to the code. The expression could be pasted directly into the selector: var $thisMenu = $("#menu-" + $thisLink.attr("id").substr("click-me-".length));. –  Schism Jul 10 at 15:33
    
Kamonwat Sangudsub thanks a lot!!!! And Schism, thanks to you too. I will take your tip and I will put it on the code :) –  user3800667 Jul 11 at 9:15
add comment

Right, wrong, or indifferent, most of the world codes in English. I would recommend you do the same. I can understand wanting to code in your native language though. Learning either English or a Programming language is quite a bit to take on by themselves, let alone in combination.

As @jfriend00 pointed out, there's quite a bit of code duplication here. You can DRY this up by introducing another function. As a general rule of thumb, anytime you reach for copy & paste you should stop yourself and write a function to do it. We're programmers. We're lazy. If we need to make a change to a piece of code, we want to make that change just once.

share|improve this answer
    
I tried to provide a code sample, but it proved to be just too difficult to read two languages that I don't know. –  ckuhn203 Jul 8 at 12:56
    
Hi ckuhn203, I've translated the code to english. Is that ok? –  user3800667 Jul 10 at 10:21
add comment

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.