Refactoring the Tennis Kata – was it worth ?

Veröffentlicht von

Due to the COVID-19 lockdown, I had plenty of time and decided to try myself on the Tennis Refactoring Kata. It took me about four hours for the whole refactoring, including one and a half hours to refactor my refactored code :-). It’s funny how much time you can invest in your code after all unit tests become green.

The Refactoring

First I checked out TennisGame1.cs to get some ideas in which direction I should move:

public string GetScore()
        {
            string score = "";
            var tempScore = 0;
            if (m_score1 == m_score2)
            {
                switch (m_score1)
                {
                    case 0:
                        score = "Love-All";
                        break;
                    case 1:
                        score = "Fifteen-All";
                        break;
                    case 2:
                        score = "Thirty-All";
                        break;
                    default:
                        score = "Deuce";
                        break;

                }
            }
            else if (m_score1 >= 4 || m_score2 >= 4)
            {
                var minusResult = m_score1 - m_score2;
                if (minusResult == 1) score = "Advantage player1";
                else if (minusResult == -1) score = "Advantage player2";
                else if (minusResult >= 2) score = "Win for player1";
                else score = "Win for player2";
            }
            else
            {
                for (var i = 1; i < 3; i++)
                {
                    if (i == 1) tempScore = m_score1;
                    else { score += "-"; tempScore = m_score2; }
                    switch (tempScore)
                    {
                        case 0:
                            score += "Love";
                            break;
                        case 1:
                            score += "Fifteen";
                            break;
                        case 2:
                            score += "Thirty";
                            break;
                        case 3:
                            score += "Forty";
                            break;
                    }
                }
            }
            return score;
        }

 

Based on the if - else if - else construct I had the feeling a state machine is the best choice for the refactoring, the score moves „forward“ (0-0, 15-0, 30-0, 30-15 …) and so a state machine should not be so hard to implement. Besides, I’ve watched a Pluralsight course regarding the state pattern beforehand, which might influence me at my choice.

Based on the ReadMe instructions my state machine looks like shown in the below figure:

 

 

I decided to use five states. The initial state is GameStarted, where none of the players have scored a point. After the first player scores, we switch to NormalGame. NormalGame takes care of the „standard“ point counting like love-fifteen, fifteen-all,  … . From this state, we can either switch to GameOver (if a player wins in the normal game phase) or to Deuce. The Deuce state is simple, as long as both players have an equal score of 30-30 or 40-40 we remain in this state. If one of the player scores we move to Advantage. If the other player scores we move back to Deuce, since player score is equal again. If one player scores two consecutive points in the state Advantage the game is over and we switch to GameOver.

Here is the overall class design:

 

 

We have the TennisGame4 class which is the application/tests entry point, it has two methods void WonPoint(string playerName) and string GetScore(). TennisGame4 holds a context class (ScoreContextForTwoPlayer) responsible to store the actual game state (IScore) and the scores of both players. Calls to WonPoint and GetScore are forwarded from the context to its actual IScore reference, which points to one of the derived state classes. Each state class decides in WonPoint() if it shall update the contexts IScore reference to another state object or not. Based on that the state class implementations stay simple and focused. To keep implementation easy to read I decided to move the „heavy“ computations (game won or not, deuce or not) to the NormalGameWon, GameWonInExtraTime and GameIsDeuce classes. The idea is to show the core logic in the state objects and move the harder to read code (=infrastructure) to the xxxGamexxx classes.

The state classes in detail

public abstract class ScoreBase : IScore
   {
       public ScoreBase(IScoreContext scoreContext)
           => ScoreContext = scoreContext;

       protected IScoreContext ScoreContext { get; }

       protected int Player1Score => ScoreContext.Player1Score;
       protected int Player2Score => ScoreContext.Player2Score;

       public abstract string GetScore();
       public abstract void WonPoint(string player);
   }

To reduce code duplication I decided to create a base state class to serve often used properties like Player1Score, Player2Score, and ScoreContext to the derived classes.

public sealed class GameStarted : ScoreBase
  {
      public GameStarted(IScoreContext context)
          :base(context)
          => ScoreContext.GameState = this;

      public override string GetScore() => "Love-All";

      public override void WonPoint(string player)
      {
          ScoreContext.GameState = new NormalGame(ScoreContext);
          ScoreContext.GameState.WonPoint(player);
      }
  }

GameStarted is simple. If no player has scored a point we return „Love-All“. If one player scores we’ve to switch to the next state (= NormalGame). The next time GetScore is called the result is returned from the NormalGame object.

public sealed class NormalGame : ScoreBase
   {
       private readonly GameWonInStandardMode _gameOver;
       private readonly GameIsDeuce _gameIsDeuce;
       private readonly IReadOnlyDictionary<int, string> normalGameScores = new Dictionary<int, string>
       {
           {0, "Love" },
           {1, "Fifteen" },
           {2, "Thirty" },
           {3, "Forty" },
       };

       public NormalGame(IScoreContext context)
           :base(context)
       {
           _gameOver = new GameWonInStandardMode(context);
           _gameIsDeuce = new GameIsDeuce(context);
       }

       public override string GetScore() =>
           string.Format("{0}-{1}",
                         normalGameScores[ScoreContext.Player1Score],
                         Player1Score == Player2Score ? "All" : $"{normalGameScores[ScoreContext.Player2Score]}");

       public override void WonPoint(string player)
       {
           ScoreContext.PlayerScored(player);
           _gameOver.Yes(nameOfWinner => ScoreContext.GameState = new GameOver(nameOfWinner));
           _gameIsDeuce.Yes(() => ScoreContext.GameState = new Deuce(ScoreContext));
       }
   }

