Inheritance/OO design question

Question

This will probably wind up being a fairly simple question, but it needs some background first... I'm modelling various hardware switches, which can be turned on and off. The switches are configurable, allowing a user to specify the numeric values that are sent to the hardware to signal that it should turn on or off. This is the Switch class:-

public class Switch
{
    public Id { get; set; }
    public SwitchConfig Config { get; set; }

    public void TurnOn()
    {
        WriteToSerialPort(Id, Config.OnValue);
    } 

    public void TurnOff()
    {
        WriteToSerialPort(Id, Config.OffValue);
    } 
}

public class SwitchConfig
{
    public int OnValue { get; set; }
    public int OffValue { get; set; }
}

So far so good.

I now need to model a new type of switch that can not only be turned on and off, but can also have a temperature threshold set, so it seems sensible to inherit from the Switch class. The configuration will also need extending to include min/max permitted temperatures, e.g.:-

public TemperatureSwitchConfig : SwitchConfig
{
    public int MinTemperature { get; set; }
    public int MaxTemperature { get; set; }
}

The specialised switch class will look something like this:-

public TemperatureSwitch : Switch
{
    public void SetTemperature(int temperature)
    {
        var temperatureSwitchConfig = Config as TemperatureSwitchConfig;

        if (temperature < temperatureSwitchConfig.MinTemperature
            || temperature > temperatureSwitchConfig.MaxTemperature)
        {
             return;
        }

        WriteToSerialPort(Id, temperature);
    }
}

It doesn't feel "right" where I'm having to cast the Config property. I'm sure there must be a slicker OO solution to this, but I think "analysis paralysis" has set in and I can't see the wood for the trees! Or am I worrying over nothing - is the casting an acceptable solution in this scenario?

Answer 1

I recommend you to remove Config property from the class and put all the properties of it into the class itself. Then, you set those properties in constructor of each switch or as properties.

You say that SwitchConfig comes from XML serialization. What if you want to parametrize that serialization in some way, this is usually done using attributes in model classes (SwitchConfig in your case). Then the SwitchConfing and transitively Switch becomes dependent on module for XML serialization.

What if you would want to create the switches based on something else than XML? Then you would need to needlessly create the Config classes, instead of setting the parameters in constructors or properties.

Last thing that comes to mind is: How do you create concrete type of Switch class? Eg. how do you decide to create Switch or TemperatureSwitch? Probably based on type of SwitchConfig. If you already have code like this, why not just make it into full-featured factory, that builds specific switches based on XML data? And the XML serialization can be encapsulated internally inside this factory.

And if you are worrying about having to change many things when configuration changes, then you can do what Karl Bielefeldt said and use weak-typed deserialization instead of explicit class model.

Answer 2

Your comment about needing to generate the Config from xml adds a whole new dimension to the problem. Strong types are most helpful at compile time, which isn't applicable here. My inclination would be to instead create a Dictionary to hold the parsed config, and use it like:

public TemperatureSwitch : Switch
{
    public void SetTemperature(int temperature)
    {
        if (temperature < config["MinTemperature"]
         || temperature > config["MaxTemperature"])
        {
             return;
        }

        WriteToSerialPort(Id, temperature);
    }
}

You can put validation into your parser code, or into the setter code. Alternately, you could just forego the Config altogether, and have your xml parser code create the Switch objects directly, with individual fields for the config items.

Answer 3

You can make the config a parameter:

public abstract class Switch<TConfig> where TConfig : SwitchConfig
{
    public Id { get; set; }
    public TConfig { get; set; }

    public void TurnOn()
    {
        WriteToSerialPort(Id, Config.OnValue);
    } 

    public void TurnOff()
    {
        WriteToSerialPort(Id, Config.OffValue);
    } 
}

public class NormalSwitch : Switch<SwitchConfig>
{
}

public TemperatureSwitch : Switch<TemperatureSwitchConfig>
{
    public void SetTemperature(int temperature)
    {

        if (temperature < Config.MinTemperature
            || temperature > Config.MaxTemperature)
        {
             return;
        }

        WriteToSerialPort(Id, temperature);
    }
}

Answer 4

Well, you can use the C# generics (or equivalent in your choice of language). so basically your 1st code block would look like:

    public class Switch<T> where T:SwhitchConfig
    {
       public Id { get; set; }
       public T Config { get; set; }

       public void TurnOn()
       {
           WriteToSerialPort(Id, Config.OnValue);
       } 

       public void TurnOff()
       {
            WriteToSerialPort(Id, Config.OffValue);
       } 
   }

   public class SwitchConfig
   {
       public int OnValue { get; set; }
       public int OffValue { get; set; }
   }

