Improving Code pointers

Question

I'm new to jQuery and I've been messing about with this code, It works but I want to learn how to shorten the code by the eliminating unnecessary repeated code.

Below is a link to jsFiddle

http://jsfiddle.net/eSZFD/168/

Thanks for reading.

HTML

<div id="div1" style="display:none;">one<div id="divb" class="upper">close</div></div>
<div id="div2" style="display:none;">two<div id="divb" class="upper">close</div></div>
<div id="div3" style="display:none;">three<div id="divb" class="upper">close</div></div>
<div id="div4" style="display:none;">four<div id="divb"     class="upper">close</div></div>
</br></br></br></br></br></br></br></br></br></br></br><div class="buttons">
<a class="button" id="showdiv1">Div 1</a>
<a class="button" id="showdiv2">Div 2</a>
<a class="button" id="showdiv3">Div 3</a>
<a class="button" id="showdiv4">Div 4</a>
</div>

JQuery

$(function() {
     $('#showdiv1').click(function() {
        $('html, body').animate({ scrollTop: 0 }, 'slow');
         $('div[id^=div]').slideUp().delay(1000);
          $('#div1').slideDown('slow').delay(1000);

    });

      $('.upper').click(function() {
      $('html, body').animate({ scrollTop: 0 }, 'slow');
       $('#div1').slideUp('slow');
    });

    $('#showdiv2').click(function() {
        $('html, body').animate({ scrollTop: 0 }, 'slow');
         $('div[id^=div]').slideUp().delay(1000);
          $('#div2').slideDown('slow').delay(1000);

    });

     $('.upper').click(function() {
      $('html, body').animate({ scrollTop: 0 }, 'slow');
       $('#div2').slideUp('slow');
    });


     $('#showdiv3').click(function() {
        $('html, body').animate({ scrollTop: 0 }, 'slow');
         $('div[id^=div]').slideUp().delay(1000);
          $('#div3').slideDown('slow').delay(1000);

    });

     $('.upper').click(function() {
      $('html, body').animate({ scrollTop: 0 }, 'slow');
       $('#div3').slideUp('slow');
    });

   $('#showdiv4').click(function() {
        $('html, body').animate({ scrollTop: 0 }, 'slow');
         $('div[id^=div]').slideUp().delay(1000);
          $('#div4').slideDown('slow').delay(1000);

    });

     $('.upper').click(function() {
      $('html, body').animate({ scrollTop: 0 }, 'slow');
       $('#div4').slideUp('slow');
    });

})

Ok, after much headache - I'm new to this sh#t. I've changed the code to this. All works fine. I would like someone that knows, to give me any tips for improvement, as I'm totally improvising here, thankyou.

jquery

$(document).ready(function(){

    $('a').click(function () {
    var divname= this.name;
    $('html, body').animate({ scrollTop: 0 }, 'slow');
    $('div[id^=div]').slideUp().delay(1000);
      $("#"+divname).slideDown('slow').delay(1000);

    });

     $('.upper').click(function() {
    $('html, body').animate({ scrollTop: 0 }, 'slow');
    $('div[id^=div]').slideUp('slow');

  });
    });

html

<body>


<div id="div1" style="display:none">
Hello World  <a class="upper" id="div1">close</a>
</div>

<div id="div2" style="display:none">
Test<a class="upper" id="div2">close</a>
</div>

<div id="div3" style="display:none">
Another Test
</div>

<div id="div4" style="display:none">
Final Test
</div>

</br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br></br>
<div class="menu">
<ul>
<li><a href="#" name="div1" >one</a></li>
<li><a href="#" name="div2" >two</a></li>
<li><a href="#" name="div3" >three</a></li>
<li><a href="#" name="div4" >four</a></li>
</ul>
</div>



</body>

Hi and welcome to Code Review. Please post your code in the question as well. Also make sure to check this out: codereview.stackexchange.com/faq
@Yuck: You can't post people code for them. As this page gives away rights via collective comments. You have to ask them to post their own code.
@Yuck: Obviously it was a well-intentioned edit but Loki's right, and here are additional reasons why.

Daniel Imms · Accepted Answer · 2013-03-22 02:05:11Z

View all my changes on this jsFiddle.

HTML

