Generating Settings Objects from Database via Factory Class

Question

In an ASP.NET MVC application (C#) have a factory-like class that generates settings objects of ISettingType for my application. However, some settings are very simple and a standard built-in such as a string or number type value can also suffice. I think that the way that I'm doing this is very much a code smell though.

These settings are persisted within my database in a table that's acting as a key/value store. The key is a NVARCHAR(50) column being the name of the setting also acting as the primary key, and the value field is an XML column. Manipulating the XML directly is unnecessary, and I have various classes that implement the ISettingType interface, which includes methods for de/serialization from/to XML. As I mentioned some of these settings are very simple and only require a single string or number value.

The factory looks like this:

public static class SettingsFactory { private const string MODEL_NAMESPACE = "Company.Models.Administration.SettingTypes."; public static ISettingType Create(ApplicationSetting appSetting) { object setting = DeserializeValue(appSetting); var settingType = setting as ISettingType; if (settingType != null) return settingType; throw new InvalidOperationException("Setting is not of a complex type requiring an ISettingType"); } public static T Create<T>(ApplicationSetting appSetting) { if (typeof(ISettingType).IsAssignableFrom(typeof(T))) return (T)Create(appSetting); return (T) DeserializeValue(appSetting); } private static object DeserializeValue(ApplicationSetting appSetting) { XElement rootElement = XElement.Load(appSetting.SettingValue); if (rootElement.HasElements) { Type settingType = Type.GetType(MODEL_NAMESPACE + appSetting.SettingName); if (settingType != null) { using (XmlReader reader = rootElement.CreateReader()) { reader.MoveToContent(); XmlSerializer serializer = new XmlSerializer(settingType); return (ISettingType)serializer.Deserialize(reader); } } } var numeric = ConvertToNumber(rootElement.Value); return numeric ?? rootElement.Value; } private static object ConvertToNumber(string str) { decimal number; if (decimal.TryParse(str, NumberStyles.Any, NumberFormatInfo.InvariantInfo, out number)) { if (number % 1 == 0) { return (long)number; } return number; } return null; } } 

To clarify one thing the class ApplicationSetting is an EF entity storing the key/value pair within the database.

Having a factory that has two "get" (called Create() in the above code) methods where one only works for a specific type of setting seems like a code smell. Additionally having a factory that creates both objects implementing the interface AND primitive types just feels "wrong," but I'm not really sure if wrapping these settings in tiny classes that have a single property so they can implement ISettingType is really the best way to handle this either.

The ApplicationSetting EF Entity class is just a simple POCO:

public class ApplicationSetting { public string SettingName { get; set; } public string SettingValue { get; set; } } 

The ISettingType interface:

public interface ISettingType { string SerializeValue(); ApplicationSetting ToApplicationSetting(); } 

I have an abstract base class that implements this:

public abstract class BaseSetting : ISettingType { public virtual string SerializeValue() { StringBuilder sb = new StringBuilder(); using (var writer = new StringWriter(sb)) using (var xmlWriter = XmlWriter.Create(writer)) { XmlSerializer serializer = new XmlSerializer(this.GetType()); xmlWriter.WriteStartElement("Value"); serializer.Serialize(xmlWriter, this); xmlWriter.WriteEndElement(); xmlWriter.Flush(); } return sb.ToString(); } public virtual ApplicationSetting ToApplicationSetting() { var name = this.GetType().Name; var value = this.SerializeValue(); return new ApplicationSetting { SettingName = name, SettingValue = value }; } } 

An example of a class derived from this is Smtp:

public class Smtp : BaseSetting { public class SmtpServer { [XmlText] [Url] [Required] [Display(Name = "Host")] public string Host { get; set; } [XmlAttribute("port")] [Display(Name = "Port")] public int Port { get; set; } public SmtpServer() { Port = 25; } } [XmlElement("Server")] public SmtpServer Server { get; set; } [XmlElement("Username")] [Required] [Display(Name = "Username")] public string Username { get; set; } [XmlIgnore] [Display(Name = "Password")] public string Password { get; set; } [XmlElement("Password")] public string SerializedPassword { get { return Password.EncryptString(); } set { this.Password = value.DecryptString(); } } } 

Smtp is a view model and is used as below from controller methods:

public async Task<ActionResult> GetSmtpForm() { //db is my Entity Framework context var setting = await db.ApplicationSettings.SingleAsync(s => s.SettingName.Equals("Smtp")); var smtp = SettingFactory.Create<Smtp>(setting); return PartialView("SmtpForm", smtp); } [HttpPost] public async Task<ActionResult> UpdateSmtp(Smtp smtp) { //perform validation -- below is if passed var setting = smtp.ToApplicationSetting(); db.ApplicationSettings.Attach(setting); db.Entity(setting).State = EntityState.Modified; await db.SaveChangesAsync(); //Send data back to client } 

Answer 1

However, some settings are very simple and a standard built-in such as a string or number type value can also suffice.

I think your entire code would be much simpler (and efficient) if all settings implemented the ISettingType interface, regardless of whether that's a "complex type" or a "simple" value.

Just from the caller's perspective - by treating "mere simple strings" and "puny little integer values" differently, you've added the burden of knowledge to the caller's side:

var setting = _factory.Create(settingEntity); // hmmm wait a minute... 

Whether that call will succeed or fail can't be determined at compile-time, because it depends entirely on some serialized value stored in settingEntity. It's putting the caller in a very uncomfortable position:

var setting = _factory.Create<int>(settingEntity); // or is it <bool>? <string>? <DateTime>? 

If I'm new to the code base, I've no idea. Either I go by trial-and-error, or I inspect the serialized data in settingEntityat run-time to see which overload will work.

All that, just because someone decided an int setting was "too simple" to warrant being properly serialized and exposed to the API as an ISettingType.

If every single setting implemented the same interface regardless of its type, this calling code will work every time:

var setting = _factory.Create(settingEntity); 

"Oh my God, but you're so careless about object allocations!" - Yeah, well, until that's measured and proven to be a bottleneck somewhere, then it's not a problem. I'd deem this "special-casing" of standard built-in types, nothing less than premature optimization - that's right, the root of all evil.

---

But let's look at the implementation, regardless.

public static ISettingType Create(ApplicationSetting appSetting) { object setting = DeserializeValue(appSetting); var settingType = setting as ISettingType; if (settingType != null) return settingType; throw new InvalidOperationException("Setting is not of a complex type requiring an ISettingType"); } 

IMO every scope should have its braces, so that if scope needs braces - yes, even for one single-line return statement.

Why is settting explicitly declared as object when var is sporadically used elsewhere? I'm not sure I'd be able to grasp when to and when not to use var in that code base. Besides, it could be simplified to this:

var result = DeserializeValue(appSetting) as ISettingType; if (result == null) { throw new InvalidOperationException(...); } return result; 

Actually, that exception should probably be a more specific ArgumentException. After all, an InvalidOperationException is normally thrown when the state of the object doesn't allow that operation - and we're dealing with a static method, on a static class: having state in this context would be very questionable.. and the problem stems from the supplied argument anyway - I'd definitely go with an ArgumentException here.

In the circumstances, I like that you're throwing an exception here, instead of just returning null: it makes that much less for the caller to care about already.

---

This is worrying:

Type settingType = Type.GetType(MODEL_NAMESPACE + appSetting.SettingName); 

That means the settings namespace is carved in stone, cast in concrete, completely unrefactorable (that needs to be a word). If some smartypants newcomer comes in and moves all settings to Company.Models.Settings, and doesn't update the MODEL_NAMESPACE constant, everything falls apart. Why is that constant declared so far from where it's used anyway? Clearly it belongs in the DeserializeValue scope.

I don't like that there's a magic string in the data that needs to match not only the type, but also the namespace of that type. I think I'd prefer switching over an enum and store an int value instead; there would be some static dictionary mapping enum values to setting types, with a comment explaining why each enum has an explicit value:

// these values are stored in the db; DO NOT CHANGE THEM! private static readonly IDictionary<SettingType, Type> SettingTypeMap = new Dictionary<SettingType, Type> { { SettingType.Foo, typeof(FooSetting) }, { SettingType.Bar, typeof(BarSetting) }, //{ SettingType.Smurf, typeof(SmurfSetting) }, }; 

And that eliminates the frail Type.GetType code, and now if a happy coder walks in and moves all settings elsewhere, or renames them, nothing breaks.

---

I have a hard time seeing why this one needs to return an object:

private static object ConvertToNumber(string str) { decimal number; if (decimal.TryParse(str, NumberStyles.Any, NumberFormatInfo.InvariantInfo, out number)) { if (number % 1 == 0) { return (long)number; } return number; } return null; } 

It returns either a decimal, a long (Int64? Really?), or a null reference, and incurs a boxing penalty on the way out, which nullifies the type safety of generics when you un-box it here:

return (T) DeserializeValue(appSetting); 

Not to mention, that's presumably casting the long back into an int most of the time. Why not give it the T and make it generic?

private static Nullable<T> ConvertToNumber<T>(string str) { decimal number; if (decimal.TryParse(str, NumberStyles.Any, ...) { return new Nullable<T>(number); } return null; } 

That way you're not boxing the value type into an object, and you're only casting to the type the caller asked for - i.e. if an int is needed, then you're not returning a long.

But then again, half of that is moot if every setting implements the ISettingType interface.