And your specialized implementation would look like:

    public TemperatureSwitch : Switch<TemperatureSwitchConfig>
    {

    }        

Answer 5

Most OO languages properly implement the rules of subtyping, as stated by Barbara Liskov (in "A behavioral notion of subtyping", see fig. 4). In particular, if you have a method with the signature

X method(Y arg1, Z arg2);

then you can override this method in a subclass using the signature

A method(B arg1, C arg2);

where:

• A is a subclass of X (or is X)
• B is a superclass of Y (or is Y)
• C is a superclass of Z (or is Z)

In your case, your TemperatureSwitch class could override

SwitchConfig Switch::Config();

with

TemperatureSwitchConfig Switch::Config();

Thus, if you have a variable of type TemperatureSwitch, its Config method will return you a TemperatureSwitchConfig. If you have a variable of type Switch that actually points to a TemperatureSwitch, then its Config method will return you a SwitchConfig, which is actually a TemperatureSwitchConfig (just like your current situation).

Answer 6

Disclaimer: My C# is pretty rusty. The syntax may be off.

I now need to model a new type of switch that can not only be turned on and off, but can also have a temperature threshold set, so it seems sensible to inherit from the Switch class.

A natural response, but you should be careful. Inheritance is dangerous, it shouldn't be your first reaction to a problem.

Here's my take on this:

First, since all your fields are public, it doesn't make sense to make TurnOn and TurnOff methods. It'd be slightly better to have these be static functions. The upside is that when you need to change the implementation of TurnOn/TurnOff or provide alternate implementations, you don't need to recompile anything that depends on Switch.

Next, you can easily make Switch immutable, and you should prefer immutability when possible. By making it immutable you don't have to worry about keeping track of possible state changes as you pass Switch values around in your program. It also allows you to use them as keys in dictionaries, since a key's equality and hashcode shouldn't depend on mutable state.

With that out of the way, we need to tackle the issue that there's more than one kind of switch. You need to make a decision here: do I want to make introducing new types of switches easy, or make it easy to do new things with the switches you already have? Given that these are hardware switches, it'd guess that the addition of new switch types will be a relatively rare event. If that's the case, you'd benefit from using a sum type. This will restrict us to a finite set of kinds of switches but give us a type-safe way to distinguish between them and ensure you appropriately handle each one. So, your code would go something like this (constructors omited for brevity):

// Switch.cs
public abstract Switch {
    private Switch() {} // Prevent inheritance outside of this scope

    public abstract R Match<R>(Func<NormalSwitch, R> IfNormal, Func<TemperatureSwitch, R> IfTemp);

    public sealed NormalSwitch : Switch {
        public <IdType> Id { get; }
        public SwitchConfig Config { get; }

        public R Match<R>(Func<NormalSwitch, R> IfNormal, Func<TemperatureSwitch, R> IfTemp); {
            return IfNormal(this);
        }
    }

    public sealed TemperatureSwitch : Switch {
        public <IdType> Id { get; }
        public TempSwitchConfig Config { get; }

        public R Match<R>(Func<NormalSwitch, R> IfNormal, Func<TemperatureSwitch, R> IfTemp); {
            return IfTemp(this);
        }
    }
}

// SwitchFunctions.cs
public static class SwitchFunctions {
    public static void TurnOn(NormalSwitch Switch) { ... }
    public static void TurnOff(NormalSwitch Switch) { ... }
}

// TempSwitchFunctions.cs
public static class TempSwitchFunctions {
    public static void TurnOn(TemperatureSwitch Switch) { ... }
    public static void TurnOff(TemperatureSwitch Switch) { ... }
    public static void SetTemperature(TemperatureSwitch Switch) { ... }
}

Now in your client code you may do this:

Switch switch = parseXml()
switch.match(
    IfNormal: NormalSwitch => TurnOn(NormalSwitch),
    IfTemperature: TempSwitch => {
        TurnOn(TempSwitch);
        SetTemperature(TempSwitch);
    }
);

It's all strongly-typed, and you can write alternative implementations to the switch functions without having to recompile anything that depends on the original implementations.

Uh oh, I introduced some functional programming into your code. But that's OK, since your Switch was just a data container.