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

3 Answers 3

Reset to default
22
\$\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\$
4
  • \$\begingroup\$ Didn't know this, good to know. \$\endgroup\$ Commented May 26, 2015 at 9:50
  • \$\begingroup\$ @Quill to show the value being url encoded. \$\endgroup\$ Commented May 26, 2015 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\$ Commented May 26, 2015 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\$ Commented May 26, 2015 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\$
6
  • 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\$ Commented May 26, 2015 at 7:48
  • \$\begingroup\$ Agreed. Possibly a subclass (or wrapper) for StringBuilder that encapsulates this behaviour would make sense. QueryBuilder perhaps? \$\endgroup\$ Commented May 26, 2015 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\$ Commented May 26, 2015 at 10:15
  • \$\begingroup\$ thanks for the suggestions, i refactored the code to a separate class. \$\endgroup\$ Commented May 26, 2015 at 10:41
  • 1
    \$\begingroup\$ @BatteryBackupUnit Yes, good point. A QueryBuilder that offered an AppendLine method would not make much sense. \$\endgroup\$ Commented May 27, 2015 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\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.