Lets do a kata - Tennis in f# - part3 - refactor command
The tennis kata in f# with refactorings - factoring into a command handler
In Part 1 and Part 2 we came up with a solution that relied on invariants across simply tracking the number of points a played had with a complex parser to output the right values.
Upon reaching something workable it didnt feel right, every behavioural call was prefixed with a guard and moreover we depended on integer having enough numbers so that we dont overflow.
Whilst that last concern is not really a real problem (until we get ai playing tennis and both players settling on a ‘do not lose’ strategy) - it does point to a modelling failure. A game has potentially infinite nodes.
I found this challenging
I knew vaguely what I wanted, but I didn’t know how to get there. This means that concepts appear and disappear throughout these refactorings as I play with various ideas. Interestingly the code ends up being almost identical in length to the example we are refactoring - but the way it works is very different.
The endpoint is better because rather than inferring state from an int we end up modelling all the score states and transitions between them. This means adding new scores or say changing scoring rules will be simpler because everything is explicit (rather than depending upon a circumstantial invariant that applies to this form of tennis but maybe not all forms - eg badminton).
The final piece of code can be found here
It took 2 separate exercises to get there. Whilst doing this I cared greatly for never straying too far from something that would work. As a result I attempted to leave the early game part functional whilst I refactored the advantage part.
I think this would of been possible except I forgot I was trying to do this after a nights sleep.
We have a concept of early game and advantage: lets extract and formalise that
The idea was as I was doing this under a set of green tests all would be good. Those changes are not hit by executing code anywhere.
type GamePhase = | Early | Advantage | Won
type PlayerScore = | Deuce | AdvantageA | AdvantageB | WonA | WonB
type EarlyGameScore = | Love| Fifteen | Thirty | Fourty | Won
type GameState = { playerAScore: EarlyGameScore; playerBScore: EarlyGameScore; phase: GamePhase; score: PlayerScore; }
- Introduced the idea of game phases
- Formalised the scoring system and broke it down according to phases
- expanded the gamestate so that existing would work, but I have space to build off on new fields
I was expecting to be able to integrate this easily into existing and rewrite the advantage stuff.
We get a far simpler scorer function
let calculate_score (player_score:EarlyGameScore) = match player_score with
| Love -> "L"
| Fifteen -> "15"
| Thirty -> "30"
| Fourty -> "40"
| Won -> "unsupported"
let build_early_score_string state =
(calculate_score state.playerAScore) + "-" + (calculate_score state.playerBScore)
let build_advantage_string state =
match state.score with
| AdvantageA -> "Adv PlayerA"
| AdvantageB -> "Adv PlayerB"
| Deuce -> "Deuce"
| WonA -> "Player A Won"
| WonB -> "Player B Won"
match state.phase with
| Early -> build_early_score_string state
| _ -> build_advantage_string state
We are now using 2 different parts of game state at 2 different times. This connascence of meaning.
But the score function is now very simple - it has been reduces to maps via matching. Very happy.
How about the command handler?
(expensive) Logic executes on receipt of command rather than on read
We have broken the game into 2 phases. The command handler needs to apply the score commands differently depending on those phases. Whereas the getScore function simply maps the result.
Most systems have most load on the read side - so putting all the logic into the read seems folly. It is far more intuitive to have the engine process and update the state on receipt of a command in an aggregate.
Advantage game scoring
let handleAdvantage command state =
match state.score with
| Deuce -> match command with
| PlayerAScores -> { state with score = AdvantageA }
| PlayerBScores -> { state with score = AdvantageB }
| AdvantageA -> match command with
| PlayerAScores -> { state with score = WonA }
| PlayerBScores -> { state with score = Deuce }
| AdvantageB -> match command with
| PlayerAScores -> { state with score = Deuce }
| PlayerBScores -> { state with score = WonB }
| _ -> raise (PhaseHandlingError("attempting to score game that isnt in advantage phase using advantage phase rules"))
By using discriminated unions we can easily define the state space and transitions from each state in this game.
Doing this for the early part of the game would be horrendous. There are a lot of transitions as players can get to deuce (or win without the advantage game) in a wide variety of ways.
But the simplicity of this makes me think this refactor is going in the right direction.
Early game scoring
It very rapidly became apparent that taking the approach above - ie enumerating all the states and transitions by hard coding them was going to be arduous. There are many many paths that lead to any given state.
As a result I ended up taking a hybrid approach as an experiment. Because there are only 4 score states per player and we know that the getScore call will just map both players scores to strings we can greatly simplify the number of combinations whilst strongly typing the score.
This does however mean that an extension for different games would not simply be a piece of configuration now - previously we could of just changed the score is less than 40 -> early game check (40 = 3 points) to say 11 points. But for this version of tennis it is quite nice and has better symmetry with the approach for the advantage game.
let set_phase_to_advantage state =
match state.playerAScore = Fourty && state.playerBScore = Fourty with
| true -> { state with phase = Advantage }
| false -> state
let set_playerA_won state =
match state.playerAScore = Won with
| true -> { state with score = WonA; phase = Advantage }
| false -> state
let set_playerB_won state =
match state.playerBScore = Won with
| true -> { state with score = WonB; phase = Advantage }
| false -> state
let increment_playerA_score state =
match state.playerAScore with
| Love -> Fifteen
| Fifteen -> Thirty
| Thirty -> Fourty
| Fourty -> Won
| Won -> raise (WinScoreExceeded("Player A score exceeded win condition"))
let increment_playerB_score state =
match state.playerBScore with
| Love -> Fifteen
| Fifteen -> Thirty
| Thirty -> Fourty
| Fourty -> Won
| Won -> raise (WinScoreExceeded("Player B score exceeded win condition"))
let earlyGameScore command state =
match command with
| PlayerAScores -> { state with playerAScore = (increment_playerA_score state) }
| PlayerBScores -> { state with playerBScore = (increment_playerB_score state) }
|> set_phase_to_advantage
|> set_playerA_won
|> set_playerB_won
let handle command state =
match state.phase with
| Early -> earlyGameScore command state
| _ -> handleAdvantage command state
The early game on the other hand does not feel too clever. We end up in a pipeline situation again. There are a couple of steps to the pipeline
- Increment scores
- Check to see if we need to transition state - so that the score function will output right info.
… later
- Score is requested.
This feels unsucessful
Several early game functions not only change the phase of the game, but they reach into the data of the later phase and set it to the values of wonA and wonB This causes tight coupling and feels bad. Moreover its a lot of code compared to Advantage for a situation we know was very simple when we simply incremented integers. This is an example of connascence of meaning and we can turn this into connascence of type later.
Lets keep refactoring - encapsulate phases of game
If we know there are 2 game states and we have code that is switching on them then maybe we should separate the 2 sets of state into their own state classes (to stop code that accesses the same record type also accessing fields it is not supposed to). Maybe this will give us a better solution to setting win conditions also?
type EarlyGameState = {playerA: EarlyGameScore; playerB: EarlyGameScore}
type AdvantageState = {score: PlayerScore}
type GameState =
| EarlyGame of EarlyGameState
| AdvantageGame of AdvantageState
When these changes are worked into the code then the state gets cleaner,
- we match on the type of state rather than a subfield.
- We remove any early game data that advantage game could abuse.
- When we transition rather than returning a record that represents everything we simply return a
AdvantageGame {score = WonB}rather than having to copy a record overwriting some values. -it is just simpler
PlayerAWonState turns into this
let transitionToPlayerAWon state =
match state with
| EarlyGame s -> match s.playerA = Won with
| true -> AdvantageGame { score = WonA }
| false -> EarlyGame s
| AdvantageGame _ -> state
The command handler turns into this
let handle command state =
match state with
| EarlyGame s -> scoreEarlyGame command s
| AdvantageGame s -> AdvantageGame(scoreAdvantage command s)
Is this a successful refactor?
I think so
- All the scores are now typed rather than relying on inference from an integer.
- The game can go on for infinity
- It is very clear and does not rely on magic numbers
Next steps
For where we are now I am happy with this. However any new requirements may push me back to using int in early game for instance. We would probably end up extracting the various functions to enable some composition of the scoring engine.
Conclusion
Writing code is hard, getting it right first or second time even more so. Which version of this code is better? I prefer the second but with some heavy caveats regarding its extensibility, but the first got me to green and shippable value quicker at the cost of copmlexity in a part that a newcomer to the code base might expect to be simple.
I feel the second version is far far safer to extend. There are no matches on integer values anymore, each phase of the game only has the fields it requires. Rather than reaching into objects to match on their fields we are increasingly using the types of the objects themselves.
I think F# is really good. It has allowed me to solve things in a way that is fairly safe.
I used Rider on both windows and Ubuntu. I think I prefer it in Ubuntu - code coverage now works in ubuntu which is huge. This may still be lacking in c# .net core in Rider on Ubuntu.