13
\$\begingroup\$

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 couldn't 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?
\$\endgroup\$
19
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Didn't know this, good to know. \$\endgroup\$ – BCdotWEB May 26 '15 at 9:50
  • \$\begingroup\$ @Quill to show the value being url encoded. \$\endgroup\$ – RobH May 26 '15 at 10:29
  • 3
    \$\begingroup\$ I'm not sure it's a good idea to fall back internal framework classes and rely on their undocumented behavior. \$\endgroup\$ – svick May 26 '15 at 10:37
  • \$\begingroup\$ @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. \$\endgroup\$ – RobH May 26 '15 at 10:53
8
\$\begingroup\$

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
\$\endgroup\$
  • 2
    \$\begingroup\$ 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.. \$\endgroup\$ – Abbas May 26 '15 at 7:48
  • \$\begingroup\$ Agreed. Possibly a subclass (or wrapper) for StringBuilder that encapsulates this behaviour would make sense. QueryBuilder perhaps? \$\endgroup\$ – Nick Udell May 26 '15 at 8:41
  • \$\begingroup\$ @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 \$\endgroup\$ – BatteryBackupUnit May 26 '15 at 10:15
  • \$\begingroup\$ thanks for the suggestions, i refactored the code to a separate class. \$\endgroup\$ – BatteryBackupUnit May 26 '15 at 10:41
  • 1
    \$\begingroup\$ @BatteryBackupUnit Yes, good point. A QueryBuilder that offered an AppendLine method would not make much sense. \$\endgroup\$ – Nick Udell May 27 '15 at 12:45
5
\$\begingroup\$

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
\$\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.