1
\$\begingroup\$

I have written this code which adds a querystring to a URL:

exports.addQueryString = function(url, queryString) {
  var isQuestionMarkPresent = url && url.indexOf('?') !== -1,
    separator = '';

  if (queryString) {
    separator = isQuestionMarkPresent ? '&' : '?';
    url += separator + queryString;
  }

  return url;
};

Is there any way I can write this better?

Usage:

addQueryString('http://example.com', 'q=1&val=3&user=me');
\$\endgroup\$
  • \$\begingroup\$ Have you considered proper encoding of query string values? From your code it looks like it might be a separate operation since you already pass in a full query string but may be relevant overall. \$\endgroup\$ – Peter Monks Jan 9 '14 at 14:27
  • \$\begingroup\$ @PeterMonks Yes..That is indeed a great point but I handled this case later (after I posted the question here). Once again thanks. \$\endgroup\$ – sachinjain024 Jan 9 '14 at 14:48
3
\$\begingroup\$

I think that is ok, only that i see not necesary make url.indexOf('?') if queryString is empty, because you are not going to use it.

exports.addQueryString = function(url, queryString) {   
  if (queryString) {
    var isQuestionMarkPresent = url && url.indexOf('?') !== -1,
      separator = isQuestionMarkPresent ? '&' : '?';
    url += separator + queryString;
  }

  return url;
};
\$\endgroup\$
1
\$\begingroup\$

What happens when you just want to add a single queryString to and already long queryString? You would need to know the entire queryString to do that, just to add a single new value.

What your function should do is take a single queryString value and if it already exists it should update it and if it doesn't exist it should append it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.