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

2 Answers 2

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\$
4
  • \$\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\$
    – aydjay
    Nov 13, 2014 at 10:46
  • \$\begingroup\$ Any reason especially for a new char[] when you can just pass in: '/' to TrimEnd. \$\endgroup\$
    – aydjay
    Nov 13, 2014 at 10:48
  • 1
    \$\begingroup\$ Not really, intellisense said char[] so I used it ;-) \$\endgroup\$
    – Heslacher
    Nov 13, 2014 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, 2014 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\$
7
  • \$\begingroup\$ You don't need to initialize it with null, that value is never used. \$\endgroup\$
    – ANeves
    Nov 13, 2014 at 10:32
  • \$\begingroup\$ See stackoverflow.com/questions/359732/… regarding your curly braces. Plugins like StyleCop specifically flag this structure \$\endgroup\$
    – ediblecode
    Nov 13, 2014 at 10:39
  • \$\begingroup\$ @ANeves please tell us why? \$\endgroup\$
    – dieter
    Nov 13, 2014 at 10:40
  • 1
    \$\begingroup\$ Also shouldn't it be fullUrl = ConnectionStringData.Location.Trim(); \$\endgroup\$
    – ediblecode
    Nov 13, 2014 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, 2014 at 11:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.