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.

I'm developing an app that will be used to open and close valves in an industrial environment, and was thinking of something simple like this:-

public static void ValveController
{
    public static void OpenValve(string valveName)
    {
        // Implementation to open the valve
    }

    public static void CloseValve(string valveName)
    {
        // Implementation to close the valve
    }
}

(The implementation would write a few bytes of data to the serial port to control the valve - an "address" derived from the valve name, and either a "1" or "0" to open or close the valve).

Another dev asked whether we should instead create a separate class for each physical valve, of which there are dozens. I agree it would be nicer to write code like PlasmaValve.Open() rather than ValveController.OpenValve("plasma"), but is this overkill?

Also, I was wondering how best to tackle the design with a couple of hypothetical future requirements in mind:-

  1. We are asked to support a new type of valve requiring different values to open and close it (not 0 and 1).
  2. We are asked to support a valve that can be set to any position from 0-100, rather than simply "open" or "closed".

Normally I would use inheritance for this kind of thing, but I've recently started to get my head around "composition over inheritance" and wonder if there is a slicker solution to be had using composition?

share|improve this question
2  
I would create a generic valve class which has an identifier for the specific valve (not a string, perhaps an enum) and any information necessary for control flow inside of the OpenValve/CloseValve methods. Alternatively you could make the valv class abstract and make separate implementations for each one, where open/close valve just calls the logic inside the given valve class for the event that different valves have different opening/closing mechanisms. The common mechanism would be defined in the base class. –  Jimmy Hoffa Oct 15 '12 at 15:10
2  
Don't worry about hypothetical future requirements. YAGNI. –  pdr Oct 15 '12 at 15:16
3  
@pdr YAGNI is a double edged blade, I agree it's worth following in the general, but taken to the extreme one could say doing anything to help future maintainability or readability is violating YAGNI, because of this I find the scope of YAGNI too ambiguous for many. That said, many people recognize where to use YAGNI and where to toss it because accounting for the future will save you serious pain. I just think one should be careful suggesting folks follow YAGNI when you don't know where they'll land on that spectrum. –  Jimmy Hoffa Oct 15 '12 at 15:23
2  
Man, 'composition over inhertiance' is overrated. I'd make a Valve abstract class/interface and then subclass them into PlasmaValve. And then I'd make sure that my ValveController would work with Valve(s), not caring about which subclass they are exactly. –  MrFox Oct 15 '12 at 15:30
2  
@suslik: Absolutely. I've also seen excellent code called spaghetti by people who don't understand SOLID principles. We could go on forever with this. My point is that I've seen more problems caused by dismissing established principles (born of many years' experience) out-of-hand than I've seen caused by over-adherence. But I agree that both extremes are dangerous. –  pdr Oct 15 '12 at 16:17
show 5 more comments

2 Answers

up vote 12 down vote accepted

If each instance of the valve object would run the same code as this ValveController, then it seems like multiple instances of a single class would be the right way to go. In this case just configure which valve it controls(and how) in the valve object's constructor.

However if each valve control needs different code to run, and the current ValveController is running a giant switch statement that does different things depending on the type of valve, then you have reimplemented polymorphism poorly. In that case rewrite it to multiple classes with a common base(if that makes sense) and let the single responsibility principle be your design guide.

share|improve this answer
1  
+1 for mentioning type-based switch statements as code smell. I frequently see these kinds of switch statements where the developer claims he was just following KISS. Perfect example of how design principles can be perverted heh –  Jimmy Hoffa Oct 15 '12 at 15:20
2  
Multiple instances might also make it easier to link valves in a sequence, enabling you to model the actual plant piping as a directed graph in your code. You could also then add business logic to the classes, in case you need to do something like open one valve when another one closes to avoid pressure build-up, or close all downstream valves so you don't get a "water hammer" effect when the valve is opened again. –  TMN Oct 15 '12 at 15:48
add comment

My major gripe is using strings for the parameter identifying the valve.

At least create a Valve class which has a getAddress in the form the underlying implementation needs and pass those to the ValveController and ensure that you can't create nonexistent valves. This way you won't have to handle wrong strings in each of the open and close method.

Whether you create convenience methods that call the open and close in ValveController is up to you, but to be honest I would keep all the communication to the serial port (including the encoding) in a single class which other classes will call when needed. This means that when you need to migrate to a new controller you only need to modify one class.

If you like testing you should also make ValveController a singleton so you can mock it (or create a training machine for the operators).

share|improve this answer
    
I have never seen anyone recommend a singleton for sake of testing before--usually it goes the other way. –  Kazark Oct 18 '12 at 16:42
    
honestly the singleton is more to avoid the statics and so the communication can be synchronized –  ratchet freak Oct 18 '12 at 17:02
add comment

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.