current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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?

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

2 Answers 2

up vote 4 down vote accepted

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

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;
    }
}
share|improve this answer
    
Thanks for your comments, I will rework it. –  d347hm4n Nov 13 '14 at 10:12
    
You don't need to initialize it with null, that value is never used. –  ANeves Nov 13 '14 at 10:32
    
See stackoverflow.com/questions/359732/… regarding your curly braces. Plugins like StyleCop specifically flag this structure –  danrhul Nov 13 '14 at 10:39
1  
Also shouldn't it be fullUrl = ConnectionStringData.Location.Trim(); –  danrhul Nov 13 '14 at 10:41
2  
@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.) –  ANeves Nov 13 '14 at 11:52

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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