Today is “beautify ugly JS” (BUJS) day. Enjoy!

This post is part of the BUJS series, a collection of posts that focus on and discuss best practices in JavaScript by revealing the inefficiencies and code-smells of randomly selected JS code from the wild.

Today’s source: from here — sheesh, 150 upvotes!

function getParameterByName( name )
  name = name.replace(/[[]/,"\[").replace(/[]]/,"\]");
  var regexS = "[\?&]"+name+"=([^&#]*)";
  var regex = new RegExp( regexS );
  var results = regex.exec( window.location.href );
  if( results == null )
    return "";
    return decodeURIComponent(results[1].replace(/+/g, " "));

What’s wrong with it?

Well, nothing, but at the same time, everything!

  • WTF is the first line about? I guess the intention is to handle when name is something like meh[]. The [] isn’t standardised, AFAIK. Also, WTF is the point if it’s only going to return the first value of meh[]. Either give it full support ("?meh[]=1&meh[]=2" -> [1,2]) or give it NO support.
  • Why exactly is regexS stored whatsover… it’s only used once elsewhere. Readability won’t suffer at all if we were to simply place the assigned expression where it’s eventually needed.
  • We don’t need to escape ? when it’s in a regular-expression character-class. It’s in a character class FFS! Let’s rid ourselves of that \ malarky!
  • Cool kids know that the new operator isn’t always required. Readability matters. I will assume the person reading the code has read the spec!
  • RegExp.prototype.exec can only return a match array (of Array type) or null. null is the only potential falsey return value — may as well be cool and rid ourselves of == null.
  • Two return statements… where’s the delicious ternary operator? (or “conditional operator”) — cool people use this.
  • Use instead of location.href so we can do-away with handling #... things (in the original regex: =([^&#]*)).
// Revised, cooler.
function getParameterByName(name) {
    var match = RegExp('[?&]' + name + '=([^&]*)')
    return match ?
        decodeURIComponent(match[1].replace(/+/g, ' '))
        : null;

EDIT: I posted my solution on SO, if you feel so obliged to upvote! 😉

EDIT2: As Andy E. noted in his comment (@ StackOverflow) I could shorten it further by using return match && decodeURIComponent(match[1].replace(/+/g, ' ')); instead of the ternary operator. This totally slipped my mind. The && operator will return the left operand if the right operand is falsey and since .exec() returns null anyway this works out nicely! I’ll leave the original improved version intact here, see the updated one here.

Potential improvement: protect against ill-use by escaping Regex characters in name… also potentially String(name) to make it almost bulletproof.

Join me next time. This series’ll rock!

Thanks for reading! Please share your thoughts with me on Twitter. Have a great day!