Getting valid URL from user input

Question

I have a dialog which populates my ConnectionStringData object with the URL of the service that I'm trying to connect to. I then consume that URL with the following property:

 /// <summary>
 /// Where the Service is located.
 /// </summary>
 public string ServiceLocation
 {
 get
 {
 string fullUrl = ConnectionStringData.Location.Trim();
 if (fullUrl.EndsWith("/"))
 {
 fullUrl += "Routes";
 }
 else
 {
 fullUrl += "/Routes";
 }
 Uri uri = null;
 if(Uri.TryCreate(fullUrl, UriKind.Absolute, out uri))
 {
 return uri.ToString();
 }
 throw new UriFormatException(String.Format("{0} is an invalid url", fullUrl));
 }
 }

Should I be doing work like this in the getter? Can I do this better?

• Nick Udell: I'm concerned by the exception in the getter. This indicates that something uncontrollable by the caller is getting thrust in their lap. FXCop agrees: msdn.microsoft.com/en-us/library/bb386039.aspx
• aydjay: You are bang on the money. I'm having to work in a very flaky legacy application so I have to step carefully at every step. I will take the advice of @Abbas and rework it on the consumption side.
• svick: You don't need to initialize uri to null, out will take care of that.

Answer 1

A much more readable version would be

public string ServiceLocation
{
 get
 {
 string fullUrl = ConnectionStringData.Location.Trim().TrimEnd('/') + "/Routes";
 Uri uri = null;
 if (Uri.TryCreate(fullUrl, UriKind.Absolute, out uri))
 {
 return uri.ToString();
 }
 return String.Empty;
 }
} 

• aydjay: You are right, I prefer this style as opposed to the additional ternary. You've used backslashes however which is correctly for folder path's but not URL's. :D
• aydjay: Any reason especially for a new char[] when you can just pass in: '/' to TrimEnd.
• Heslacher: Not really, intellisense said char[] so I used it ;-)
• svick: Be aware that this behaves differently when the original string ends with multiple /s, thought I guess such URLs should be pretty rare.

Answer 2

This piece:

if (fullUrl.EndsWith("/"))
{
 fullUrl += "Routes";
}
else
{
 fullUrl += "/Routes";
}

can be rewritten to:

fullUrl += fullUrl.EndsWith("/") ? "Routes" : "/Routes";

or:

if (!fullUrl.EndsWith("/"))
 fullUrl += "/";
fullUrl += "Routes";

Since you're using the TryCreate method, there's no need to throw an exception. You just have to notify the caller whether the conversion succeeded or not. Plus, throwing an exception in a get statement is bad practice. This results in following:

if (Uri.TryCreate(fullUrl, UriKind.Absolute, out uri))
 return uri.ToString();
//if no succesful creation:
return String.Empty;

If you do want to throw an exception, use a try/catch block and a regular Uri constructor. This should be placed in a method and not in the get statement then:

try
{
 uri = new Uri(fullUrl, UriKind.Absolute);
 return uri.ToString();
}
catch (UriFormatException ex)
{
 throw;
}

My favor would still be with the TryCreate resulting in following final code:

public string ServiceLocation
{
 get
 {
 var fullUrl = ConnectionStringData.Location.Trim();
 fullUrl += fullUrl.EndsWith("/") ? "Routes" : "/Routes";
 Uri uri = null;
 if (Uri.TryCreate(fullUrl, UriKind.Absolute, out uri))
 return uri.ToString();
 return String.Empty;
 }
}

• ANeves: You don't need to initialize it with null, that value is never used.
• ediblecode: See stackoverflow.com/questions/359732/… regarding your curly braces. Plugins like StyleCop specifically flag this structure
• devops: @ANeves please tell us why?
• ediblecode: Also shouldn't it be fullUrl = ConnectionStringData.Location.Trim();
• ANeves: @dit you initialize with null, then immediately use it as an out parameter, which overrides the value. So the null value will always be overwritten without a chance to be used. (Quote from MSDN: Although variables passed as out arguments do not have to be initialized before being passed, the called method is required to assign a value before the method returns.)