patterncsharpModerate
Avoid using object to hold int, bool, double application setting values
Viewed 0 times
applicationboolavoidholddoublesettingusingvaluesobjectint
Problem
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
Is there any way to avoid this? What I'd like to do is have
Here is the code referenced above:
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");
}
}Solution
As you say, returning an
Then implementing classes can use:
This:
Is bad.
You require a
Consider whether you actually need to be able to change
Shouldn't this be the
object isn't ideal. This is a relatively straightforward use case for generics:public abstract class ModelSetting
{
public T Value { get; set; }
public abstract string ToString();
public ModelSetting(T value)
{
Value = value;
}
}Then implementing classes can use:
public class DoubleSetting : ModelSetting
{
//...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!Code Snippets
public abstract class ModelSetting<T>
{
public T Value { get; set; }
public abstract string ToString();
public ModelSetting(T value)
{
Value = value;
}
}public class DoubleSetting : ModelSetting<double>
{
//...The rest of the class
}public abstract string ToString();return (bool)Value ? "1" : "0";Context
StackExchange Code Review Q#96089, answer score: 10
Revisions (0)
No revisions yet.