App settings helper

Question

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<TResult>(string key, Func<TResult> 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<TResult>(key, valueOfKey);
 }
 return result;
 }
 private static TResult ConvertToResult<TResult>(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 = GetDefaultValue((Func<TResult>)Convert.ChangeType(DefaultValue, typeof(Func<TResult>)));
 }
 return result;
 }
 private static TResult GetDefaultValue<TResult>(Func<TResult> defaultValue)
 {
 var result = default(TResult);
 if (defaultValue != null)
 {
 try
 {
 result = defaultValue.Invoke();
 }
 catch (Exception ex)
 {
 if (ThrowException)
 {
 throw new Exception($"Failed to to invoke delegate {defaultValue} of type {defaultValue.GetType()}", ex);
 }
 }
 }
 return result;
 }
 }
}

• RubberDuck: What version of the framework are you targeting? Can I assume it's >= 2?
• Stefan Karlsson: Yes its .NET 4.6.
• Stefan Karlsson: If I were to summarize all the answers, as I have understood it. It is preferable to store shared environment settings in the database. For such settings that varies in different environments, I should use Properties.Settings? Have I understood correctly?
• Stefan Karlsson: Also found this and it looks interesting: github.com/davidwhitney/System.Configuration.Abstractions
• RubberDuck: the answer to that question is "it depends". There are lots of things to consider and you have to weigh them on a case by case basis. For instance, if you store the settings in a database, where do you keep the connection string for the db?

Answer 1

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<TResult>(string key, Func<TResult> 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<TResult>(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<TResult>(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.";
}

• Stefan Karlsson: Thank you for this refactoring and comments, i like your solution better than mine, I will also have a look at Properties.Settings suggest by @RubberDuck.
• t3chb0t: I added example exceptions that you could throw and added them to the previous code.

Answer 2

I simultaneously love and hate giving answers like this, but I feel like I have to...

You're trying to solve the issue that everything is stored as an Object in the App.Config file, right? Well, this problem has already been solved by the .Net team a long time ago. Exactly 10 years ago actually. The solution only works if you're on at least version 2.0 of the framework, but there are relatively few apps out there running on something less than that.

Anyway, instead of declaring items in the config with

<add key="Application1" value="MyApplication1" />

You can strong type your settings by using the GUI. Then you can use your strongly typed settings like this.

var mySetting = Properties.Settings.Default.Application1

Which would return a String. You can use any type you like, even classes, enums, and structs that you've written yourself.

• RobH: FWIW, you can override those settings in the app/web config easily: stackoverflow.com/questions/6608240/… (so you lose nothing)
• t3chb0t: Is anyone actually using the Properties.Settings? You need the app.config etc. these days only for the connection string to read the real settings from a database.
• RubberDuck: @t3chb0t I find it to be a convenient way to switch certain things out between dev, test, & prod environments. Our dev and test environments share a database though. I'd prefer your approach if it was an option for me.