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