Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a jQuery function using a series of "if" and "else" statements. I am wondering if there is a more elegant way I can code this. I am thinking I could create and array with my "hashtag names" and then loop it through a function. However I am not quiet sure how to go about doing that. Here is my code...

setTimeout(function() {
          var i = 0;
          if(window.location.hash == "#" + "somename"){
               var i = 0;
               return scroll(i);
          }

          else if(window.location.hash == "#" + "somethingelse"){
               var i = 1;
               return scroll(i);
          }

          else if(window.location.hash == "#" + "someotherword"){
               var i = 2;
               return scroll(i);
          }

          else if (window.location.hash == "#" + "sometext"){
               var i = 3;
               return scroll(i);
          }
     }, 500 );

EDIT: Some more code

function scroll(i){
          var title = $('.title').eq(i);
          var top =  $(title).offset().top - 50 ;
          $('html,body').animate({ scrollTop: top }, 'slow');
     }

Basically what I am doing is checking to see what hashtag is in the URL, and then scrolling to the part of the page that is related to that hashtag. This method works, but I would like to optimize the "if/else" part of my code if possible.

Any help is appreciated. An array with a loop may not be the best approach. I am open to any ideas. Thanks.

share|improve this question

2 Answers

up vote 1 down vote accepted

You can just parse the number right out of the location like this:

setTimeout(function() {
    var matches = window.location.hash.match(/^#page(\d+)$/);
    if (matches) {
        scroll(+matches[1] - 1);
    }
}, 500);

OK, now that you've changed to random strings, you could use a table lookup like this:

setTimeout(function() {
    var values = {
        "#somename": 1,
        "#somethingelse": 2,
        "#someotherword": 3,
        "#sometext": 4
    };
    var val = values[window.location.hash];
    if (val) {
        scroll(val - 1);
    }
}, 500 );

P.S. Note, there is no reason to return anything from the setTimeout() callback as the return value is not processed by anything which is why I removed the return statement.

share|improve this answer
Sorry, I am actually not using numbers... I just updated my question to reflect that. I put that them there as an example I didn't intend for it be like that, my mistake. For my scenario I am actually using different names for each page. – japanFour Aug 13 '12 at 19:37
@KrisHollenbeck - OK, I've now provided a lookup table option for random strings. – jfriend00 Aug 13 '12 at 19:46
Thanks, I will see if I can get it to work with my code and get back to you. – japanFour Aug 13 '12 at 19:54
Hmm.. I am thinking I need to post some more code. That doesn't seem to work with what I have... I will update my question – japanFour Aug 13 '12 at 20:04
FYI.. I get this error when trying to implement the "table lookup" .. Uncaught TypeError: Cannot read property 'top' of null – japanFour Aug 13 '12 at 20:08
show 3 more comments

The book High Performance JavaScript provides some good advice on looping in JavaScript. To quote it:

Generally speaking, if-else is best used when there are two discrete values or a few different ranges of values for which to test. When there are more than two discrete values for which to test, the switch statement is the most optimal choice. ... When there are a large number of discrete values for which to test, both if-else and switch are significantly slower than using a lookup table. (Ch. 4 Algorithms and Flow Control)

Also, instead of calling window.location.hash with each evaluation, copy that value to a local variable and evaluate that. It will improve performance since you won't keep going back to the DOM to look-up the value.

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.