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 after a bit of OO design advice... I'm about to start developing a Windows application that communicates with an external machine via RS232. The machine has an onboard "system controller" consisting of registers (addresses). You can write a value to a register (e.g. to turn a pump on), or read a value from a register (e.g. to see if the pump is running). The app will have a config file detailing each register's settings, e.g. its address, whether it's 16- or 32-bit, a friendly "alias" (so other code can deal with "Pump XYZ" rather than address "50B3D124"), and a few other things.

My first thought was to have a Register class reflecting these properties, plus Read() and Write() methods. But this raises the first question - should the Read() method have a return value, or should it populate a Value property on the class?

Similarly, should the Write() method include a "valueToWrite" parameter, or should it write whatever value is currently held in the Value property?

I could take this one step further (possibly even answering the above question) - instead of Read/Write methods, put the functionality into the Value property's getter and setter? Using a property for this purpose doesn't feel right though.

I then started wondering if a Register class was even necessary, as its read/write functionality would be doing little more than calling Read/Write methods on a SystemController class, e.g.

long ReadRegister(string registerAlias);
void WriteRegister(string registerAlias, long value);

Why not just let the rest of the application read/write registers via the SystemController class? (I suspect I would still need some concept of a Register class, even if it's just to represent a register's config settings).

share|improve this question
    
Assuming that you do not want to use Java, the Net 4 and Net 4.5 frameworks have serialStream clases that are well defined and fairly well abstracted in both dot.Net C# and VB. –  user121071 Feb 24 at 10:28

2 Answers 2

up vote 4 down vote accepted

But this raises the first question - should the Read() method have a return value, or should it populate a Value property on the class? Similarly, should the Write() method include a "valueToWrite" parameter, or should it write whatever value is currently held in the Value property?

If you use a Value property instead of returning a value from Read(), you can "write" values to the Register via Write() but still see an old value until you call Read(). you've introduced unnecessary mutation into your implementation, and more importantly that's not how the hardware registers work.

Your Register class should be immutable, only store the minimal amount of information required to access the real hardware register (probably just the address), and should hide that information from the outside world. The only thing you should be able to do with a Register variable is read, write, and check if it's the same hardware register as another Register. For example, in Java:

public final class Register<T extends Number> {
    public static final Register<Short> PUMP_XYZ = new Register<Short>(0xA3F990);
    public static final Register<Integer> PUMP_ABC = new Register<Integer>(0xA3F9A0);
    // and so on

    private final int address;

    private Register(int address) {
        this.address = address;
    }

    public T read() throws CommunicationException {
        return /* read hardware */
    }

    public void write(T value) throws CommunicationException {
        /* write to hardware */
    }

    @Override
    public boolean equals(Object other) {
        if (other instanceof Register) {
            return this.address == ((Register) other).address;
        } else {
            return false;
        }
    }

    @Override
    public int hashCode() {
        return address;
    }

    @Override
    public String toString() {
        return String.format("Register(0x%h)", address);
    }

    public static final class CommunicationException extends RuntimeException {}
}

This is the simplest implementation that captures all the essential details of a real register. It's also immutable and disallows inheritance, both of which you should prefer when possible.

It's tempting to attach a printable name to Register, but that's not really a part of what you're trying to model. A hardware register has no notion of a name. It'd be better to keep that separate and create a Map<Register, String> or a simple struct which holds a String and Register field.

Why not just let the rest of the application read/write registers via the SystemController class?

One day you might change the communication protocol to be USB or use a different API to make the RS-232 calls. The application doesn't care how the registers are accessed and shouldn't have to know; it's an implementation detail.

share|improve this answer
1  
You might consider discussing the addition of more abstraction layers (e.g. so you can call Pump1.Start()), but what you have now is definitely a solid answer. –  paul Feb 21 at 22:00
    
@paul That's true; working at the level of bytes isn't very useful. But without more details, it'd be impossible to delve into it much further. –  Doval Feb 22 at 5:29
1  
Some useful comments thanks. It's funny how one can get bogged down over-thinking a design. I was dead-set on having a "SystemController" class (mirroring the physical entity) that contained the logic for reading and writing registers - hence the second part of my question asking if the "Register" class was necessary. Couple this with some kind of "register provider" that lets a consumer retrieve an instance of a Register based on a given string alias, and I should be sorted. –  Andrew Stephens Feb 24 at 10:41
    
@AndrewStephens In retrospect, having a SystemController class is a cleaner design. I shouldn't have coupled the Register class with communication logic. Even so, it's still worth keeping, because it makes sure only valid values are used in calls to Read and Write. You could make Register a static inner Enum of SystemController, allowing the latter to access the Register's private address field while keeping it private to everyone else. –  Doval Feb 24 at 12:14
    
@AndrewStephens On that note, SystemController doesn't necessarily have to be an object you can instantiate. If you'll always use the same communication info (i.e. always connect to the same remote machine with the same parameters) it could simply be a collection of static methods with the communication parameters hard-coded (perhaps in private static variables.) You could go all the way and make SystemController an Interface but unless you need to use various kinds of communication protocols I'd consider that overkill and a premature generalization. Keep it simple. –  Doval Feb 24 at 12:37

There are several approaches that can work; which is best may depend upon the speed and reliability of the communications protocol, the nature of the operations, and the extent to which use of the program will "flow" better if users wait for each request to complete before proceeding to the next, enter requests as fast as they wish and have the program handle them as fast as it can, or have users enter all requests and then process them as a group.

If you want the user interface to wait while each operation is performed, then having methods which simply perform operations directly but don't keep any state may be the simplest way of going about it. Such an approach is simple, and generally makes it easy to tell what's going on. If the remote end of the connection has a few important aspects of state which affect the way communications will be performed (e.g. communications mode settings, or packet sequence numbers), it may be helpful to have the class manage those aspects of state itself, but aspects of state other than communication should be managed by client code.

An alternative design is to have "read all state" and "write all state" methods, and have the UI flow entail loading state from a device or opting to create new state, editing the state without involving the device, and then storing the state to the device once editing is complete. This approach will minimize the number of times a user will have to wait for the device, consolidating all delays into one wait at the beginning and one at the end. The one limitation with this approach is that the user won't be able to see the effect of any changes on the remote device until the "apply" button is pressed (since no changes will actually be made until then). Whether that's a good or bad thing will depend upon the nature of the device.

A final approach is to keep two main sets of state--what the device has last reported, and what changes (if any) have been requested but not yet applied; along with that one may wish to keep track of how many unsuccessful attempts have been made to perform various operations. One might for each device property display a read-only and editable field next to each other. A background task should scan the properties and determine what things need updating on either the screen or in the device, and perform any updates as it notices them. I would suggest having the read-only text boxes implemented with a custom type with a method that can set the text at any time from any thread. The method should immediately set a private variable to the new text and BeginInvoke an UpdateText method when no update request is already pending. If multiple requests are made to set the text before the UI thread has started processing the first one, the repeated operations should not generate multiple BeginInvoke calls. Instead, the later operations should simply change the string which will get shown when the UpdateText method finally starts executing.

An important thing to bear in mind is that while the actual properties of the physical device and the desired properties would ideally be one and the same, they are separate aspects of state and should probably be both maintained and (especially with the third approach) displayed as such. If for whatever reason, reading back the device state after setting it doesn't yield the new value, code should be prepared to convey that information to the user. The user might be confused as to why the value isn't updating, but will probably be less confused than if software suggests that the device holds the new value but it really doesn't.

share|improve this answer

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.