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

If the current page URL has an argument 'myid1' or 'myid2' in the querystring, for each link in my webpage with class 'rewrite', I want the link href's querystring to be replaced by the current page URL's querystring. I'm using the code given below. Since I'm new to javascript, I'm not sure if its optimized. I want it to execute as fast as possible. Please help. Thanks in advance :)

<script type='text/javascript' src='https://ajax.googleapis.com/ajax/libs/jquery/1.6.4/jquery.min.js'></script>

<script type="text/javascript">
$(function() {
    var requestid = gup('myid1');
    if (requestid) {
        $("a.rewrite").each(function() {
            var base = this.href;
            var pos = base.indexOf("?");
            if (pos != -1) {
                base = base.substr(0, pos);
            }
            this.href = base + "?myid1=" + requestid;
        })
    }
    var requestid2 = gup('myid2');
    if (requestid2) {
        $("a.rewrite").each(function() {
            var base = this.href;
            var pos = base.indexOf("?");
            if (pos != -1) {
                base = base.substr(0, pos);
            }
            this.href = base + "?myid2=" + requestid2;
        })
    }
})

function gup( name )
{
  name = name.replace(/[\[]/,"\\\[").replace(/[\]]/,"\\\]");
  var regexS = "[\\?&]"+name+"=([^&#]*)";
  var regex = new RegExp( regexS );
  var results = regex.exec( window.location.href );
  if( results == null )
    return "";
  else
    return results[1];
}
</script>

<a href="http://www.website.com/?someid=1234" class="rewrite">Hyperlink</a>
share|improve this question
    
Optimization is easy, don't use jQuery. –  Raynos Nov 14 '11 at 14:18
    
@Raynos How do you suppose I do that? It doesn't work without jquery. –  Shyam Sundar Nov 14 '11 at 14:53
    
Use this thing called the DOM. Maybe QSA ? –  Raynos Nov 14 '11 at 15:31
    
@Raynos I think you're missing the point of codereview.se. But if you really think jQuery is slowing this code down, please step up and post a working example without jQuery. –  kojiro Nov 14 '11 at 23:23
    
@kojiro am I not allowed to make comments without providing full solutions? –  Raynos Nov 14 '11 at 23:53

2 Answers 2

up vote 2 down vote accepted

There's not very much you can do about the performance.

You can remove the repeated code using a loop:

$(function() {
  $.each(['myid1', 'myid2'], function(index, id){
    var requestid = gup(id);
    if (requestid != "") {
      $("a.rewrite").each(function() {
        var base = this.href;
        var pos = base.indexOf("?");
        this.href = (pos != -1 ? base.substr(0, pos) : base) + "?" + id + "=" + requestid;
      });
    }
  });
})

In the gup function you can use a single replace instead of two:

function gup(name) {
  var pattern = "[\\?&]" + name.replace(/([\[\]])/g,"\\$1") + "=([^&#]*)";
  var results = new RegExp(pattern).exec(window.location.href);
  return results == null ? "" : results[1];
}

(Try to use more descriptive names than "gup", though...)

share|improve this answer
    
Perfect! Thank you very much Guffa. –  Shyam Sundar Nov 15 '11 at 9:33
    
var results = new RegExp(pattern).exec(window.location.href); still execute each time a link href is altered. Not efficient, see my post. –  steveyang Nov 16 '11 at 2:01
    
@yangchenyun: You are mistaken. Look at the code again. The call to the gup function where that code is, is outside the loop that changes the links. –  Guffa Nov 16 '11 at 2:09
    
Yep, it is optimized than original code, but the same calculation to fetch key/value pair from window.location.href still repeat itself for each id (myid1, myid2) which could be optimized. My method use an regular expression to fetch all the key-value pair at once and cache the result for later usage. –  steveyang Nov 16 '11 at 2:13
    
@yangchenyun: Yes that is true, but on the other hand you are looping through all query string values, even if they are not going to be used. –  Guffa Nov 16 '11 at 7:45
up vote 2 down vote

The code you provide is inefficient in two ways:

  1. Unnecessary loops. It loops through a.rewrite anchor each time for one querystring match. It could be optimized into one loop;

  2. Repeated calculation on regular expression matches. regexS is executed for each gup function and it could be reduced into one calculation.

The solutions are:

  1. Fetch the window.location.href data into one variable which could be referred to later;

  2. Integrate the two (or more) loops into one and finish all the replacement in one loop.

Here the optimized code goes:

//First you fetch the query string as key-value pairs in the window.location.href, this equals your gup function.

//This code, fetch the ?key1=value1&key2=value2 pair into an javaScript Object {'key1': 'value1', 'key2':'value2'}
var queryString = {}; 
var queryStringPattern = new RegExp("([^?=&]+)(=([^&]*))?", "g");
window.location.href.replace(
    queryStringPattern,
    function($0, $1, $2, $3) { queryString[$1] = $3; }
);

//Second you collect all the anchor with class rewrite and execute the replacement.

$("a.rewrite").each(function () {
  this.href.replace(
    queryStringPattern,
    function ($0, $1, $2, $3) {
      return queryString[$1] ? $1 +  "=" + queryString[$1] : $1 + '=' + $3;
    }
  )
});
share|improve this answer
    
I don't understand this code. Where exactly should I insert "myid1" & "myid2"? There is going to be only one key at a time in the url. So, I won't require fetching multiple values like ?key1=value1&key2=value2 etc. –  Shyam Sundar Nov 14 '11 at 14:51
    
It's certainly more readable, so props for that. Did you try an actual benchmark for empirical proof? In particular, I'm curious how $.each would stack up against $.attr("href", function … ) –  kojiro Nov 14 '11 at 14:53
    
@Shyam Sundar This code behaves the same with your former code and it doesn't care which variable name you use in your query string If in the window url you have ?key1=value1&key2=value2, it will substitute all the anchor elements whose href contains either key1=anothervalue or key2=anothervalue to key1=value and key2=value. The replacement happens on the line return queryString[$1] ? $1 + "=" + queryString[$1] : $1 + '=' + $3; –  steveyang Nov 15 '11 at 5:21
    
@kojiro Didn't know tools to run benchmarks. Also interested in $.attr("href", function … ) performance against $.each and this.href. –  steveyang Nov 15 '11 at 5:26
    
@yangchenyun consider jsperf.com for simple pseudo-benchmarks. When more precision is needed, you'll have to devise a controlled local environment, but for this I think jsperf will do fine. –  kojiro Nov 15 '11 at 17:10

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.