Use the self-closing tag <br /> not the closing tag </br>.
Your close buttons (.upper) are doing a very similar function to your <a> tags below, the should both be <a> tags. The reason why I wouldn't make them <div>s is because you would need to manually add a tab-index to enable keyboard accessibility, links come with it.
I'd use a HTML5 data- attribute instead of ID to specify the id of the div you're showing.
No need to include an id on the close buttons.

You can add a class to your 'sections' so they can be targeted together.

<div id="div1" class="section">
    one
    <a class="hide-div">close</a>
</div>
<div id="div2" class="section">
    two
    <a class="hide-div">close</a>
</div>
<div id="div3" class="section">
    three
    <a class="hide-div" href="#">close</a>
</div>
<div id="div4" class="section">
    four
    <a class="hide-div" href="#">close</a>
</div>

<br /><br /><br /><br /><br /><br /><br /><br /><br />

<div class="buttons">
    <a class="show-div" data-div="div1" href="#">Div 1</a>
    <a class="show-div" data-div="div2" href="#">Div 2</a>
    <a class="show-div" data-div="div3" href="#">Div 3</a>
    <a class="show-div" data-div="div4" href="#">Div 4</a>
</div>

CSS

Put the display:none; style into your CSS.

.section {
    height:600px;
    display:none;
}

.hide-div {
    display:block;
}

JavaScript

Target the .button class, not a. I would also rename .button to something a little more descriptive like .show-div
.upper isn't very descriptive, something like .hide-div would be better.
You should only hide and delay the sections if there are any, this gives the user a weird second long pause if there are no sections visible. I've used the .visible class to aid with this.

Pulled the hide sections code out because it's used in multiple places.

$(document).ready(function () {
    $('.show-div').click(function () {
        var divId = this.getAttribute('data-div');
        $('html, body').animate({
            scrollTop: 0
        }, 'slow');

        hideSections();

        $("#" + divId)
            .slideDown('slow')
            .delay(1000)
            .addClass('visible');
    });

    $('.hide-div').click(function () {
        $('html, body').animate({
            scrollTop: 0
        }, 'slow');
        hideSections();
    });
});

function hideSections() {
    if ($('.visible').length > 0) {
        $('.section')
            .slideUp()
            .delay(1000)
            .removeClass('visible');
    }
}

Thank you Tyriar and Back in a Flash, very helpful. Lots of great tips, I'm very pleased. May the learning curve continue.

hjpotter92 · Answer 2 · 2013-03-21 23:09:43Z

Your newer code is much much better than the previous attempt. Some little fixes that I'd suggest are:

Don't use the following
```
$('div[id^=div]').slideUp().delay(1000);
```
as it hides the close button inside the div. So, if I open div1 and click the div1 again, I can't see the close button there.
Since you are continually using jQuery, I'd prefer to use .attr() method to fetch the name instead of
```
this.name;
```
that you have used inside your new code.
You must not bind your .click() to the a tag. Instead you already have class="button" assigned to them. Use it.

Here is an updated fiddle link that addresses the issues and fixes them.

jQuery Code

$(function () {
    var newId = "";
    $('.button').click(function () {
        $('html, body').animate({
            scrollTop: 0
        }, 'slow');
        if (newId !== "") $('#' + newId).slideUp().delay(1000);
        newId = $(this).attr("name");
        $('#' + newId).slideDown('slow').delay(1000);

    });
    $('.upper').click(function () {
        $('html, body').animate({
            scrollTop: 0
        }, 'slow');
        $(this).parent('div').slideUp('slow');
    });
});

I strongly disagree with number 2. Even though he is using jQuery, jQuery is a framework on top of JavaScript. Why take the effort to create a jQuery object and then call a far slower method to produce the EXACT same result when this.name is faster and more accessible.
The language being used is JavaScript though. I understand the importance of code consistency, but there is nothing wrong with using simple object properties when you're using jQuery.
Yup, @Tyriar which is why I mentioned that I'd prefer to use as opposed to you shouldn't.

asked	2 months ago
viewed	68 times
active	2 months ago

Improving Code pointers

HTML

JQuery

html

2 Answers

HTML

CSS

JavaScript

jQuery Code

Your Answer

Not the answer you're looking for? Browse other questions tagged jquery html clean-code or ask your own question.

Improving Code pointers

HTML

JQuery

html

2 Answers

HTML

CSS

JavaScript

jQuery Code

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged jquery html clean-code or ask your own question.

Related