Avoid using object to hold int, bool, double application setting values

Question

I have some code to handle integer, double, Boolean, and string settings. Right now, each setting variant extends an abstract class, where the setting value is stored as an object.

Is there any way to avoid this? What I'd like to do is have IntSetting objects return their Value as an integer, DoubleSetting objects return their Value as a double and so on and so forth.

Here is the code referenced above:

public abstract class ModelSetting
{
 public object Value { get; set; }
 public abstract string ToString();
 public ModelSetting(object value)
 {
 Value = value;
 }
}
public class IntSetting : ModelSetting
{
 public IntSetting(int value) 
 : base(value)
 {
 }
 public override string ToString()
 {
 return (bool)Value ? "1" : "0";
 }
}
public class DoubleSetting : ModelSetting
{
 public DoubleSetting(double value)
 : base(value)
 {
 }
 public override string ToString()
 {
 return ((double)Value).ToString("%g");
 }
}

• Heslacher: This seems to be pseudo code IntSetting => (bool)Value ? "1" : "0"; and would be off topic here.
• Ben Aaronson: @Heslacher I think it's real code with a glaring bug

Answer 1

As you say, returning an object isn't ideal. This is a relatively straightforward use case for generics:

public abstract class ModelSetting<T>
{
 public T Value { get; set; }
 public abstract string ToString();
 public ModelSetting(T value)
 {
 Value = value;
 }
}

Then implementing classes can use:

public class DoubleSetting : ModelSetting<double>
{
 //...The rest of the class
}

---

This:

public abstract string ToString();

Is bad. ToString is already defined on Object, so by declaring an abstract version here, you're not overriding, you're hiding. This is far less frequently done, and for good reason- it breaks our basic expectations of polymorphism. You should only do it with good reason, and you don't have one here. Just get rid of the ToString redeclaration, it gives you nothing.

---

You require a value in your constructor, but then let Value be set publicly. This doesn't really make much sense- somebody could just pass default(T) to the constructor, making the constructor meaningless. You're giving two different ways to set the value, for no particular reason. It's not a big deal, but is needlessly confusing.

Consider whether you actually need to be able to change Value for an existing ModelSetting instance. If so, remove the constructor. If not (and this is probably better!), make the property private set. This makes the value immutable, which makes it much easier to reason about the class. For example, if I hold an instance, I know I can pass that instance to whoever I want without having to worry about them changing the value without me expecting it.

---

return (bool)Value ? "1" : "0";

Shouldn't this be the BoolSetting version, rather than the IntSetting one? At least you can take comfort that this bug is a demonstration of one of the advantages of the strong-typedness that you get from the generic approach!

• rookie: One more question before I accept your answer. How should I store the settings in a collection? What I'd like is a mapping of string values to ModelSetting objects (i.e. a Dictionary<string, ModelSetting>). Is this possible?
• rookie: I decided to use a Dictionary<string, object> and hide the casting details with methods like public void AddSetting<T>(string key, ModelSetting<T> value). Thanks again!
• mgw854: You could also define a non-generic interface IModelSetting that ModelSetting<T> extends. Then your Dictionary is a little more strongly typed (you know you can only have IModelSettings). Alternatively, you could use an abstract class. I've done both in similar situations.