Good practice throw an exception to satisfy a function return

Built a custom IDataReader which goes looking in XML for values that match particular element names and, if found, returns the values. The GetValue function - which has to return an Object, as specified by the interface, looks like this:

    public object GetValue(int i)
    {
        string SearchString = _columns[i];

        var searchedAttributeValue = from nm in _el.Attributes(SearchString) select nm;
        if (searchedAttributeValue.Count() > 0)
        {
            return ParseTypes(searchedAttributeValue.First().Value);
        }

        var searchedElementValue = from nm in _el.Elements(SearchString) select nm;
        if (searchedElementValue.Count() > 0)
        {
            return ParseTypes(searchedElementValue.First().Value);
        }
    }

This won't build, obviously, because it's possible to reach the end of the function without a valid return.

In this instance, if that happens, it means there's a config error - the user is looking for an element or attribute in the XML that's not there.

I discovered (this was new to me) that you can get round the problem by doing this:

    public object GetValue(int i)
    {
        if (_el == null)
        {
            _el = XNode.ReadFrom(_reader) as XElement;
        }
        string SearchString = _columns[i];

        var searchedAttributeValue = from nm in _el.Attributes(SearchString) select nm;
        if (searchedAttributeValue.Count() > 0)
        {
            return ParseTypes(searchedAttributeValue.First().Value);
        }

        var searchedElementValue = from nm in _el.Elements(SearchString) select nm;
        if (searchedElementValue.Count() > 0)
        {
            return ParseTypes(searchedElementValue.First().Value);
        }

        throw new Exception("oh crap!");
    }

So the function doesn't return, it just throws an error.

However, something feels fundamentally wrong with this approach. Is it okay, why is it okay/not okay and is there a better way to handle the situation?

Jon Skeet
people
quotationmark

Well unless you really need the catch block, you don't need to introduce try/catch/finally. You can just add a throw statement at the end of your initial method.

Along the way, I'd start using FirstOrDefault(), follow normal C# naming conventions for local variables, stop using query expressions when they're not terribly useful, and throw a more specific exception:

public object GetValue(int i)
{
    string searchString = _columns[i];

    var searchedAttribute = _el.Attributes(searchString).FirstOrDefault();
    if (searchedAttribute != null)
    {
        return ParseTypes(searchedAttribute.Value);
    }

    var searchedElement = _el.Elements(searchString).FirstOrDefault();
    if (searchedElement != null)
    {
        return ParseTypes(searchedElement.Value);
    }
    // Nothing found - oops.
    throw new SomeSpecificException("Some message here");
}

If you need the catch block for logging, I'd probably try to catch a specific exception, which probably means moving that into ParseTypes anyway (as you shouldn't get any exceptions from the rest of the calls...) Either way, keep the throw statement at the end.

EDIT: In terms of design, whether you should throw an exception when the value isn't found or not really depends on the expectations. We have no way of knowing whether that indicates something wrong with the system, or just a perfectly normal absence of data. That should determine your choice here. In particular, if every caller is going to try to catch the exception, then that's a design smell and you should either return null or use something like the Dictionary<,>.TryGetValue approach.

people

See more on this question at Stackoverflow