Collaborators vs. More Functional Interface

[Brian Capozzi]

Uncle Bob -

I have been thinking a bit about moving toward more functional design of certain aspects of a system I am working on.  As such, I have a design question for you.  

Specifically, I have a class LocalController which is responsible for managing access to a specific airport runway.  In general, the runway may be desired to be used by aircraft wanting to take off, those wanting to land, or by aircraft that must simply taxi across the runway to get somewhere else.  As such, this LocalController makes use of estimates of the "demand" for each of these different types.  It then applies a number of different rules to determine what sort of operation to clear onto the runway next.  At the moment, it gains access to this demand by querying a number of collaborators that are injected into the LocalController at construction time by "main" (technically via a factory that is called by main on application startup).  Collaborators include an ApproachMonitor (to get a view of landing aircraft) and various RunwayQueue(s) that provide a view of departing and taxiing aircraft.

The actual interface to the LocalController from the system perspective is:

void update(long timestamp);

List<Clearance> getClearancesToIssue();

Thus, we first call a generic update method to effectively "poll" the various demand sources and make decisions and then subsequently call a method to get any movement "clearances" that are to be delivered to the simulated aircraft.

The tests I have written for this class, in order to "arrange" the state of the system, end up of course either creating actual instances of the collaborators and setting their instance variables or, in most cases, creating mocks (I use jmock at the moment).  In the case of creating mocks, I end up having to add expectations on the return values of the collaborator methods that I know get called internally within the update() method of the LocalController to update its picture of the demand state.  These collaborations originally came about as tests revealed that the LocalController needed certain knowledge about certain aspects, and it seemed to make sense to partition the management of some of that knowledge outside of the LocalController so that it could be about the rules, and not the data.  However, as I read the tests, it seems in some sense like having to specify expectations on the collaborators is revealing a bit too much of the implementation.  Maybe not.  But I can't help but wonder if this might make them a bit more fragile over time...?

So I was wondering if moving to a more functional interface might be cleaner:

void update(long timestamp, RunwayDemand demand)

where RunwayDemand provides the demand more directly to the LocalController.  Obviously some entity in the system would have to be responsible for creating and maintaining this object, so technically nothing would really have changed.  It is still a collaborator, but the collaborator gets passed in directly in the method call as opposed to having been injected at construction time.  But it would make testing the LocalController a bit more straightforward.  Then, the "setup" could just configure the RunwayDemand directly rather than indirectly via the collaborators.  Further, the actual run-time type that implements RunwayDemand for the actual simulation could still technically be backed by the real collaborators.

So I guess my question can be boiled down to:  do you feel it is any cleaner to depend on a number of disparate collaborators or on a directly-injectable collaborator?  I guess it comes down to whether it is more valuable to separate out the different demand types and provide type-specific collaborators or whether a "role" in the simulation is to provide a view into the various demand for the runway, providing a unified view of the different types.

Note that, as per a number of recent code casts, I have been moving toward hierarchical tests and focusing on readability of the tests.  I have created "given" classes that handle the setup so that the tests read well somewhat independent of the setup implementation.  So this is in some sense a question of the code "behind" the clean-reading "arrange" methods.  But of course it also impacts the production code within LocalController that must reason about the different demand types.

As an aside, it also occurs to me that it might even be better to combine the update() and getClearancesToIssue() into a single call:

List<Clearance> update(long timestamp, RunwayDemand demand)

I think I originally separated the update() and getClearancesToIssue() calls via some misguided application of command/query separation a while back.

Anyways, I would be interested to hear any thoughts you might have on this.

Cheers,

BC

[Uncle Bob]

It appears to me that you have talked yourself into the solution and don't really need my input.  But I'll give it anyway <grin>.

From your description it seems to me that your LocalController maintained a list of Collaborators.  Upon update, it accumulated a list of demands by polling all those collaborators.  

In your letter you talked yourself out of this and decided to simply pass the demands into the LocalController, so that the LocalController would be ignorant of where the demands came from.

I agree that's a better design.  It reduces the coupling  of the LocalController, and lessens the amount of "injection" you need to do.

However, there might be a cost.  Timing.  How current does the list of demands need to be?  If there is a significant delay between the time that the demands are gathered, and update is called, then the update might be working with stale data.  In a simulation this might not be a problem, but in a real life airport, it could be significant.  

You can get the best of both worlds by passing to update the _function_ that gathers the list of demands.  There are lots of ways to do this.  You could use a Command pattern for example.  Update would simply call the command, and the return would be the list of demands.  Another way to implement it would be to pass an iterator to update.  You write that iterator to poll the collaborators.  The update function just walks the iterator thinking it's walking a list.  It's really gathering the demands in real time.

In a function language, like Clojure, we would pass a lazy-list of demands into update.  The lazy list works just like the iterator, evaluating the demands as they are requested from the list.

I hope this helps.  In any case, I think your thought process was on target.