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?
3 Answers 3
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.
-
\$\begingroup\$ Didn't know this, good to know. \$\endgroup\$BCdotWEB– BCdotWEB2015年05月26日 09:50:21 +00:00Commented May 26, 2015 at 9:50
-
\$\begingroup\$ @Quill to show the value being url encoded. \$\endgroup\$RobH– RobH2015年05月26日 10:29:43 +00:00Commented 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\$svick– svick2015年05月26日 10:37:40 +00:00Commented 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\$RobH– RobH2015年05月26日 10:53:16 +00:00Commented May 26, 2015 at 10:53
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 casestring.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 ofStringBuilder SB
-
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– Abbas2015年05月26日 07:48:30 +00:00Commented 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\$Nick Udell– Nick Udell2015年05月26日 08:41:05 +00:00Commented 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
StringBuilder
s methods are not needed and b) one should favor composition over inheritance \$\endgroup\$BatteryBackupUnit– BatteryBackupUnit2015年05月26日 10:15:08 +00:00Commented May 26, 2015 at 10:15 -
\$\begingroup\$ thanks for the suggestions, i refactored the code to a separate class. \$\endgroup\$BatteryBackupUnit– BatteryBackupUnit2015年05月26日 10:41:21 +00:00Commented May 26, 2015 at 10:41
-
1\$\begingroup\$ @BatteryBackupUnit Yes, good point. A
QueryBuilder
that offered anAppendLine
method would not make much sense. \$\endgroup\$Nick Udell– Nick Udell2015年05月27日 12:45:23 +00:00Commented May 27, 2015 at 12:45
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