0

I have a custom configuration reader which performs various functions on top of .NET's CloudConfigurationManager and ConfigurationManager implementations, such as caching, type conversion, etc.

The following method is used to retrieve a configuration item:

public T GetSetting<T>(string key)
{
    string value = GetSetting(key);

    return (T)Convert.ChangeType(value, typeof(T), CultureInfo.InvariantCulture);
}

There are a few failure points for which I want to add the correct exception handling.

GetSetting<T> with exception handling:

public T GetSetting<T>(string key)
{
    // If the key is null or empty
    if (String.IsNullOrWhiteSpace(key))
    {
        throw new ArgumentNullException(String.Format("Configuration value with key '{0}' cannot be null or empty", key));
    }

    string value = GetSetting(key);

    // If the value does not exist in the underlying configs
    if(value == null)
    {
        throw new ArgumentOutOfRangeExcetion(String.Format("Configuration value with key '{0}' does not exist", key));
    }

    try
    {
        return (T)Convert.ChangeType(value, typeof(T), CultureInfo.InvariantCulture);
    }
    catch(InvalidCastException ex)
    {
        throw new ArgumentOutOfRangeException(String.Format("Configuration value with key '{0}' could not be converted: Conversion not supported.", key), ex);
    }
    catch (FormatException ex)
    {
        throw new ArgumentOutOfRangeException(String.Format("Configuration value with key '{0}' could not be converted: Format error.", key), ex);
    }
}

My questions and concerns:

If the key is null or empty:

Is it acceptable to throw an ArgumentNullException when the string parameter is not only null, but also empty? Perhaps a ArgumentException is better as this is more general issue than just null checking.

If the value does not exist in the underlying configs:

An ArgumentOutOfRangeException is generally used for a index which doesn't exist in list- or dictionary-based collection. Does it apply here? The configs are essentially key value pair dictionaries.

An idea is that the method can just return null, but I have provided a HasSetting(string) method for this reason. In my opinion, the more explicit the better. Checking for null in client code requires implicit understanding of the encapsulated functionality, which can often be misused, misread and misunderstood.

If parsing to T fails for whatever reason:

This is the tricky one. Which is the best exception type to encapsulate the 'conversion' exceptions? Should I let the underlying exceptions from Convert.ChangeType() bubble up?

Convert.ChangeType() overflow exception:

I see no reason to handle the OverflowException in Convert.ChangeType(). I will just let this throw.

For reference reasons, these are the exceptions thrown from Convert.ChangeType():

// Exceptions:
//   System.InvalidCastException:
//     This conversion is not supported. -or-value is null and conversionType is
//     a value type.-or-value does not implement the System.IConvertible interface.
//
//   System.FormatException:
//     value is not in a format for conversionType recognized by provider.
//
//   System.OverflowException:
//     value represents a number that is out of the range of conversionType.
//
//   System.ArgumentNullException:
//     conversionType is null.

1 Answer 1

2

Is it acceptable to throw an ArgumentNullException when the string parameter is not only null, but also empty? Perhaps a ArgumentException is better as this is more general issue than just null checking.

Yes, I think that ArgumentException fits best here. I guess the "proper" solution would be to throw ArgumentNullException for null and ArgumentException for empty, but I don't think that's worth it.

An ArgumentOutOfRangeException is generally used for a index which doesn't exist in list- or dictionary-based collection. Does it apply here? The configs are essentially key value pair dictionaries.

ArgumentOutOfRangeException is not used for dictionaries. For example, the indexer of Dictionary throws KeyNotFoundException. I think you should use that, especially since you don't really have any range here.

An idea is that the method can just return null, but I have provided a HasSetting(string) method for this reason.

Can the configuration change between the call to HasSetting() and GetSetting()? If it can, consider adding something like bool TryGetSetting<T>(string key, out T item), so that an item that may not be present can be retrieved safely without exception handling.

But I agree that checking for null is usually not a good idea.

If parsing to T fails for whatever reason

ArgumentOutOfRangeException is even less appropriate here. If you don't find any built-in exception that fits, create a custom exception class and throw that.

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