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 have the following code to get the ID of a <a href="#ID"> </ a> and go to their respective div <div id="" />:

$('a[href=#certificados]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#certificados').offset().top }, 1000);
});

$('a[href=#team]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#team').offset().top }, 1000);
});

$('a[href=#house]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#house').offset().top }, 1000);
});

$('a[href=#contact]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#contact').offset().top }, 1000);
});

How could I optimize the code?

share|improve this question

migrated from stackoverflow.com Jan 14 at 13:57

This question came from our site for professional and enthusiast programmers.

4 Answers 4

up vote 11 down vote accepted

Get the "respective" div in a more general way from the link's href attribute:

$('a[href=#certificados], a[href=#team], a[href=#house], a[href=#contact]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $(this.hash).offset().top }, 1000);
    // this.hash would be equivalent to $(this).attr("href") in your case
});

Probably you also can use a much better selector now - maybe selecting those links by a common class.

share|improve this answer
3  
Bad thing about this solution is maintaining the list of items and the slower look-up. You would be better off using a common classname. –  epascarello Jan 14 at 14:25
3  
@epascarello: That's what I suggested in the last sentence :-) –  Bergi Jan 14 at 14:36
    
Just hoping the OP saw it. :) –  epascarello Jan 14 at 14:46

If you want to make the code DRYer, you can do

['#certificados', '#team', '#house', '#contact'].forEach(function(anchor){
    $("a[href="+anchor+"]").click(function(e){
        e.preventDefault();
        $("html, body").animate({ scrollTop: $(anchor).offset().top }, 1000);
    });
})

If all internal links are to be handled this way, do

$('a[href^="#"]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $(this.hash).offset().top }, 1000);
});
share|improve this answer
    
Why a forEach? jQuery supports multiple elements in a selector! –  epascarello Jan 14 at 14:01
    
@epascarello to not compute n and to be dryer on the a[href=...] thing –  dystroy Jan 14 at 14:01
    
Maybe rename n to anchor ?, +1 –  konijn Jan 14 at 14:04
1  
this.href does not work –  Bergi Jan 14 at 14:14
1  
this.href returns the full URL, You would need to use this.getAttribute("href") or $(this).attr("href"); or hash jsfiddle.net/7439C/1 –  epascarello Jan 14 at 14:22

You can make an extension, that will let you specify what link scrolls to where, without creating a hard dependancy between the href attribute and the target identity:

$.fn.scrollTo = function(target){
  return this.click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $(target).offset().top }, 1000);
  });
};

$('a[href=#certificados]').scrollTo('#certificados');
$('a[href=#team]').scrollTo('#team');
$('a[href=#house]').scrollTo('#house');
$('a[href=#contact]').scrollTo('#contact');
share|improve this answer
    
Nice, but this doesn't optimize it at all. –  Silviu Burcea Jan 14 at 22:36
    
@SilviuBurcea: Funny that you should single out this answer and point out that it won't optimise the code (and I assume that you only mean optimise for speed, not any other aspect), as neither of the previous answers does that either... –  Guffa Jan 14 at 23:13

Add a class to the anchors, like scrollable.

$('.scrollable').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $(this.hash).offset().top }, 1000);
});

Each is called under the hood, no crazy functions, just KISS.

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.