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 8 down vote favorite
1

I speedily wrote a bunch of crappy checks, to check whether strings are empty, in order to build a query string.

StringBuilder SB = new StringBuilder();

SB.AppendFormat("{0}guide/search.aspx?", root);

root is a const, containing the website address.
It's like this because we have test servers.

if (!String.IsNullOrEmpty(item)) {
    SB.AppendFormat("&i={0}", Server.UrlEncode(item));
}
if (!String.IsNullOrEmpty(found)) {
      SB.AppendFormat("&f={0}", Server.UrlEncode(found));
}
if (!String.IsNullOrEmpty(what)) {
      SB.AppendFormat("&wt={0}", Server.UrlEncode(what));
}
if (!String.IsNullOrEmpty(where)) {
      SB.AppendFormat("&wr={0}", Server.UrlEncode(where));
}

I tried replacing these with ternaries, but I couldnt get my head around how to do that with the sb.AppendFormat in place.

  • Is there a better way to check empty strings than -String.IsNullOrEmpty?
  • Is my method of naming variables correct with C# standards?
  • Is Server.UrlEncode the best way to format strings into query strings?
share|improve this question

3 Answers 3

up vote 10 down vote accepted

Although there's nothing wrong with your code, it is generally easier to fall back on framework classes for this kind of thing. Namely UriBuilder and HttpValueCollection.

public static Uri BuildUri(string root, NameValueCollection query)
{
    // omitted argument checking

    // sneaky way to get a HttpValueCollection (which is internal)
    var collection = HttpUtility.ParseQueryString(string.Empty);

    foreach (var key in query.Cast<string>().Where(key => !string.IsNullOrEmpty(query[key])))
    {
       collection[key] = query[key];
    }

    var builder = new UriBuilder(root) { Query = collection.ToString() };
    return builder.Uri;
}

Then you can simply use with a name value collection:

var values = new NameValueCollection 
{ 
    {"i", "item" }, 
    { "f", "found" },
    { "wt", "what%" },
    { "wr", "where" }
}; 
BuildUri("http://example.com/search.aspx", values);
// http://example.com/search.aspx?i=item&f=found&wt=what%25&wr=where

The HttpValueCollection handles all of the encoding for you so you don't need to worry about doing it the right way. You just create the name value collection and any null/empty values will just be skipped in the BuildUri method.

Update

As svick has pointed out in the comments it might not be ideal to use an internal class in this way. I think the risk of the implementation changing is quite low but it is a risk and should be thought about.

share|improve this answer
    
Why is wt set as what% in the values? –  Quill yesterday
    
Didn't know this, good to know. –  BCdotWEB yesterday
    
@Quill to show the value being url encoded. –  RobH yesterday
1  
I'm not sure it's a good idea to fall back internal framework classes and rely on their undocumented behavior. –  svick yesterday
    
@svick - that's certainly a valid concern. I think the risk in HttpValueCollection changing is very low but you are right: it could lead to a pretty subtle bug if it ever did change. –  RobH yesterday
up vote 4 down vote

how about:

QueryBuilder.For("guide/search.aspx")
    .Query("i", item)
    .Query("f", found)
    .Query("wt", what)
    .Query("wr", where)
    .ToString();

public class QueryBuilder
{
    public static QueryBuilder For(string page)
    {
        return new QueryBuilder(page);
    }

    private readonly StringBuilder sb;

    private QueryBuilder(string page)
    {
        sb = new StringBuilder()
                 .AppendFormat(CultureInfo.InvariantCulture,
                               "{0}{1}?",
                               root,
                               page);
    }

    public QueryBuilder Query(string queryName, string queryValue)
    {
        // todo: throw if parameter sb or queryValue is null

        if(!string.IsNullOrEmpty(queryValue))
        {
            sb.AppendFormat(CultureInfo.InvarianteCulture,
                            "&{0}={1}",
                            queryName,      
                            Server.UrlEncode(what));
        }

        return this;
    }

    public override string ToString()
    {
        return sb.ToString();
    }
}

regarding your questions:

  • string.IsNullOrEmpty is ok. Depending on the use case string.IsNullOrWhitespace (=> null or empty or only whitespaces like space, tab, newline) may be a good fit too.
  • local variable names should be lower case StringBuilder sb instead of StringBuilder SB
share|improve this answer
2  
An extension method is a little bit overkill for this use, I would agree to create a separate method but its purpose is, in my opinion, too small to create an extension method. From MSDN on the guidelines for extension methods: In general, we recommend that you implement extension methods sparingly and only when you have to.. –  Abbas yesterday
    
Agreed. Possibly a subclass (or wrapper) for StringBuilder that encapsulates this behaviour would make sense. QueryBuilder perhaps? –  Nick Udell yesterday
    
@Abbas I Agree that a Wrapper would be a better solution. I wouldn't subclass, because a) a lot of the StringBuilders methods are not needed and b) one should favor composition over inheritance –  BatteryBackupUnit yesterday
    
thanks for the suggestions, i refactored the code to a separate class. –  BatteryBackupUnit yesterday
    
@BatteryBackupUnit Yes, good point. A QueryBuilder that offered an AppendLine method would not make much sense. –  Nick Udell 8 hours ago
up vote 1 down vote

What you have is a collection of key-value pairs of strings, and you're performing the same operation on all of them. A simple improvement is to throw them into a collection and iterate over it.

var parameters = new List<KeyValuePair<string, string>>
{
    new KeyValuePair<string, string>("i", item), // "item"
    new KeyValuePair<string, string>("f", found), // null or ""
    new KeyValuePair<string, string>("wt", what), // "what"
    new KeyValuePair<string, string>("wr", where) // "where"
};

foreach (string parameter in parameters)
{
    if (!String.IsNullOrEmpty(parameter)) {
        SB.AppendFormat("&{0}={1}", parameter.Key, Server.UrlEncode(parameter.Value));
    }
}
// http://example.com/guide/search.aspx?&i=item&wt=what&wh=where

There is a problem with your code and the code above, though: the query string is always going to start with an ampersand. A quick and dirty solution would be to build out the query string first, and then take a substring to truncate the first character before appending it to the StringBuilder.

Another way to solve this is using Linq to filter and format the data, and then bringing it all together with a String.Join call, in which we can specify the ampersand as a separator.

var parameters = new List<KeyValuePair<string, string>>
{
    new KeyValuePair<string, string>("i", item), // "item"
    new KeyValuePair<string, string>("f", found), // null or ""
    new KeyValuePair<string, string>("wt", what), // "what"
    new KeyValuePair<string, string>("wr", where) // "where"
};

var validParameters = parameters.Where(p => !String.IsNullOrEmpty(p.Value));
var formattedParameters = validParameters.Select(p => p.Key + "=" + Server.UrlEncode(p.Value));
SB.Append(String.Join("&", formattedParameters));
// http://example.com/guide/search.aspx?i=item&wt=what&wh=where
share|improve this answer

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.