Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
    
Why do you need config? Why not just put the fields into the switch itself? –  Euphoric Apr 25 at 12:10
    
Why don't you simply inject the Config in your method? Your SetTemperature signature would look like this SetTemperature(int temperature, TemperatureSwitchConfig config) –  im_a_noob Apr 25 at 12:11
    
@Euphoric see my comments on the answer from Geerten below. –  Andrew Stephens Apr 25 at 12:34

6 Answers 6

up vote 1 down vote accepted

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.

share|improve this answer

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.

share|improve this answer
2  
There's no reason you can't have static typing just because you're parsing XML at runtime. –  Doval Apr 25 at 13:17
    
Of course. If you notice, I proposed that as an alternate. It's just that most of the benefits of static typing happen at compile time. If you have to do the checking at runtime anyway, you may as well take advantage of that if it simplifies your code. Note that the dictionary has a static type. –  Karl Bielefeldt Apr 25 at 14:27
    
The dictionary fails to distinguish between the different kinds of configs though. Now, given a dictionary, I don't know if it has 1) all the fields required of a regular switch's configs; 2) all the fields required of a temperature switch's configs; or 3) neither. In order to avoid #3, you'd have to check that #1 or #2 before you return the dictionary, and at that point you might as well return actual Config types. –  Doval Apr 25 at 15:07

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);
    }
}
share|improve this answer
    
Programmers is tour conceptual questions and answers are expected to explain things. Throwing code dumps instead of explanation is like copying code from IDE to whiteboard: it may look familiar and even sometimes be understandable, but it feels weird... just weird. Whiteboard doesn't have compiler –  gnat Apr 25 at 13:06
    
The answer is "use a parameter". The code simply illustrates that. –  Tobias Brandt Apr 25 at 13:09
    
I toyed with the idea of generics but it's likely there could be further levels of inheritance (e.g. something inheriting from TemperatureSwitch). While it's still possible, it does result in an explosion of <TConfig> parameters everywhere. Something to bear in mind though. –  Andrew Stephens Apr 25 at 15:16

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>
    {

    }        
share|improve this answer
    
Programmers is tour conceptual questions and answers are expected to explain things. Throwing code dumps instead of explanation is like copying code from IDE to whiteboard: it may look familiar and even sometimes be understandable, but it feels weird... just weird. Whiteboard doesn't have compiler –  gnat Apr 25 at 13:05

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).

share|improve this answer
1  
Your explanation has nothing to do with LSP. Its just basic polymorphism of inheritance. –  Euphoric Apr 25 at 13:11
    
@Euphoric: of course it does. Quoting from wikipedia: "Liskov's principle imposes some standard requirements on signatures 1. Contravariance of method arguments in the subtype 2. Covariance of return types in the subtype 3. No new exceptions should be thrown by methods of the subtype, except where those exceptions are themselves subtypes of exceptions thrown by the methods of the supertype." –  barjak Apr 25 at 13:22
1  
That is just effect of applying LSP. Not LSP itself. Quoting from wikipedia: "It states that, in a computer program, if S is a subtype of T, then objects of type T may be replaced with objects of type S (i.e., objects of type S may substitute objects of type T) without altering any of the desirable properties of that program" –  Euphoric Apr 25 at 13:34
    
My opinion is that the Liskov substitution principle encompasses more than you think it does. I really think the very definition of the Subtype Relation (the constraint rule) is an integral part of LSP. The rules about the covariance of return type and contravariance of argument types are stated in Liskov's original paper, "A behavioral notion of subtyping", see fig. 4. I give you the benefit of the doubt, though, by editing the first sentence of my answer to not refer to LSP directly. –  barjak Apr 25 at 14:21

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.

share|improve this answer
    
Downvoter: Please explain your reasoning, in detail. –  Doval Apr 25 at 13:18
1  
Your approach came to mind when I first started this application. Rather than strictly adhering to the rule that a class should contain both data and behaviour, I wanted another class to be responsible for turning switches on and off. This seemed a more natural fit for my scenario - there are various dependencies involved in controlling a switch (e.g. writing to a serial port), and passing these to a whole load of "model" classes didn't feel quite right. Unfortunately the OO purists where I work won out, so I'm stuck with putting the behaviour in the class. –  Andrew Stephens Apr 25 at 15:30
    
@AndrewStephens That's unfortunate. You could still turn Switch into a sum type even if you leave TurnOn, TurnOff, and SetTemperature as methods and it'd solve your casting problem, but I don't know if the OO purists would like that. I'd bet you could get away with it if you used the Visitor pattern though; it's essentially the OO workaround when you don't have lambdas and delegates. –  Doval Apr 25 at 15:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.