The Large Class code smells refers to a class that has too many responsibilities. It’s doing too much. Ideally a class should only have one responsibility (Single Responsibility Principle).
Code Smell: Large Class
Definition: A class has too many responsibilities.
Solution:
- Identify all of the distinct responsibilities of the Large Class.
- For each responsibility, extract out a class:
- Create a new class.
- Move fields and methods.
- Encapsulate fields to hide implementation from the Large Class.
Large Class code smell example
Here’s an example of the Large Class code smell (source: Trivia refactoring kata).
using System;
using System.Collections.Generic;
using System.Linq;
namespace Trivia
{
public class Game
{
private readonly List<string> _players = new List<string>();
private readonly int[] _places = new int[6];
private readonly int[] _purses = new int[6];
private readonly bool[] _inPenaltyBox = new bool[6];
private readonly LinkedList<string> _popQuestions = new LinkedList<string>();
private readonly LinkedList<string> _scienceQuestions = new LinkedList<string>();
private readonly LinkedList<string> _sportsQuestions = new LinkedList<string>();
private readonly LinkedList<string> _rockQuestions = new LinkedList<string>();
private int _currentPlayer;
private bool _isGettingOutOfPenaltyBox;
public Game()
{
for (var i = 0; i < 50; i++)
{
_popQuestions.AddLast("Pop Question " + i);
_scienceQuestions.AddLast(("Science Question " + i));
_sportsQuestions.AddLast(("Sports Question " + i));
_rockQuestions.AddLast(CreateRockQuestion(i));
}
}
public string CreateRockQuestion(int index)
{
return "Rock Question " + index;
}
public bool IsPlayable()
{
return (HowManyPlayers() >= 2);
}
public bool Add(string playerName)
{
_players.Add(playerName);
_places[HowManyPlayers()] = 0;
_purses[HowManyPlayers()] = 0;
_inPenaltyBox[HowManyPlayers()] = false;
Console.WriteLine(playerName + " was added");
Console.WriteLine("They are player number " + _players.Count);
return true;
}
public int HowManyPlayers()
{
return _players.Count;
}
public void Roll(int roll)
{
Console.WriteLine(_players[_currentPlayer] + " is the current player");
Console.WriteLine("They have rolled a " + roll);
if (_inPenaltyBox[_currentPlayer])
{
if (roll % 2 != 0)
{
_isGettingOutOfPenaltyBox = true;
Console.WriteLine(_players[_currentPlayer] + " is getting out of the penalty box");
_places[_currentPlayer] = _places[_currentPlayer] + roll;
if (_places[_currentPlayer] > 11) _places[_currentPlayer] = _places[_currentPlayer] - 12;
Console.WriteLine(_players[_currentPlayer]
+ "'s new location is "
+ _places[_currentPlayer]);
Console.WriteLine("The category is " + CurrentCategory());
AskQuestion();
}
else
{
Console.WriteLine(_players[_currentPlayer] + " is not getting out of the penalty box");
_isGettingOutOfPenaltyBox = false;
}
}
else
{
_places[_currentPlayer] = _places[_currentPlayer] + roll;
if (_places[_currentPlayer] > 11) _places[_currentPlayer] = _places[_currentPlayer] - 12;
Console.WriteLine(_players[_currentPlayer]
+ "'s new location is "
+ _places[_currentPlayer]);
Console.WriteLine("The category is " + CurrentCategory());
AskQuestion();
}
}
private void AskQuestion()
{
if (CurrentCategory() == "Pop")
{
Console.WriteLine(_popQuestions.First());
_popQuestions.RemoveFirst();
}
if (CurrentCategory() == "Science")
{
Console.WriteLine(_scienceQuestions.First());
_scienceQuestions.RemoveFirst();
}
if (CurrentCategory() == "Sports")
{
Console.WriteLine(_sportsQuestions.First());
_sportsQuestions.RemoveFirst();
}
if (CurrentCategory() == "Rock")
{
Console.WriteLine(_rockQuestions.First());
_rockQuestions.RemoveFirst();
}
}
private string CurrentCategory()
{
if (_places[_currentPlayer] == 0) return "Pop";
if (_places[_currentPlayer] == 4) return "Pop";
if (_places[_currentPlayer] == 8) return "Pop";
if (_places[_currentPlayer] == 1) return "Science";
if (_places[_currentPlayer] == 5) return "Science";
if (_places[_currentPlayer] == 9) return "Science";
if (_places[_currentPlayer] == 2) return "Sports";
if (_places[_currentPlayer] == 6) return "Sports";
if (_places[_currentPlayer] == 10) return "Sports";
return "Rock";
}
public bool WasCorrectlyAnswered()
{
if (_inPenaltyBox[_currentPlayer])
{
if (_isGettingOutOfPenaltyBox)
{
Console.WriteLine("Answer was correct!!!!");
_purses[_currentPlayer]++;
Console.WriteLine(_players[_currentPlayer]
+ " now has "
+ _purses[_currentPlayer]
+ " Gold Coins.");
var winner = DidPlayerWin();
_currentPlayer++;
if (_currentPlayer == _players.Count) _currentPlayer = 0;
return winner;
}
else
{
_currentPlayer++;
if (_currentPlayer == _players.Count) _currentPlayer = 0;
return true;
}
}
else
{
Console.WriteLine("Answer was corrent!!!!");
_purses[_currentPlayer]++;
Console.WriteLine(_players[_currentPlayer]
+ " now has "
+ _purses[_currentPlayer]
+ " Gold Coins.");
var winner = DidPlayerWin();
_currentPlayer++;
if (_currentPlayer == _players.Count) _currentPlayer = 0;
return winner;
}
}
public bool WrongAnswer()
{
Console.WriteLine("Question was incorrectly answered");
Console.WriteLine(_players[_currentPlayer] + " was sent to the penalty box");
_inPenaltyBox[_currentPlayer] = true;
_currentPlayer++;
if (_currentPlayer == _players.Count) _currentPlayer = 0;
return true;
}
private bool DidPlayerWin()
{
return !(_purses[_currentPlayer] == 6);
}
}
}
Code language: C# (cs)Before we begin
Refactoring rule #1: Always make sure you have tests covering the code you’re about to refactor. Run the tests after each small step.
The Trivia refactoring kata doesn’t have unit tests. So before I start refactoring, I’ll create a “golden master.” This means comparing the output before and after making changes. The output should remain the same after refactoring.
Identify all of the responsibilities
We can tell the Game class is suffering from the Large Class code smell because it has too many responsibilities. To identify the responsibilities, we need to do two things:
- Look at a class diagram.
- Read through the code.
The class diagram gives us a high-level overview of the class.
The class diagram is not enough to figure out all of the responsibilities. It’s a good start, but it’s not enough. If we’re lucky, the class will have good method names that tell us exactly what the code is doing. Unfortunately that’s almost never the case. For example, take a look at the Add() method. You can’t tell what this method is doing by looking at the class diagram. We have go look at the code to really know what it’s doing.
After walking through the code, and using the class diagram as a guide, we now have a list of the Game class’s responsibilities:
- Handling game logic.
- Generating and managing trivia questions.
- Managing players.
The Game class should really only have one responsibility: handling game logic. Therefore we’ll refactor this by extracting out classes for the other responsibilities.
Extract Questions class
The first responsibility we want to remove from the Game class is generating and managing trivia questions. We’ll extract this responsibility out to a new class called Questions.
Let’s take a look at the Game class and try to find all the code that has to do with questions.
We’ll use the Extract Class refactoring to move code over to the new Questions class. The Game class will be changed to use the Questions class.
It may seem like a good idea to move everything over to the new class all in one big step. But it’s better to do it in small steps. This way we can run the tests and verify we didn’t break anything.
1. Create the Questions class
namespace Trivia
{
public class Questions
{
}
}
Code language: C# (cs)2. Add a Questions property in the Game class
public class Game
{
private readonly Questions questions = new Questions();
...
Code language: C# (cs)3. Move Field _popQuestions
- Cut the _popQuestions field from the Game class.
- Paste it into the Questions class.
- Change the accessor to public.
public class Questions
{
public readonly LinkedList<string> _popQuestions = new LinkedList<string>();
}
Code language: C# (cs)- Update the Game class to use questions._popQuestions.
Here is an example of one of the places we have to update:
public Game()
{
for (var i = 0; i < 50; i++)
{
questions._popQuestions.AddLast("Pop Question " + i);
Code language: C# (cs)Note: Because we cut the _popQuestions out of the Game class, the compiler will report errors showing the exact locations of the code we need to update. This makes our job much easier. This is referred to as leaning on the compiler.
4. Move Fields _scienceQuestions, _sportsQuestions, _rockQuestions
Apply the same Move Field refactoring on the remaining question fields. After moving them, the Questions class should look like this:
public class Questions
{
public readonly LinkedList<string> _popQuestions = new LinkedList<string>();
public readonly LinkedList<string> _scienceQuestions = new LinkedList<string>();
public readonly LinkedList<string> _sportsQuestions = new LinkedList<string>();
public readonly LinkedList<string> _rockQuestions = new LinkedList<string>();
}
Code language: C# (cs)And the Game class should now be referring to these fields.
5. Inline Method CreateRockQuestion()
It might seem like we need to move CreateRockQuestion() to the Questions class, but let’s take a look at what it’s doing:
public Game()
{
for (var i = 0; i < 50; i++)
{
questions._popQuestions.AddLast("Pop Question " + i);
questions._scienceQuestions.AddLast(("Science Question " + i));
questions._sportsQuestions.AddLast(("Sports Question " + i));
questions._rockQuestions.AddLast(CreateRockQuestion(i));
}
}
public string CreateRockQuestion(int index)
{
return "Rock Question " + index;
}
Code language: C# (cs)Notice that the method serves no purpose. It’s creating Rock questions exactly like how the other questions are being created, so why bother moving this method to the new class? Instead of moving it, we can inline it.
- Copy the code from CreateRockQuestion().
- Replace the call to CreateRockQuestions() by pasting in the code.
- Delete CreateRockQuestion().
public Game()
{
for (var i = 0; i < 50; i++)
{
questions._popQuestions.AddLast("Pop Question " + i);
questions._scienceQuestions.AddLast(("Science Question " + i));
questions._sportsQuestions.AddLast(("Sports Question " + i));
questions._rockQuestions.AddLast("Rock Question " + i);
}
}
Code language: C# (cs)6. Apply the Move Method refactoring on the question generation logic in the constructor
You might be asking yourself, “how can we move a method if it doesn’t exist?” It’s generating questions in the Game constructor. This logic should really be in a separate method called GenerateQuestions(). However, since we are interested in moving this logic to the Questions class, we are really performing a Move Method refactoring.
- Create new method GenerateQuestions() in the Questions class.
- Cut and paste the question generation logic from the Game class constructor into Questions.GenerateQuestions().
- Remove the “questions.” qualifier.
public void GenerateQuestions()
{
for (var i = 0; i < 50; i++)
{
_popQuestions.AddLast("Pop Question " + i);
_scienceQuestions.AddLast(("Science Question " + i));
_sportsQuestions.AddLast(("Sports Question " + i));
_rockQuestions.AddLast("Rock Question " + i);
}
}
Code language: C# (cs)- Call Questions.GenerateQuestions() from the Game constructor.
public Game()
{
questions.GenerateQuestions();
}
Code language: C# (cs)7. Encapsulate Fields _popQuestions, _scienceQuestions, _sportsQuestions, and _rockQuestions
AskQuestion() is getting the next question based on the current category, removing it from the list of questions, then showing the question. Instead, it should be asking the Questions class for the next question, and then displaying it. We’ll refactor this by encapsulating the question fields and providing a new method called GetNextQuestion().
- Create a new method in Questions called GetNextQuestion().
public string GetNextQuestion(string Category)
{
return null;
}
Code language: C# (cs)- Copy and paste (don’t cut and paste this time) Game.AskQuestions() into Questions.GetNextQuestions().
public string GetNextQuestion(string Category)
{
if (CurrentCategory() == "Pop")
{
Console.WriteLine(questions._popQuestions.First());
questions._popQuestions.RemoveFirst();
}
if (CurrentCategory() == "Science")
{
Console.WriteLine(questions._scienceQuestions.First());
questions._scienceQuestions.RemoveFirst();
}
if (CurrentCategory() == "Sports")
{
Console.WriteLine(questions._sportsQuestions.First());
questions._sportsQuestions.RemoveFirst();
}
if (CurrentCategory() == "Rock")
{
Console.WriteLine(questions._rockQuestions.First());
questions._rockQuestions.RemoveFirst();
}
}
Code language: C# (cs)- Fix the “Pop” questions code:
- Replace CurrentCategory() with Category.
- Remove the “questions.” qualifier.
- Save the result of _popQuestions.First().
- Remove Console.WriteLine().
if (Category == "Pop")
{
string question = _popQuestions.First();
_popQuestions.RemoveFirst();
return question;
}
Code language: C# (cs)- Apply the same fix to “Science”, “Sports”, and “Rock”. In the end, we’ll have the following:
public string GetNextQuestion(string Category)
{
if (Category == "Pop")
{
string question = _popQuestions.First();
_popQuestions.RemoveFirst();
return question;
}
if (Category == "Science")
{
string question =_scienceQuestions.First();
_scienceQuestions.RemoveFirst();
return question;
}
if (Category == "Sports")
{
string question = _sportsQuestions.First();
_sportsQuestions.RemoveFirst();
return question;
}
if (Category == "Rock")
{
string question = _rockQuestions.First();
_rockQuestions.RemoveFirst();
return question;
}
return null;
}
Code language: C# (cs)Note: The Questions class is full of code smells and should be refactored, but right now we are focusing on refactoring the Large Class code smell in the Game class. When we’re refactoring, we need to put on blinders and focus on the bigger picture. If we were to go off on tangents, we’d never finish the initial refactoring and create an even bigger mess for ourselves.
- In Game.AskQuestion(), replace the question getting logic with a call to Questions.GetNextQuestion().
private void AskQuestion()
{
var question = questions.GetNextQuestion(CurrentCategory());
Console.WriteLine(question);
}
Code language: C# (cs)- Change the access modifier on the question fields to private.
public class Questions
{
private readonly LinkedList<string> _popQuestions = new LinkedList<string>();
private readonly LinkedList<string> _scienceQuestions = new LinkedList<string>();
private readonly LinkedList<string> _sportsQuestions = new LinkedList<string>();
private readonly LinkedList<string> _rockQuestions = new LinkedList<string>();
Code language: C# (cs)We’ve completed extracting out the Questions class. We’ll move on to extracting the next responsibility – managing players.
Extract Players class
The Game class is currently managing players. We’ll extract this responsibility into a new class called Players.
Let’s take a look at the Game class diagram to help us figure out what we need to extract out to the Players class.
Nearly all of the methods deal with players.
We’ll use the Extract Class refactoring to create a new class called Players, move fields and logic, and update the Game class to use the Players class.
1. Create Players class
namespace Trivia
{
public class Players
{
}
}
Code language: C# (cs)2. Constructor inject the Players object
- Add a Players property.
- Add a Players parameter to the constructor, and initialize the Players property.
private readonly Players players;
public Game(Players players)
{
this.players = players;
questions.GenerateQuestions();
}
Code language: C# (cs)- We just broke the existing calls to the Game constructor. We’ll need to update them to pass in the new parameter. Use the compiler errors to find all the places to update.
public static void Main(string[] args)
{
var aGame = new Game(new Players());;
Code language: C# (cs)3. Move Field “_players” into Players class as _playerNames
The _players field is a list of player names. So we’ll move this into the Players class with the appropriate name of _playerNames.
- Cut and paste _players into the Players class.
- Change the name to _playerNames.
public class Players
{
public readonly List<string> _playerNames = new List<string>();
}
Code language: C# (cs)- Update the Game class to use players._playerNames instead of _players. Use the compiler errors to help find all of the places to fix.
Example:
public bool Add(string playerName)
{
players._playerNames.Add(playerName);
_places[HowManyPlayers()] = 0;
_purses[HowManyPlayers()] = 0;
_inPenaltyBox[HowManyPlayers()] = false;
Console.WriteLine(playerName + " was added");
Console.WriteLine("They are player number " + players._playerNames.Count);
return true;
}
Code language: C# (cs)4. Move Fields _places, _purses, and _inPenaltyBox
Apply the Move Field refactoring to the remaining player fields. After this the Players class should look like this:
public class Players
{
public readonly List<string> _playerNames = new List<string>();
public readonly int[] _places = new int[6];
public readonly int[] _purses = new int[6];
public readonly bool[] _inPenaltyBox = new bool[6];
}
Code language: C# (cs)The Game class should now be using these fields in the Players class.
5. Move Method HowManyPlayers()
- Cut and paste HowManyPlayers() from the Game class to the Players class.
public int HowManyPlayers()
{
return _playerNames.Count;
}
Code language: C# (cs)- Update the Game class to use players.HowManyPlayers(). Use the compiler errors to find all of the places to update.
6. Move Method Add()
- Cut and paste Add() from the Game class to the Players class.
Note: The name Add() makes sense contextually in the Players class, so there’s no need to rename this method. If we were to keep this in the Game class, it would need to be renamed AddPlayer().
- Remove “players.” qualifier. Now it’ll look like this:
public bool Add(string playerName)
{
_playerNames.Add(playerName);
_places[HowManyPlayers()] = 0;
_purses[HowManyPlayers()] = 0;
_inPenaltyBox[HowManyPlayers()] = false;
Console.WriteLine(playerName + " was added");
Console.WriteLine("They are player number " + _playerNames.Count);
return true;
}
Code language: C# (cs)- Update all references to Game.Add() to Players.Add(). Use the compiler errors to find all of the places to fix.
Example:
The GameRunner class was calling Game.Add(). Now it needs to call Players.Add().
public class GameRunner
{
private static bool _notAWinner;
public static void Main(string[] args)
{
var players = new Players();
players.Add("Chet");
players.Add("Pat");
players.Add("Sue");
var aGame = new Game(players);
Code language: C# (cs)7. Encapsulate Field players._places[]
Look at how the players._places[] field is being used in Games. The Game class is getting the value and adding to it. We need to encapsulate this by adding a getter method and a setter method that only allows the Game class to add to the places field.
- In the Players class add GetPlace() and AddToPlace().
public int GetPlace(int playerNumber)
{
return _places[playerNumber];
}
public void AddToPlace(int playerNumber, int addAmount)
{
_places[playerNumber] += addAmount;
}
Code language: C# (cs)- Change the access level on the _places to private.
private readonly int[] _places = new int[6];
Code language: C# (cs)- Update all references to players._places in the Game class to use the GetPlace() and AddToPlace() methods. Like usual, use the compiler errors to help find all the places to update.
Example of updating the Game class to use these new methods:
public void Roll(int roll)
{
Console.WriteLine(players._playerNames[_currentPlayer] + " is the current player");
Console.WriteLine("They have rolled a " + roll);
if (players._inPenaltyBox[_currentPlayer])
{
if (roll % 2 != 0)
{
_isGettingOutOfPenaltyBox = true;
Console.WriteLine(players._playerNames[_currentPlayer] + " is getting out of the penalty box");
players.AddToPlace(_currentPlayer, roll);
if (players.GetPlace(_currentPlayer) > 11) players.AddToPlace(_currentPlayer, -12);
Code language: C# (cs)8. Encapsulate Fields _playerNames[], _purses[], and _inPenaltyBox[]
Apply the Encapsulate Field refactoring to the remaining fields in Player. Remember, we need to look at how these fields are being used in the Game class. The table below shows the Getter/Setter methods we need to create to encapsulate the fields.
Note: If a field is not getting updated by the Game class, there is no need to add a Setter.
| Field | Getter | Setter |
| _playerNames[] | string GetPlayerName(int playerNumber) int HowManyPlayers() – already exists, need to update the Game class to use this instead of _playerNames.Count. | |
| _purses[] | int GetPurse(int playerNumber) | void IncrementPurse(int playerNumber) |
| _inPenaltyBox[] | bool IsInPenaltyBox(int playerNumber) | void PutInPenaltyBox(int playerNumber) |
After this step, all fields in the Player class should be private, and the Game class should be using the getters/setters.
End results
We’ve successfully eliminated the Large Class code smell by extracting out the Players and Questions classes from the Game class. Now the Game class has a single responsibility: handle the game logic.
Here is what the final class diagram look with these two classes extracted:
There are still plenty of code smells in this code, but we’ve accomplished our main goal of dealing with the Large Class code smell.
