Refactoring, Round 1 -- Fight!

One of my goals for this project was not just to make a game, but also--to the best of my ability--make a game well. I knew from the very beginning that I wanted to utilize continuous integration and automated build/unit testing, and that the architecture of the project and systems would be a large factor in the success of those efforts. As mentioned in my previous post, code quality was sacrificed in the final push to complete my first gameplay prototype. Well, since then I've been working solely on refactoring. The three main issues I identified were: my over use of the singleton pattern, unmanaged references to database assets, and monolithic classes.

Let's look at the monolith in the room: GameData. At the time of the v0.1.0 gameplay prototype, this class handled everything. Player values such as health and combo counter, UI state management, combat evaluation, and combat history--it did it all. Plus, it implemented a singleton pattern and a static accessor, which allowed all other objects in the scene to find it--great for prototyping, but not great for managing dependencies or unit test setup.

To refactor this class, I first grouped all of the methods and fields into regions according to what specific responsibility they were fulfilling. Then, one by one, I began extracting those regions in to their own classes. The player values like health became PlayerData. The state management for usable moves became the UsableMoveSet (now attached as a component to the character prefab).

public class UsableMoveSet : NetworkBehaviour
{
    private NetworkVariable<byte> _moves = new();
    public NetworkVariable<byte> Moves { get => _moves; }

    public override void OnNetworkSpawn()
    {
        var moveSet = GetComponent<PlayerCharacter>().Character.CharacterMoveSet;
        InitializeMoveSet(moveSet); 
    }

    public void InitializeMoveSet(CharacterMoveSet moveSet)
    {
        foreach (Move.Type type in Enum.GetValues(typeof(Move.Type)))
        {
            byte isUsable = (byte)(moveSet.GetMoveByType(type).UsableByDefault ? 1 : 0);
            byte typeAsByte = (byte)type;
            _moves.Value |= (byte)(typeAsByte * isUsable);
        }
    }

    public void DisableMoveByType(Move.Type type)
    {
        byte mask = (byte) (Byte.MaxValue - (byte)type);
        _moves.Value &= mask;
    }

    public void EnableMoveByType(Move.Type type)
    {
        byte mask = (byte) type;
        _moves.Value |= mask;
    }

    public bool CheckEnabledByType(Move.Type type)
    {
        byte mask = (byte) type;
        return (_moves.Value & mask) == mask;
    }
}

I was concerned that as I extracted more and more responsibilities that I would have to untangle the dependencies within this class and within other classes, but the opposite happened. The depencies actually became clearer and less tangled. Plus, it exposed opportunities to improve my network communication. For example, by making the PlayerData class a network serializable struct, now I could send all value updates after combat in a single packet, rather than having the individual NetworkVariable values each send a message upon each change.

Without the GameData.Instance singleton, how can other systems, like the UI or the state machine, access the player data, then? Answer: dependency injection!

The TurnHistory class that spawned from the dismantling of GameData maintains a collection of TurnData structs, each representing the state at the end of a turn of combat. The GameUIManager class needs to subscribe to changes to the TurnHistory class's NetworkList<TurnData>, in order to update the state of UI elements, like player health bars, after each turn. The way it found this data before was by calling GameData.Instance. Now, the GameUIManager has a field for the TurnHistory object, and this field gets injected by an installer upon initialization of the scene. The GameUIManager no longer cares about how it gets access to the turn history, and it surrenders that responsibility to the DI framework, Zenject. Furthermore, now the dependencies of each class are clearly identified, and I no longer need to do a "Find all references" search for GameData.Instance in Visual Studio when making a small change.

GameUIManager Class

public class GameUIManager : NetworkBehaviour
{
    private TurnHistory _turnHistory;

    [Inject]
    public void Construct(TurnHistory turnHistory)
    {
        _turnHistory = turnHistory;
    }

    public override void OnNetworkSpawn()
    {
        _turnHistory.TurnDataList.OnListChanged += HandleTurnData;
    }

    public override void OnNetworkDespawn()
    {
        _turnHistory.TurnDataList.OnListChanged -= HandleTurnData;
    }

    private void HandleTurnData(NetworkListEvent<TurnData> changeEvent)
    {
        var turnData = changeEvent.Value;
        // Update UI
    }
}

Installer that binds TurnHistory

public class GameplayInstaller : MonoInstaller
{
    public override void InstallBindings()
    {
        Container.Bind<TurnHistory>()
            .FromComponentInHierarchy()
            .AsSingle();
    }
}

The installer of the binding for TurnHistory is contained within the context of the gameplay scene. There are some dependencies, though, that need to be injected across multiple scenes or the entire project. like the CharacterDatabase and MoveDatabase assets. These ScriptableObject databases expose a simple interface that allows for access to Character and Move assets using an integer ID value. This means that the size of communication of characters and moves over the network can be minimized to just that integer value. Granting objects access to these databases meant setting up a public or a serializable private field, and dragging in the reference in the Unity editor.

