Another refactoring today: Use the observer pattern to invert an infelicitous dependency. In and of itself, this is a modest refactoring, but its smell often co-presents with others, and unraveling it all can be tricky.
(Note: We aren’t remotely done talking about first and second-order refactorings, there are plenty more to go. But I’m not writing a catalog, I’m working a project, so when a hefty one like this comes along, that’s when I’m talking about it. You’re gettin’ em as I’m doin’ em.)
"Infelicitous dependency", eh? What could make a dependency an unhappy one? There are a lot of possibilities, but for me they usually amount to one of two cases: 1) an important abstraction is dependent on an unimportant detail, 2) it keeps me from writing a test I want to write.
A concrete example might help, and I have just the infelicitous dependency we need for this, because it fits both bills, being both a "wrong direction" dependency and an inherently anti-testing class. Aren’t we just terribly terribly lucky?
The kontentment app’s primary metaphor is that of a performance expressed using a script. As such, there are three primary uses: 1) live performance (like a crude powerpoint), 2) recorded performance (to make video overlays), and 3) editing for either kind.
The most fundamental part of this abstraction involves time & timing, and the Rhythm handles that. Rhythm is a kind of playable pauseable clock with a given duration. The API has about 10 items in it.
Some performances have backing video, others do not. In fact, any given script can be performed in either of those two modes I mentioned above, live, stepped through by a presenter, or recorded, played in time over either video or a fixed background.
MediaPlayer is how one plays video in JavaFx. One gets a Media, stuffs it in the MediaPlayer, and renders it with a MediaView. MediaPlayer is a concrete final class with no super-interface, apparently because the JDK designers are still grappling with the early ’90s.
Now, if I seek the Rhythm to, say, 30 seconds into the performance, I also need to seek the MediaPlayer 30 seconds in. Similarly, if I play/pause the Rhythm, I have to play/pause the MediaPlayer. Further, up at the top of the app, there’s a MediaView riding on the player.
And that sets up the Before picture. The Rhythm both uses a MediaPlayer internally and exposes that MediaPlayer so the MediaView can watch it. The line of dependency is an arrow from Rhythm -> MediaPlayer.
This dependency is icky, for several reasons, but I’ll just highlight 3 of them. 1) testability, 2) changeability, 3) layer crossing.
MediaPlayer is anti-test. As with many JavaFx UI classes, it does not work unless the JavaFx framework is running. Further, the only non-visual assertions you can make against it are testing its Player state. I can test it, but not cheaply, and I work for a living.
So, big deal, MediaPlayer is anti-test. That’s fine, why would I want to test it anyway, it’s not my code, and if it doesn’t work as advertised there’s very little I can do about it. It’s gonna be anti-test forever, in both the Before and the After picture.
No, that’s not the issue. The issue is that the embedded MediaPlayer inside Rhythm makes Rhythm anti-test, too. And that, I can’t abide. Rhythm is fundamental. Everything in the app is centered around it. If I can’t be perfectly confident that it’s righteous, I’m in trouble.
Changeability? First, that’s not a theoretical question. In point of fact, MediaPlayer is non-performant, doesn’t handle a broad range of important media types, and is weakly supported by the JDK team, which has better things to do. I actually currently want to change it.
But if I swap another media system in for MediaPlayer, I’m going to be putzing around with the most fundamental abstraction in my — shipping — app. I’d be virtually forced to make a long-lived feature branch, and down that road lies great risk.
Layer-crossing: Rhythm is deep in the system, 5 or 6 layers down from the screen. And the MediaView, way up near the pixels, has to reach down through those layers to get its hands on that MediaPlayer. (Further, its exposed to every intermediate layer that doesn’t use it.)
You could say, analytically, it violates the dependency inversion principle and the interface segregation principle. I just say, non-analytically, it makes my stomach hurt. Crossing conceptual layers like this is the essence of spaghetti: everything connected to everything else.
So this Before is an ugly one, and in one sentence, it comes down to that dependency arrow: Rhythm -> MediaPlayer. If I could reverse that arrow, we’d be living the dream, innit?
Enter Observer. The observer pattern does exactly this. It takes "A -> B" and changes it so that "B -> A". It does this by inventing an interface, some kind of "Observer", and adding two public API’s to A, and one internal operation.
The particulars of the interface needed for the Observer part vary. You can go generic, with a single API, something like "heySomethingChanged( something )", you can also go quite tightly custom, as I did with Rhythm. My observer has 3 api’s: play(), pause(), and seek().
The two new public API’s on A just attach or detach given instances of the Observer part to the A. In my case, the thing that owns the MediaPlayer, now way up next to the MediaView where it belongs, implements the observer interface and attaches itself to the Rhythm.
Adding & removing & notifying Observers is entirely handleable by a delegate, because it’s super-generic. It’s just a list of what’s been attached, and a single method that means "tell all the observers something". That’s good, cuz it can be implemented & tested by itself.
The final piece: going through everywhere the Rhythm called MediaPlayer and replacing it with a call to that generic RhythmWatchers list. So mediaPlayer.pause() becomes watchers.notify { pause() }. And that’s it.
So what did we do? We added a private field to Rhythm, the tested RhythmWatchers object that manages the list and does the notify’s. We exposed two of its methods as methods on Rhythm. We replaced direct calls to MediaPlayer with calls to RhythmWatchers’s notify.
We deleted the public MediaPlayer instance, and it now lives much closer to the pixels it drives, up at the top of the app. We had to implement that RhythmWatcher interface up there, too.
But what did we accomplish? In one short phrase, we "made MediaPlayer depend on Rhythm, not Rhythm depend on MediaPlayer". But of course, who cares about that. What we really did was resolve all three of the icky aspects of the Before picture.
1) Rhythm can now be rigorously and cheaply tested. 2) Changing away from MediaPlayer no longer means changing my most important class, Rhythm. 3) Intermediate layers are no longer exposed to the MediaPlayer they never cared about.
Now, I don’t want you to think that this was all dreadfully obvious, and that I, cool, calm, collected, took it in at a glance and knew what to do.
"Name?"
"Bond. GeePaw Bond."
Naw, it wasn’t like that.
I spent a bunch of time just tightening down Rhythm. It had been transliterated from Java, and had a lot of non-idiomatic stuff in it. I did that first.
Then I traced up from the MediaPlayer field to see what the clients were doing with it. There were only two, and one was just a hack function that could be implemented a different way. I did that.
Remember, part of my motivation was that Rhythm was untested. So all of that was done by making tiny steps, then doing GAK (geek at keyboard) runs to assert that my two most illustrative scripts were still working.
The generic RhythmWatchers object was next, a couple dozen lines of code, test-driven, entirely seperate from Rhythm and MediaPlayer.
I then dual-implemented, up at the top, the way the MediaView got its MediaPlayer. Variant 1 was the old way, and Variant 2 was the new one, and I just ran it both ways and made sure it worked.
Only at that point did I feel confident enough to go ahead and remove all the MediaPlayer parts of Rhythm. (Not shown in photo: Rhythm was an interface with 3 implementors, and they all came up to a single Rhythm concrete class at the end.)
There were a couple of ‘splosions along the way, too. Fortunately, they were big enough I couldn’t miss them, even testlessly. There’s one lingering bug, but it’s minor, and until I really bring Rhythm under rigorous test, I’m not gonna worry about it.
My parting shot is the theme I wish all geeks could take to their hearts in the beginning: I’m the boss. The code works for me, I don’t work for the code. When the code isn’t what I want, I change the code. In this case, the code was messing up my other work. So it had to go.
The observer pattern exists to invert a dependency, to turn A -> B into B -> A. It’s conceptually straightforward, but it can be a little hairy if the Before is messy, and it often is. The benefit, though, of inverting dependencies, is often huge and multiple.
The GeePaw Podcast
If you love the GeePaw Podcast, show your support with a monthly donation to help keep the content flowing. Support GeePaw Here. You can also show your support by sending in voice messages to be included in the podcasts. These can be questions, comments, etc. Submit Voice Message Here.