App settings helper

The helper should be able to set a default value if the value can't be fetched with the provided key. There is also a bool that will throw an error if the key value or the default value could not be fetched or converted.

Can I make it more compact, safer, and still have the same functionality?

It feels like I am using too many try/catch blocks.

```
using System;
using System.Configuration;

namespace Successful.Misc
{
public static class AppSettingsHelper
{
private static bool ThrowException { get; set; }
private static object DefaultValue { get; set; }
public static TResult Get(string key, Func defaultValue = null, bool throwException = false)
{
ThrowException = throwException;
DefaultValue = defaultValue;

var result = default(TResult);
var valueOfKey = ConfigurationManager.AppSettings[key];

if (string.IsNullOrWhiteSpace(valueOfKey))
{
if (ThrowException && DefaultValue == null)
{
throw new ArgumentNullException(valueOfKey,
$"Failed to get a value for the key {key} in the App.config file.");
}

result = GetDefaultValue(defaultValue);
}
else
{
result = ConvertToResult(key, valueOfKey);
}

return result;
}

private static TResult ConvertToResult(string key, string valueOfKey)
{
TResult result;

try
{
result = (TResult)Convert.ChangeType(valueOfKey, typeof(TResult));
}
catch (Exception ex)
{
if (ThrowException && DefaultValue == null)
{
throw new Exception($"Failed to convert {valueOfKey} from type {valueOfKey.GetType()} to the type {typeof (TResult)}", ex);
}

result = GetDefaul

You can of course use a different approach as @RubberDuck suggested but for the case you stick to this one I'd refactor it to this:

public static class AppSettingsHelper
{   
    public static TResult GetValue(string key, Func getDefaultValue = null, bool canThrowException = false)
    {
        if (string.IsNullOrEmpty(key)) throw new ArgumentNullException(nameof(key));

        var result = default(TResult);
        var value = ConfigurationManager.AppSettings[key];

        try
        {
            if (string.IsNullOrWhiteSpace(value))
            {
                if (getDefaultValue != null)
                {
                    result = getDefaultValue(defaultValue);
                }

                if (canThrowException && result == default(TResult))
                {
                    throw new ValueNullException(key);
                }                               
            }
            else
            {
                result = ConvertValue(value);
            }
        }
        catch(ValueNullExcpetion)
        {
            // no need to check the 'canThrowException'
            throw;
        }
        catch(ValueConvertExcpetion ex)
        {
            // no need to check the 'canThrowException'
            ex.Key = key;
            throw;
        }
        catch(Exception ex)
        {
            if (canThrowException) throw new GetValueExcpetion(key, ex);
        }

        return result;
    }

    private static TResult ConvertValue(string value, bool canThrowException)
    {
        try
        {
            var result = (TResult)Convert.ChangeType(valueOfKey, typeof(TResult));
            return result;
        }
        catch (Exception ex)
        {
            if (canThrowException)
            {
                throw new ValueConvertExcpetion(value, typeof(TResult), ex);
            }
            return default(TResult);
        }
    }        
}

In my opinion

• you don't need the static fields

• you should use more descriptive names for the methods and parameters then simply Get or defaultValue if it's actually a function getDefaultValue is a better name I think

• if you use a true/false variable it's better to give it a name that begins with can/has/is etc. throwException suggests it's a function

• you don't need the GetDefaultValue method at all, it doesn't have much value

• you can throw exceptions and add more information to them in a big try/catch so that you don't have to forward values to the methods that they don't need but for throwing exceptions

EDIT:

You could for example create a few exceptions to give the user more information about what went wrong:

Notice that I also moved message texts into the exceptions so you can reuse them and keep the throws cleaner.

public class ValueNullExcpetion : Exception
{
    internal ValueNullExcpetion(string key) 
    {
        Key = key;
    }

    public string Key { get; private set; }

    public override string Message => 
        $"Failed to get a value for the key \"{Key}\" in the App.config file.";
}

public class ValueConvertExcpetion : Exception
{
    internal ValueConvertExcpetion(string value, Type targetType, Exception innerExeption) 
        : base(null, innerException)
    {
        Value = value;
        TargetType = targetType;
    }

    public string Key { get; internal set; }

    public string Value { get; private set; }

    public Type TargetType { get; private set; }

    public override string Message => 
        $"Failed to convert {Value} to the type \"{TargetType.Name}\". " +
        $"See inner exception for details.";
}

public class GetValueExcpetion : Exception
{
    internal ValueConvertExcpetion(string key, Exception innerException)
        base(null, innerException)
    {}

    public string Key { get; private set; }

    public override string Message => 
        $"An unexpected exception occured while getting a value for the key \"{Key}\". " +
        "See inner exception for details.";
}

StackExchange Code Review Q#102726, answer score: 6