3
\$\begingroup\$

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?

\$\endgroup\$
  • 2
    \$\begingroup\$ 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 \$\endgroup\$ – Nick Udell Nov 13 '14 at 10:06
  • \$\begingroup\$ 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. \$\endgroup\$ – d347hm4n Nov 13 '14 at 10:11
  • \$\begingroup\$ You don't need to initialize uri to null, out will take care of that. \$\endgroup\$ – svick Nov 14 '14 at 22:14
4
\$\begingroup\$

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;
    }
} 
\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – d347hm4n Nov 13 '14 at 10:46
  • \$\begingroup\$ Any reason especially for a new char[] when you can just pass in: '/' to TrimEnd. \$\endgroup\$ – d347hm4n Nov 13 '14 at 10:48
  • 1
    \$\begingroup\$ Not really, intellisense said char[] so I used it ;-) \$\endgroup\$ – Heslacher Nov 13 '14 at 10:50
  • \$\begingroup\$ Be aware that this behaves differently when the original string ends with multiple /s, thought I guess such URLs should be pretty rare. \$\endgroup\$ – svick Nov 14 '14 at 22:16
5
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ You don't need to initialize it with null, that value is never used. \$\endgroup\$ – ANeves Nov 13 '14 at 10:32
  • \$\begingroup\$ See stackoverflow.com/questions/359732/… regarding your curly braces. Plugins like StyleCop specifically flag this structure \$\endgroup\$ – ediblecode Nov 13 '14 at 10:39
  • \$\begingroup\$ @ANeves please tell us why? \$\endgroup\$ – dieter Nov 13 '14 at 10:40
  • 1
    \$\begingroup\$ Also shouldn't it be fullUrl = ConnectionStringData.Location.Trim(); \$\endgroup\$ – ediblecode Nov 13 '14 at 10:41
  • 2
    \$\begingroup\$ @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.) \$\endgroup\$ – ANeves Nov 13 '14 at 11:52

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.