If I wanted to create separate database files for production and development, maybe to experiment with new moves or a new character, that would mean I would need to drag-and-drop the new database files in every single object that referenced them--not great.

A better solution is to inject them. So, I created a Database provider class that just holds references to the two database assets. Then, I set up an installer in the project scope to install the bindings to the provider class wherever it is needed. Now if I want to swap out the databases, I just need to update them in one place: the provider class.

Database provider

public class Database : MonoBehaviour
{
    [SerializeField] private CharacterDatabase _characterDatabase;
    [SerializeField] private MoveDatabase _moveDatabase;

    public CharacterDatabase Characters { get => _characterDatabase; }
    public MoveDatabase Moves { get => _moveDatabase; }
}

Installer that binds Database

public class GlobalInstaller : MonoInstaller
{
    public override void InstallBindings()
    {
        Container.Bind<Database>()
            .FromComponentInNewPrefabResource("Database")
            .AsSingle()
            .NonLazy();
    }
}

Using the move database in the CombatEvaluator

public class CombatEvaluator
{
    private readonly Database _database;
    private readonly PlayerDataCollection _players;

    [Inject]
    public CombatEvaluator(Database database, PlayerDataCollection players)
    {
        _database = database;
        _players = players;
    }

    public void DoStuff()
    {
        var player1 = _players.GetByPlayerNumber(1);
        var player1ActionId = player1.PlayerData.Action;
        var player1Move = _database.Moves.GetMoveById(player1ActionId);
        // do stuff
    }
}

The changes made to facilitate dependency injection go hand in hand with improving testability. For example, the setter for the Action property in PlayerCharacter not only updates the PlayerData state, but also invokes a ClientRpc to a specific client in order to update their UI to reflect their move selection. Previously, this ClientRpc was invoked by calling GameUIManager.Instance.SomeMethodClientRpc(). While I could mock a GameUIManager object, mock it's property Instance to return itself, and then mock the method, it's much easier to inject the mock directly into the class being tested.

The method being tested

public int Action
{
    get
    {
        return _playerData.Action;
    }
    set
    {
        var previous = _playerData.Action;
        _playerData = new PlayerData
        {
            Health = _playerData.Health,
            SpecialMeter = _playerData.SpecialMeter,
            Action = value,
            ComboCount = _playerData.ComboCount
        };
        var clientRpcParams = new ClientRpcParams
        {
            Send = new ClientRpcSendParams
            {
                TargetClientIds = new[] { _clientId },
            }
        };
        _gameUIManager.UpdateActiveSelectionButtonClientRpc(previous, value, clientRpcParams);
    }
}

Unit test

[Test]
[TestCase(-1, -1)]  // 1
[TestCase(1, 1)]
[TestCase(2, 2)]
public void SetActionCorrectlyAndSendsRpc(int value, int expected)
{
    var mock = new Mock<IGameUIManager>();
    mock.Setup(
        m => m.UpdateActiveSelectionButtonClientRpc(
            It.IsAny<int>(),
            It.Is<int>(v => v == value),  // 2
            It.Is<ClientRpcParams>(clientRpcParam => 
                HasCorrectTargetClientId(
                    clientRpcParam,
                    _playerCharacter.ClientId)))) // 3
        .Verifiable();
    _playerCharacter.GameUIManager = mock.Object;
    _playerCharacter.Action = value;
    Assert.AreEqual(expected, _playerCharacter.Action);  // 4
    mock.Verify(
        m => m.UpdateActiveSelectionButtonClientRpc(
            It.IsAny<int>(),
            It.IsAny<int>(),
            It.IsAny<ClientRpcParams>()),
        Times.Once());  // 5
}

This test looks like a lot, but it's actually quite simple. First, with the Unity test framework being built upon NUnit, we gain access to features such as paramaterized test cases (1). In this way, we can test multiple sets of inputs and expected values with a single test method.

Using the Moq library, we can set up argument validation within the method setup. In this case, we are checking that the method UpdateActiveSelectionButtonClientRpc() is being invoked with the correct action Id (2), and the correct target clientId (3). This setup is handled within the test, rather than in a SetUp method because of the need to access the value being tested.

Besides a typical assert that the input value and the expected value are equal (4), we can also verify that the mocked method was invoked a specific number of times--in this case, once (5).

---

That's all for now! There's still room for improvement, of course, and this won't be my only round of refactoring. As I continue development, I'll be looking for opportunities to dive deeper into the Zenject framework and utilize its features to promote clean, quality code (...like using signals for audio/animation control? Could be good!). Once I finish writing test suites for the core systems, I'll be looking to iterate on gameplay design based on feedback from the prototype testing, and then it's on to the look-and-feel first pass.

In the meantime, check out the repo and project to follow my progress.