Comment by pennig | Hacker News Reader

Comment by pennig | Hacker News Reader

While fun to think about – a nice little brain stretcher – this solution smells of cleverness for cleverness' sake.

Arguing that this is a better solution on the grounds of optimization would be salient if this were a function likely to be called thousands of times on a large data set. I just can't imagine that being the case in this instance.

I subscribe to the idea that when given the choice between 'clever' optimized code, and code which is instantly readable and maintainable, it's best to opt for the latter – unless you're trying to resolve an execution bottleneck.

It doesn't even seem that optimized. The fact that the replace function sticks undefined back into the string means it has to coerce undefined into the string 'undefined' then manipulate the string needlessly (he even admits you end up with a string of 'undefined&undefined&undefined' that you throw away).

The version eventually created in the comments seemed much better:

  function compress(data){
    var match = [], q = {}, ret = "", re = /([^=&]+)=([^&]*)/g;
    while (match = re.exec(data)){
      q[match[1]] = (q[match[1]] ? q[match[1]] + "," : "") + match[2];
    }
    for ( var key in q )
      ret = (ret ? ret + "&" : "") + key + "=" + q[key];
    return ret;
  }

The underlying string manipulation of the browser can be horribly bad. I think it was IE6 that had a quadratic time concatenation issue. Granted, query strings aren't really going to be long enough to matter for this, but if you are for some reason parsing a lot of these at once, speed could matter.

Reply on news.ycombinator.com