NormalGame is the most complex state class. When WonPoint is called the context is updated. Afterward, we ask the _gameOver and _gameIsDeuce objects if we shall switch to the next state (= update the contexts IScore reference to point to the next state). Through moving the more complicated code to infrastructure classes the state implementations are easy to read. Creating the game score string is also simple. I used a dictionary to map the possible scores to their string representations (Love-Fifteen). In case the player score is equal the second part of the score string is replaced with „All„. If the score is Thirty-Thirty we switch to the Deuce state, so the next call to GetScore will be invoked on the Deuce object.

public sealed class Deuce : ScoreBase
  {
      public Deuce(IScoreContext context)
          :base (context)
      {
      }

      public override string GetScore() => "Deuce";

      public override void WonPoint(string player)
      {
          ScoreContext.PlayerScored(player);
          if (Player1Score != Player2Score)
              ScoreContext.GameState = new Advantage(ScoreContext);
      }
  }

Deuce  only updates to the next state if the player score is odd. As long as the context points to the Deuce object we return „Deuce“ as the score. The next stage is the Advantage state.

public sealed class Advantage : ScoreBase
   {
       private readonly GameWonInExtraTime _gameOver;
       private readonly GameIsDeuce _gameIsDeuce;

       public Advantage(IScoreContext context)
           : base(context)
       {
           _gameOver = new GameWonInExtraTime(ScoreContext);
           _gameIsDeuce = new GameIsDeuce(ScoreContext);
       }

       public override string GetScore()
           => string.Format("Advantage {0}",
                       Player1Score - Player2Score > 0
                       ? PlayerNames.Player1
                       : PlayerNames.Player2);

       public override void WonPoint(string player)
       {
           ScoreContext.PlayerScored(player);
           _gameOver.Yes(winner => ScoreContext.GameState = new GameOver(winner));
           _gameIsDeuce.Yes(() => ScoreContext.GameState = new Deuce(ScoreContext));
       }
   }

The Advantage object updates the player scores and switches either to the GameOver or to the Deuce state ( in case of the other player draws up). The infrastructure code hidden behind _gameOver and _gameIsDeuce makes the class easy to read. _gameOver and _gameIsDeuce are also realizing a kind of strategy pattern. If I’d inject both references via the constructor I could easily exchange introduce new decision algorithms.

For the sake of completeness I’ll also post the infrastructure code:

public sealed class GameIsDeuce : IChangeGamePhase

   {
       private readonly IScoreContext _context;

       public GameIsDeuce(IScoreContext context) => _context = context;

       private int Player1Score => _context.Player1Score;
       private int Player2Score => _context.Player2Score;

       private const int Thirty = 3;
       private const int Forty = 4;

       public void Yes(Action yes)
       {
           if (ThirtyBoth() || FortyBoth())
               yes?.Invoke();
       }

       private bool FortyBoth() => Player1Score == Forty && Player2Score == Forty;

       private bool ThirtyBoth() => Player1Score == Thirty && Player2Score == Thirty;
   }
public sealed class GameWonInStandardMode : IGameWon
   {
       public GameWonInStandardMode(IScoreContext context) => _context = context;

       private int Score1 => _context.Player1Score;
       private int Score2 => _context.Player2Score;

       private readonly IScoreContext _context;

       private const int ScoreDiffToWin = 2;
       private const int Forty = 4;

       public void Yes(Action<string> yes)
       {
           var playerDiff = Score1 - Score2;
           if (OnePlayerReachedForty() && ScoreDiffIsMoreThanTwo(playerDiff))
           {
               yes?.Invoke(GetNameOfWinner(playerDiff));
           }
       }

       private bool ScoreDiffIsMoreThanTwo(int playerDiff) => Math.Abs(playerDiff) >= ScoreDiffToWin;

       private bool OnePlayerReachedForty() => Score1 == Forty || Score2 == Forty;

       private string GetNameOfWinner(int diff) => diff >= ScoreDiffToWin
           ? PlayerNames.Player1
           : PlayerNames.Player2;
   }
public sealed class GameWonInExtraTime : IGameWon
  {
      private readonly IScoreContext _context;
      private int Player1Score => _context.Player1Score;
      private int Player2Score => _context.Player2Score;

      public GameWonInExtraTime(IScoreContext context) => _context = context;

      public void Yes(Action<string> yes)
      {
          var diff = Player1Score - Player2Score;
          if (PlayerLeadsWithTwoPoints(diff))
          {
              yes?.Invoke(WinnerPlayerName(diff));
          }
      }

      private static string WinnerPlayerName(int diff)
          => diff > 0 ? PlayerNames.Player1 : PlayerNames.Player2;

      private static bool PlayerLeadsWithTwoPoints(int diff) => Math.Abs(diff) == 2;
  }

 

Was it worth to refactor to so many classes?

This was the question I was asking myself when I felt I had finished the kata. I moved from one initial class TennisGame1 to eleven classes. The logic is split up in my solution in the initial solution one file included all the logic. To answer this question I analyzed the classes in Visual Studio via the „Code Metrics“ option:

The Maintainability Index increase from 66 (TennisGame2) to an average of 90. The summed up cyclomatic complexity is between 16 and 40 in the refactored solution in contrast to 7 to 49 in different single-file examples (TennisGame1TennisGame3). You shouldn’t forget I’m comparing the sum against the complexity of single files.

In the end I’m happy I tried the kata and was also surprised about how focused and maintainable the classes in the object-oriented design are.

You can find the full code in this Github repository.

I hope that helps.

Kommentar hinterlassen

Deine E-Mail-Adresse wird nicht veröffentlicht. Erforderliche Felder sind mit * markiert.

*

code