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 0 down vote favorite

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');
share|improve this question
    
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. –  Peter Monks Jan 9 '14 at 14:27
    
@PeterMonks Yes..That is indeed a great point but I handled this case later (after I posted the question here). Once again thanks. –  blunderboy Jan 9 '14 at 14:48

2 Answers 2

up vote 2 down vote

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;
};
share|improve this answer
up vote 1 down vote

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.

share|improve this answer
    
Thank you for the feedback –  blunderboy Jan 9 '14 at 10:37

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.