Logbook 5
A Logbook entry written by @MendelMonteiro
Thomas, Tomasz and I were present for this session.
We started the session with the objective of getting our test harness to actually test something a little more realistic (i.e. where we could interact not only with the investor but also with the markets). We took a little detour when we came back to talking about the meaning and responsibilities of a port/adapter. We finally agreed that the port part handles all the transport related functionality whilst the adapter part handles the conversion to the DTOs expected by the interfaces exposed by the domain.
We also hashed out exactly where we were going to box off the SOR in terms of what would be re-implemented in each of our target languages/frameworks/patterns (i.e. F#, Michonne, Disruptor, etc...). We agreed that we wanted to include all of the inner domain hexaqon in the interchangeble part which means the implementations would directly expose or consume the interfaces which bound the domain.
My fingers were getting a little itchy by this stage so I proposed that we keep our momentum up by advancing on the market facing interfaces.
First off we got a little refactoring out of the way so as to match what we had drawn up on the whiteboard in the previous session. The ISmartOrderRoutingService became ISmartOrderRoutingRawInprocPort (it's a port/adapter even if the 'transport layer' is just method calls. Also, ISmartOrderRouting became ISmartOrderRoutingEntryPoint.
Next up we decided to have a look at the SOR and Solver to see what they were going to need from the Market facing interfaces. We decided to make these interfaces as granular as possible and chose a first person naming convention for them (this was Tomazs' suggestion which I quite liked and wanted to see how the code read - though I admit the first impression left me expecting to see a photo of a cat on the next page).
The responsibilities got condensed down into the following interfaces:
- List of markets - IProvideMarkets
- Market data for an instrument - ICanReceiveMarketData
- Placing an order on a market - ICanRouteOrders
As we started changing the unit tests to inject these interfaces into the SOR and then start using them in the SOR we realised that the changes were going to be fairly significant and that we were going to be hard pressed to finish them in the 45 minutes that remained. We kept the interfaces as clean and as granular as possible whilst being as expedient/naive as possible in their implementations.
There's a bit of refactoring still left to be done especially around the IMarket interface and Market class. We left a bit of duplication/confusion between the IMarket and Market (Market doesn't implement IMarket for starters). Basically IMarket is an external artifact whereas Market is internal to the SOR.
As we started consuming the market data interface in the solver we realised that we needed a buffer between the market data feeds for a selection of markets and the Solver. We implemented this as the MarketSnapshotProvider.
Ding-dong! End of the session :-(
The project is building and most of the tests are passing - it would have been great to have another 15 minutes or so to tie up a few loose ends but it will have to wait for next time.
As I'm heading off to greener pastures this will be my last contribution to the on-site sessions but hopefully it won't be too long before there's a stable API up that I can use to implement my version of the SOR :)
A Logbook entry written by @tpierrain
... was my first disclaimer when we started our session with Cyrille, Ozgur and Tomasz.
Indeed. Mendel starting a new job elsewhere today, I was the only remaining ambassador for what we quickly did last Wednesday.
"Hey guys, you have to keep in mind that we are in the middle of a refactoring right now... ... there are many flaws to fix or refactor, but let's do that only once all our acceptance tests will be green again. Ok?"
Even if I repeated it louder, I couldn't get the full attention of the mob. Cyrille in particular, was really bothered by the fact that we added so many new interfaces and concrete types, increasing the overall complexity of the solution (i.e. IMarket, IProvideMarkets, ICanReceiveMarketData, ICanRouteOrders, MarketSnapshot, MarketSnapshotProvider, MarketDataProvider, etc).
He was jumping into the code in a stroboscopic mode (i.e. executing 25-per-second "Cltr-Alt" within Visual Studio), trying to understand why we still had 2 failing acceptance tests:
-
the one checking that our MarketSweepExecution strategy is executing the appropriated quantities on the Markets
-
and the one ensuring our SOR is discarding any Market after 3 failures
Yes, Cyrille was trying to figure out by himself & through the code the reasons why we now have this... and that... and this... and...
I must admit here that I've sweated for a while (with all mob's questions on me). Once in a while, I was painfully trying to re-sync myself into this new code to help my mates to grasp what was in front of our eyes. But without the keyboard, I never succeeded to follow the code path to the end in order to sustain my explanation (remember Cyrille's stroboscopic mood? ;-)
Anyway. We all decided to take a breath, and to calm down...
We walked away through the SOR code and we decided to simplify everything -on the fly- that prevented us to easily understand the code & why our tests were failing.
First, we got rid of the ExternalMarket property (of IMarket type) on the Market concrete class. This IMarket interface was really disrupting the mob:
-
Its name was too generic (we should have named it IMarketIdentifier instead).
-
It was an empty interface (for now), only used as a marker
-
The Market type could have been implemented it directly
Once this crazy bitch/awkward property gone, the concentration of the mob went to normal ;-) and it was easier for us to focus on the essential part:
After few more researches, we found that the recently introduced MarketSnapshotProvider type (responsible to provide a market data state-of-the-world: {prices, quantities} to the IInvestorInstructionSolver) was bugged.
Indeed, instead of keeping a reference on the provided Market instance (as arguments), the MarketSnapshotProvider was keeping a reference on a new Market() instance (something Mendel and I have quickly added while trying to make the solution building before the end of the session).
Here is the bugged code:
public MarketSnapshotProvider(IEnumerable<IMarket> marketsToWatch, ICanReceiveMarketData canReceiveMarketData)
{
canReceiveMarketData.InstrumentMarketDataUpdated += this.InstrumentMarketDataUpdated;
foreach (var market in marketsToWatch)
{
this._lastMarketUpdates[market] = new MarketInfo(new Market()); // bug: birth of a clone here ;-(
canReceiveMarketData.Subscribe(market);
}
}
In that condition, impossible for this MarketSnapshotProvider to update the proper & original Market instances (for quantity) => it was working with Market clones!!!.
You don't see it, but another code downstairs on the MarketSnapshotProvider was copying the original Market prices and quantities on those clones he was about to work with for the upcoming session.
In that context, only the clones of those Market instances were updated. Too bad for the acceptance tests that was checking the remaining quantities on the original Market instances. Because here the original Market instances were untouched, despite the fact that our SOR received the proper execution events! (from the Market clones actually ;-(
"But hey guys, did I told you to keep in mind that we were in the middle of a refactoring right now?!?"
;-P
I would say that we probably introduced too many changes (i.e. too many interfaces added) in a raw last Wednesday, making our refactoring a little bit too ambitious regarding the remaining time we had. This constraint led us to a CodeFuria that introduced some extra (even temporary) complexity and debt. Something awkward and complex enough for the mob to face difficulties to understand and to troubleshoot it (I must say here that our twice-a-week-2-hours-sessions don't help here => context switch in our brains).
Another finding here is that we should have introduced our integration point interfaces sooner (ICanRouteOrders, IInvestorService, ICanReceiveMarketData). Probably since the beginning. There is somehow a difference between YAGNI and YAGNI... ;-)
Also, the impact of the names within our code is worth noting. Here, a simple (and empty) IMarket completely disrupted the mob (everyone projecting his own ideas behind it, and couldn't see where everyone else wanted to go to) and forced us to clarify that point before we could move forward collectively.
We decided that we will continue our integration point (interface) extraction, by moving our Market concrete type outside our current project.
I also suggested that we rename our --Ports types to --Adapters ones to follow the Hexagonal Architecture convention suggested by Tomasz (you were right pal... ;-)
TBC
A Logbook comment by @cyrdup
As I did not attend the previous session, I took the keyboard, making my duty to fix the two red tests. I assumed this was the fastest way for me to resynch with the project.
Oh boy was I right, but what I did not anticipate was how many changes did happen in a single session. The failing test's objective was to ensure the SOR used a weighted average algorithm for execution. My initial idea was the algorithm got somehow disrupted by the on going refactoring. I was wrong. But looking and the code and debugging it confirmed the algo was indeed intact. But the the result was not consistent.
Browing through the code at the speed of sound (stroboscoping as @tpierrain aptly described) revealed that several interfaces have been added as well as new classes. Something was definitely fishy here. It dawned to me: what I saw was not refactoring in a TDD sense, it was an on going redesign effort.
Shortcuts have been taken, assumptions have been translated into code. Basically, risks have been taken, and they have bitten back: invariants were broken, entities were duplicated, states were corrupted. And fixing those was going to be painful.
Here are the mistakes I identified:
- clumsy refactoring: interfaces were added instead of being extracted from existing implementation. As such, they came with assumptions, a significant refactoring effort and therefore a long tunnel effect before returning to a stable code
- TDD violations: behaviour was added directly to the implementation, instead of being captured in tests first. The effect was it was not possible to assess the current behaviour as being the expected one, or not.
- Late interface identification: the market class is a mock class.But as we did not extract the target interface, it was possible for the code to use methods/properties necessary to control the mock. Hence, it was possible to alter its state unexpectedly.
That being said, I need to rest now.
A Logbook comment on comment ;-) by @tpierrain
Thanks Cyrille for your comment on day 17.
Refactoring, redesigning,... I won't argue too much on this, but my mum and Martin Fowler always told me that if I don't change any of the existing behaviour/contract for the system, I could call it: "Refactoring". Here, our existing contracts being illustrated by our acceptance tests (i.e. system behavours), I see nothing but ol' classic TDD things (RED, GREEN, we were here> REFACTOR <we were here).
But I agreed on the fact that we probably introduced too many changes in a raw (the notion of snapshot in particular). We clearly missed an occasion to execute smaller baby steps in the process.
Regarding how we were extracting those new interfaces now, let me clarify a little bit more:
Changing the internal structure of our SOR blackbox by splitting responsibilities of our Market concrete type in 2 categories/interfaces -wherever it was used- was our initial intent.
The 2 interfaces being: ICanReceiveMarketData & ICanRouteOrders, we were trying to make them emerging from the underbelly of the SOR. Adding on-the-fly members to them from every location we had in our code related to whether market data retrieval or order routing.
Again for us, it was nothing but changing the internal structure of the code. And since the external contract was almost unchanged: we didn't introduced any white-box tests during our attempt to make it build (and GREEN) in the very short remaining time.