Monday, 11 February 2008

The Visitor Pattern eliminates enums from the Observer Pattern

To avoid boolean parameters, every now and then in a moment of weakness I do something like this:


public void addFile(File file, InputOutput inputOrOutput) {
switch(inputOrOutput) {
case InputOrOutput.INPUT: ... break;
case InputOrOutput.OUTPUT: ... break;
default: assert false;
}
}


Which is not a very good solution. There are at least two problems:

  • I have to maintain an enum

  • There's a possibility that the parameter is null, which creates an opportunity for an error condition


I can achieve the same result by using two functions:

public void addInputFile(File file) {
...
}

public void addOutputFile(File file) {
...
}


This solves the two problems and and also satisfies the readability requirement.

How does this relate to Observers? When I use the Java Observer interface and Observable class, I usually end up with a class defining the chunk of data that describes the change. This chunk is the delta of the object state from the last notification. And a object of this class is received by the observers.

This delta class can take this form:


public class ModelObservedData {

enum Type { ADDED, REMOVED };

// Members that contain the data
...

public ModelObservedData (Type type, Data data) {
...
}

public Type getType() {
...
}

public Data getData() {
...
}

}

and then in the observer

...

update(Observable o, Object arg) {
// Assert on observable source and type or use an if when observing
// multiple sources

ModelObservedData a data = (ModelObservedData)arg;
switch(data.getType()) {
case ADDED: doAdded(data.getData()) break;
case REMOVED: doRemoved(data.getData()) break;
default: assert false;
}

}


I don't find this very elegant. Now, considering the whole multiple dispatch and visitor pattern that I wrote about, there is a more elegant solution.

Essentially, you visit the observed data instead of getting the type of the delta and switching your implementation on the type. The observed data now takes this form:


class ModelObservedData {

...

public static interface IVisitor {

void dataAdded(Data data);
void dataRemoved(Data data);

}

public void accept(IVisitor visitor) {
if (...) {
visitor.dataAdded(addedData);
} else if (...) {
visitor.dataRemoved(removedData);
}
}
}


The client implements the IVisitor interface and the update with the switch becomes an acceptance.


...

update(Observable o, Object arg) {
// Assert on observable source and type or ifs when observing
// multiple sources
ModelObservedData data = (ModelObservedData)arg;
data.accept(this);
}

void dataAdded(Data data) {
...
}

void dataRemoved(Data data) {
...
}


This mechanism doesn't have the switch in the observer, there's no more enum, and the observed data can better encapsulate the state that defines the delta.

The last bit would be to eliminate constructors of the observed data that are difficult to read. For example, you would have one constructor that receives two data parameters, the added and removed data, and stores them as fields. But then the client needs to know the ordering, and have null values in the constructor. Instead, make that constructor private, and create two factory methods that describe the construction process better:


class ModelObservedData {

Data added;
Data removed;

public static ModelObservedData createAdded(Data data)
return new ModelObservedData (data, null /* removed */);
}


public static ModelObservedData createRemoved(Data data) {
return new ModelObservedData (null /* added */, data);
}

private ModelObservedData (Data dataAdded, Date dataRemoved) {
this.dataAdded = dataAdded;
this.dataRemoved = dataRemoved;
}
}


You now have a more elegant solution (I think), with less possibility for error states and no enum to maintain. Also, the switching that each client would have to implement is now implemented only once in the observed class, so there is less duplication.

No comments: