Memoria icon indicating copy to clipboard operation
Memoria copied to clipboard

Refactoring btl_cmd for Decoupling and Testability

Open Albeoris opened this issue 7 months ago • 5 comments

Refactoring btl_cmd for Decoupling and Testability

Goal: Refactor the btl_cmd class (from the Memoria project’s Assembly-CSharp.dll) to isolate its logic into a separate module. This will improve code readability, enable easier unit testing, and allow reuse of the battle command logic without a hard dependency on the entire Assembly-CSharp. We will achieve this by moving btl_cmd and related code into a new project, using dependency inversion (introducing interfaces for external interactions), preserving Unity’s serialization requirements, writing comprehensive xUnit tests, and documenting the public API with XML comments.

1. Extract btl_cmd into a Separate Class Library Project

Objective: Create a new C# class library (DLL) for the btl_cmd logic and move the class (and minimal necessary code) there, instead of residing in the monolithic Assembly-CSharp. Separating core gameplay logic into its own assembly follows best practices for separation of concerns, making the business logic independent, reusable, and easier to test.

Steps:

  • Create a New .csproj: In the Memoria solution, add a new Class Library project (e.g. Memoria.BattleCommands). Target .NET Framework 3.5 (to match Unity’s old runtime on Mono). Ensure the new project will produce a DLL that Unity can load (if Unity is used at runtime). For example, set <TargetFramework>net35</TargetFramework> in the .csproj.

  • Copy btl_cmd Code: Add the btl_cmd.cs file to the new project. Include any direct dependencies that are purely data or enum types (for instance, command enums or simple data structs) if required. Do not reference the entire Assembly-CSharp.dll – the goal is to only bring over the code strictly needed for btl_cmd’s logic.

  • Adjust Namespaces (if needed): Place the btl_cmd class in an appropriate namespace (e.g. Memoria.Battle or similar) in the new project. Update references within btl_cmd.cs to use any new namespace for moved types. This new assembly should ideally have no dependency on UnityEngine or Assembly-CSharp – it will be a pure logic module.

  • Retarget Unity API usage: If btl_cmd code calls any Unity API or game-specific singletons, do not copy those; instead, plan to abstract those calls via interfaces (detailed in the next section). The new project should contain logic only, with external hooks abstracted.

  • Build and Verify: Ensure the new project builds independently. You may need to add references to system libraries that Unity’s old runtime uses (e.g. System.Core for LINQ, etc., compatible with .NET 3.5). At this stage, expect build errors due to missing references to game-specific classes – we will resolve those by introducing interfaces in step 2, rather than by adding references to the old assembly.

2. Identify External Dependencies and Introduce Interfaces (Dependency Inversion)

Objective: Decouple btl_cmd from game-specific or engine-specific details by using the Dependency Inversion Principle. High-level logic (battle commands) should not depend on low-level modules or global state; both should depend on abstractions. We will replace direct references to external classes with interfaces defined in the new project, and inject implementations from the original Assembly-CSharp at runtime.

Steps:

  • Scan btl_cmd for Dependencies: Review the btl_cmd code to find anything it uses that is not purely internal logic. This includes:

    • Global singletons or managers (e.g. game state, configuration flags, event triggers). For example, if the code calls FF9StateSystem.Settings.IsTranceFull directly inside btl_cmd.FinishCommand, that is a global game state dependency.
    • Other classes’ static methods or data (e.g. btl_stat, btl_calc, Status, BattleHUD, etc. from Assembly-CSharp).
    • UnityEngine or engine APIs (if any, like Debug.Log, or MonoBehaviour methods).
    • Data structures from Assembly-CSharp that are not meant to be moved entirely (e.g. if btl_cmd uses a BTL_DATA class representing characters, or CMD_DATA structures, you must decide whether to duplicate these or abstract them).
  • Define Interfaces in the New Project: For each category of dependency, create an interface in the new Memoria.BattleCommands project that represents the minimal contract btl_cmd needs. Some examples:

    • If btl_cmd accesses game settings (like IsTranceFull flag), define an interface IGameSettings with the necessary property (e.g. bool IsTranceFull { get; }).
    • If btl_cmd calls functions in a battle status class (e.g. btl_stat.AlterStatus), create an interface IBattleStatusService with methods like AlterStatus(...), RemoveStatus(...), etc., matching what btl_cmd needs to do.
    • If btl_cmd uses a battle calculation module (btl_calc.CalcMain), define an interface IBattleCalculator for the needed method.
    • For any other global or manager (e.g. FF9StateSystem for state, or a sound or UI trigger), make a corresponding interface.

    Each interface should reside in the new project and be designed in a game-agnostic way, focusing on the functionality rather than the specific class names. For instance, IGameSettings doesn’t need to know about FF9StateSystem – it just exposes the settings values needed.

  • Refactor btl_cmd to Use Interfaces: Replace references in btl_cmd code to concrete classes with calls to these interfaces. There are two patterns to do this:

    • Dependency Injection: Change btl_cmd from a purely static utility into an instance-based service that has interface references injected (e.g. via constructor). For example, give btl_cmd (or a new BattleCommandService class) a constructor like:

      public class BattleCommandService
      {
          private readonly IGameSettings _gameSettings;
          private readonly IBattleStatusService _statusService;
          ... // other dependencies
      
          public BattleCommandService(IGameSettings gameSettings, IBattleStatusService statusService, ...)
          {
              _gameSettings = gameSettings;
              _statusService = statusService;
              ...
          }
      
          public void FinishCommand(CommandData cmd) 
          {
              if (_gameSettings.IsTranceFull)
              {
                  // ... use _statusService instead of direct btl_stat calls
              }
          }
      }
      

      Using this approach, methods like FinishCommand would use _gameSettings instead of directly calling the game’s static state. This yields a clean separation: the BattleCommandService doesn’t know about FF9StateSystem or btl_stat – it only knows about the abstractions.

    • Static Service Locator (temporary): If converting to an instance-based design is too intrusive for now, you could implement a simpler approach where the new module has static properties or a singleton to hold interfaces. For example, a static class BattleCommandService with static fields GameSettings (type IGameSettings) etc., that must be initialized by the original code on startup. Then btl_cmd static methods can refer to these. This is less ideal from a design standpoint but can ease integration while still decoupling the code logic from concrete implementations. (Use this only if constructor injection proves impractical due to static usage in many places.)

  • Implement Interfaces in Original Project: In the Unity/Assembly-CSharp project, create concrete implementations that fulfill the contracts:

    • For IGameSettings, the original code’s FF9StateSystem.Settings (or wherever the flags are stored) should implement this interface. For example, create a class GameSettingsFacade : IGameSettings that wraps FF9StateSystem.Settings or copies needed properties.
    • For IBattleStatusService, implement it by calling the original btl_stat.AlterStatus, RemoveStatus, etc. (These original static methods can either be invoked directly or you refactor btl_stat similarly—however, that might be beyond scope. A simple wrapper is fine).
    • Repeat for other interfaces (e.g., BattleCalculatorFacade : IBattleCalculator calling btl_calc.CalcMain, etc.).

    These implementations will live in Assembly-CSharp (or a bridging project that has access to it) because they need to call the real game code. They act as adapters between the new battle command module and the existing game code.

  • Inject Dependencies at Runtime: In the game initialization (or wherever appropriate, e.g. a startup method or a static initializer), instantiate the BattleCommandService (or set up the static service) with the real implementations. For example:

    // In some initialization code in Assembly-CSharp:
    var battleCmdService = new BattleCommandService(
        new GameSettingsFacade(FF9StateSystem.Settings), 
        new BattleStatusService(), 
        new BattleCalculatorService());
    // store this instance where others can access it, or pass it to systems that need it
    BattleCommandService.Instance = battleCmdService; // if using a singleton pattern
    

    Now the game’s other systems can obtain this service (via singleton or DI container) and call, say, battleCmdService.FinishCommand(cmd), which internally uses the injected interfaces rather than any direct global state.

  • Example – Removing a direct dependency: In original btl_cmd, suppose we had:

    if (FF9StateSystem.Settings.IsTranceFull)
        cmd.Data.Trance = 0;
    

    After refactoring, the new code might look like:

    if (_gameSettings.IsTranceFull)
        cmd.Data.Trance = 0;
    

    The IGameSettings implementation ensures that _gameSettings.IsTranceFull actually calls into the proper game setting. Thus, btl_cmd logic is now testable by substituting a mock IGameSettings. This pattern should be applied to all external references. The result is that btl_cmd (in the new assembly) depends only on abstractions that we control, not on Unity or FF9 internals.

  • Keep Logic Intact: During this refactoring, ensure the actual battle logic remains the same. We are only redirecting how it accesses data or other systems, not changing the game rules. Write unit tests (in the next step) to validate that behavior has not changed.

3. Preserve Unity’s Serialization (Don’t Break Serialized Fields)

Objective: Unity’s serialization is sensitive to class and field changes in MonoBehaviour or ScriptableObject classes. We must not change the order, names, or types of fields in any serializable class that is being moved or refactored, to avoid losing data in existing save files, scenes, or prefabs. This is especially crucial for UnityEngine.Object-derived classes (MonoBehaviours, ScriptableObjects) and [Serializable] structs/classes used in them.

Steps:

  • Avoid Moving MonoBehaviours Directly: If btl_cmd were a MonoBehaviour (though it likely is not), you would not move that class out of Assembly-CSharp, because Unity links scene components to script classes by name and assembly. In such cases, the strategy would be to leave a minimal MonoBehaviour in place and delegate logic to the new module. (For btl_cmd which appears to be a plain class, this is not an issue, but this advice applies to any other Unity components you might refactor.)

  • Maintain Field Layout: For any class or struct that is serializable (e.g. if CMD_DATA or similar structures are involved in saving game state or defined with [Serializable]), do not reorder or remove its fields. If you must move such a type to the new assembly, copy it exactly with the same field order, names, and default values. This ensures Unity (or any binary serialization) can still map the data correctly. For example: if BTL_DATA has fields HP, MP, Atk, keep them in that exact sequence in the new definition. Even though Unity’s YAML serialization primarily matches by field name, keeping order consistent is a safe practice to avoid any subtle issues or inspector differences.

  • Do Not Change Inheritance of Serialized Classes: If a class inherits from MonoBehaviour or ScriptableObject, do not change its base class. Our refactoring should not turn a serialized class into a plain class or vice versa, as that would break Unity references. Instead, if such a class contains logic we want to move, strip out the logic but leave the class itself (with its fields intact) in Assembly-CSharp. The MonoBehaviour can call into the new system via an interface without containing logic itself.

  • Avoid renaming serialized fields to minimize risk: Even if you absolutely must rename a field or property in a serialized class for clarity, DON'T DO THIS! Don't try to use FormerlySerializedAs!

  • Verify in Unity: After refactoring, test that existing game data loads correctly. For instance, if there’s a scene or prefab that included any of the moved classes, ensure no missing script or lost data. Since our goal is to leave only integration in Assembly-CSharp, double-check that any MonoBehaviour left behind is still attached and now calling the new logic properly.

By preserving the shape of data-holding classes, we ensure that the refactoring does not corrupt saves or require tedious data migration. Unity’s serialization system is forgiving about field order in many cases, but adhering strictly to the original layout is a good safeguard during this transition.

4. Rename Types and Members Where Possible

Improve readability by assigning new, clear, and descriptive names to types and their members (methods, properties, etc.) wherever possible.

Refactoring (deletion, addition, combination) of methods and properties is allowed if it enhances clarity and does not impact serialization requirements.

5. Create a Unit Test Project (xUnit) and Write Tests for btl_cmd

Objective: For each method in btl_cmd’s logic, create unit tests using xUnit to validate its behavior in isolation. Now that the logic is separated and depends on interfaces, we can easily inject fake dependencies to simulate different game conditions. This will greatly increase confidence that the refactoring didn’t introduce bugs, and will prevent regressions going forward.

Steps:

  • Set Up Test Project: Add a new xUnit Test Project to the solution (you can use the xUnit project template). Target at least .NET Framework 4.x for the test project (e.g. net48), since xUnit 2.x requires a newer framework than 3.5 to run the tests. The test project can still reference the 3.5-targeted Memoria.BattleCommands DLL – .NET 4.x will run it fine. Include the xUnit and xUnit Runner NuGet packages. (If using Visual Studio, ensure the test project references Memoria.BattleCommands.dll and xUnit packages, and that Test Explorer recognizes the tests.)

  • Write Test Cases for Each Method: Go through every public method (and any important internal methods) of the btl_cmd class (or the new BattleCommandService). For each, write one or more [Fact] methods in the test project to cover its behavior. Use descriptive test names (e.g. FinishCommand_ShouldNotDecreaseTrance_WhenTranceIsAlwaysFull) to clarify intent. For example:

    public class BattleCommandServiceTests
    {
        [Fact]
        public void FinishCommand_TranceRemainsFull_WhenIsTranceFullSettingEnabled()
        {
            // Arrange: set up a fake game settings where IsTranceFull = true
            var settings = new FakeGameSettings { IsTranceFull = true };
            var statusService = new FakeBattleStatusService();
            var service = new BattleCommandService(settings, statusService, ...);
            var command = CreateSampleCommand(); // create a dummy command object
    
            // Act: execute FinishCommand
            service.FinishCommand(command);
    
            // Assert: verify that trance was not reduced, etc.
            Assert.True(command.Data.Trance == 255);
        }
    }
    

    In the above example, FakeGameSettings and FakeBattleStatusService would be simple stub classes implementing IGameSettings/IBattleStatusService for test purposes. You can also use a mocking framework like Moq to create dummy implementations of interfaces if preferred (e.g. setup a mock IGameSettings to return true for IsTranceFull).

  • Cover Various Scenarios: Make sure to test normal cases and edge cases:

    • Different values of game settings (trance full vs not full, etc.).
    • Boundary conditions for commands (e.g. if a command deals 0 damage or max damage, if a command targets an invalid target, etc., depending on what the method does).
    • Sequence of method calls if relevant (for example, if InitCommand must be called before FinishCommand, have tests for that sequence).
    • Error conditions: if the method is supposed to handle null or invalid data gracefully, include tests for those.
  • Test Expected Outcomes: Use xUnit assertions (Assert.Equal, Assert.True, Assert.Throws, etc.) to verify that after calling a btl_cmd method, the outcomes match expectations. This could mean:

    • The returned value is correct (if the method returns something).
    • The state of a command or a character was correctly modified.
    • The interface methods were called with expected parameters (if using mocks, you can verify interactions, e.g. that IBattleStatusService.AlterStatus was invoked when it should be).
  • Automate and Iterate: Run the test suite after making the changes. All tests should pass. If any fail, that indicates a discrepancy between expected behavior and the refactored implementation – investigate and fix the logic or adjust the test if the logic intentionally changed (though ideally, logic should remain the same).

  • Maintain Tests for Future: Include the test project in your CI/build process if possible. The tests will guard against future modifications breaking the battle command behavior.

By using xUnit and dependency injection, we can now simulate different game states easily. For example, instead of relying on a real FF9StateSystem (which might not even exist outside the game), we pass in a fake that we control. This isolation is the direct benefit of the dependency inversion: the btl_cmd logic is now a pure C# logic unit that can run in a test runner, completely outside of Unity or the full game context.

6. Add XML Documentation to All Public Types and Members

Objective: Improve maintainability by documenting the purpose and usage of the btl_cmd module’s API. We will add XML documentation comments (/// <summary> ... </summary>) in the code for every public class, method, property, etc., following the MSDN style guidelines. This helps developers understand the code intent and generates useful IntelliSense tooltips or reference docs.

Steps:

  • Enable XML Documentation Output: In the new project’s settings, enable XML documentation file generation (so that the comments get compiled into an XML). In the .csproj, this is usually <GenerateDocumentationFile>true</GenerateDocumentationFile>.

  • Write <summary> for Each Public Member: For every public class, method, property, or field in the new Memoria.BattleCommands assembly, add a XML comment. Provide a concise one-sentence summary of what it does, then more detail if necessary:

    • Class example:

      /// <summary>
      /// Handles battle command initialization, execution, and completion logic.
      /// </summary>
      public class BattleCommandService { ... }
      

      If needed, you can add <remarks>...</remarks> for additional info about the class usage or implementation details.

    • Method example:

      /// <summary>
      /// Finalizes a battle command, applying its effects and cleaning up the command state.
      /// </summary>
      /// <param name="cmd">The command data to finish (typically contains caster, target, etc.).</param>
      /// <returns>Returns <c>true</c> if the command finished successfully, or <c>false</c> if it was invalid.</returns>
      public bool FinishCommand(CommandData cmd) { ... }
      

      Note: Use <c>...</c> to mark code/keywords, <paramref name="..."> if referencing a parameter in the description, and so on, to match MSDN style. Use third-person description (“Finishes the command…” or “Gets the value…” for properties).

    • Property example:

      /// <summary>Gets or sets the current command’s unique identifier.</summary>
      public int CommandId { get; set; }
      

      Keep property summaries brief (usually “Gets or sets…”).

    • Enum or constant example: document each value if non-obvious.

  • Follow MSDN Conventions: According to Microsoft’s recommendations, all publicly visible types and members should have documentation comments. Write in complete sentences, end with a period, and use the appropriate tags:

    • <summary> for a summary of functionality.
    • <param name="name"> to describe each method parameter (what it represents or how it’s used).
    • <returns> to describe what a method returns (or say “void” method doesn’t need a returns tag).
    • <exception> tags if a method can throw exceptions under certain conditions.
    • <remarks> for any additional notes or caveats.
    • Possibly <example> to give an example usage if the method is complex.

    Ensure the XML is well-formed (malformed XML comments will generate compiler warnings). Consistency and clarity are key – the documentation should enable someone new to the project to understand the role of btl_cmd module easily.

  • Example Documentation Snippet: Here’s a sample of how a documented class and method might look:

    /// <summary>
    /// Provides functionality to initialize battle commands for a character.
    /// </summary>
    /// <remarks>
    /// This class is responsible for setting up command slots and default commands for battle entities.
    /// </remarks>
    public class CommandInitializer
    {
        /// <summary>
        /// Initializes all command slots for a given battle entity.
        /// </summary>
        /// <param name="entity">The battle entity whose commands are being initialized.</param>
        /// <param name="isEnemy">If true, the entity is an enemy and will get a default set of commands.</param>
        /// <returns>Array of initialized command slots for the entity.</returns>
        public CmdData[] InitCommands(BTL_DATA entity, bool isEnemy)
        {
            // ... logic ...
        }
    }
    

    (The above is illustrative; use actual types and logic from your code.)

  • Proofread and Iterate: Once all public members have XML comments, build the project and ensure no warnings about missing XML comments remain (you can treat missing comments as warnings or errors in build settings to enforce this). Read through the generated documentation (the XML or via IntelliSense) to ensure it’s clear and free of typos.

Documenting the code not only helps others, but also often reveals unclear parts of the design. If you find a method hard to describe, that might indicate it needs refactoring or better naming. Aim for clarity and accuracy in the docs.

7. Integrate the Refactored Module and Clean Up the Original Code

Objective: Now that btl_cmd logic lives in the new assembly (with interfaces for external calls), we need to modify the original project to use this new module. This involves removing or reducing the old btl_cmd class in Assembly-CSharp and updating any references to use the new system via the interfaces or service class. The end result should be that Assembly-CSharp no longer contains battle-command logic, just a hookup to the new assembly.

Steps:

  • Remove Old Implementation: In Assembly-CSharp, locate the btl_cmd class (and any directly related types that were moved). You have a few options here:

    • Option A: Delete the btl_cmd class entirely from Assembly-CSharp. (Only do this if you’re confident nothing in Unity scene or prefab directly expects this class by name. Since it wasn’t a MonoBehaviour, it’s likely safe to remove if all code references are updated.)

    • Option B: Alternatively, keep a stripped-down btl_cmd class as a facade that calls into the new assembly. For example, you could leave btl_cmd.FinishCommand(...) in Assembly-CSharp, but implement it like:

      public static class btl_cmd  // in Assembly-CSharp, now just a wrapper
      {
          public static void FinishCommand(CMD_DATA cmd)
          {
              BattleCommandService.Instance.FinishCommand(cmd);
          }
      }
      

      This way, any existing code that hasn’t been refactored to use the new service directly will still function. This approach is safer if many parts of code call btl_cmd statically. Mark this as [Obsolete] to signal it will be removed, and encourage moving to direct use of the new service.

    • In either case, ensure that all significant logic has been removed from the Assembly-CSharp version. It should not be doing calculations or state changes – those should happen in the new module. The old code should at most pass data to the new code.

  • Update References in Other Classes: Search the entire project for usages of btl_cmd members:

    • Replace calls like btl_cmd.InitCommand(...) or btl_cmd.FinishCommand(...) with calls to the new service. If you instantiated a singleton (e.g. BattleCommandService.Instance), use that. For example:

      - btl_cmd.InitCommand(playerBtlData);
      + BattleCommandService.Instance.InitCommand(playerBtlData);
      

      If you went with fully injected design, you might pass around the BattleCommandService instance to where it’s needed (or access it via a central GameManager). Make sure every place that used btl_cmd now knows about the new service or has the needed interface.

    • If the original code accessed fields of btl_cmd (if any were public static fields), you’ll need to determine how to handle those. Possibly they become part of some context object in the new module. Eliminate any direct data sharing; use proper accessors or methods in the new API.

  • Provide Implementation for Interfaces in Original Code: We created interface adapters in step 2 – ensure those are properly integrated:

    • For example, if BattleCommandService requires an IBattleStatusService, you should have already written BattleStatusService : IBattleStatusService in the original code. Verify that this is being passed in during initialization (and that BattleStatusService calls the correct original methods like btl_stat.AlterStatus).
    • Likewise for any other interface: confirm that the real game logic is wired up. This is effectively the integration point: original game code fulfilling contracts that the new module calls. If any interface is not implemented, implement it or adjust the design to include it.
  • Test Integration in Game: Run the game with the refactored code. All functionality related to battle commands should behave exactly as before. Use both automated tests and manual testing in the actual game:

    • Start battles, use various commands (attacks, magic, etc.), ensure they execute correctly.
    • Test scenarios that involve the refactored logic (for instance, if Memoria’s IsTranceFull cheat was involved in FinishCommand, verify that trance now behaves correctly via the new code path).
    • Check that no new exceptions or errors occur. If something fails, use the unit tests and debugging to locate the issue (perhaps a missing interface implementation or incorrect wiring).
  • Remove Redundant Code: Once confirmed working, you can clean up any redundant pieces in Assembly-CSharp:

    • If Option B (facade) was used, you might later remove the facade once all code is switched to using the new assembly directly.
    • Any static global that was replaced by interface+implementation can potentially be internalized. For example, if FF9StateSystem.Settings was only used by btl_cmd, and now it’s behind IGameSettings, you might reduce direct exposure of that global if appropriate.
    • Ensure no leftover references to old btl_cmd exist (aside from perhaps the facade). This prevents confusion going forward.
  • Documentation and Comments: Optionally, update any developer documentation or comments in the project to note this change. If other developers are used to using btl_cmd directly, guide them to use the new BattleCommandService and interfaces. Because we included XML docs, developers can also read those to understand usage.

After integration, the btl_cmd logic is effectively decoupled: the new Memoria.BattleCommands assembly contains all the core logic and can be independently evolved or even reused in another project or a later game version, and the original Assembly-CSharp just provides the concrete connections (data and effects) through interfaces. This adheres to the principle that the core business logic does not depend on the game engine details, but rather the engine depends on the abstraction the logic defines.

Conclusion

By following these steps, we achieve a modular design for the battle command system:

  • The btl_cmd class and related logic live in a dedicated assembly, making it easier to read and maintain in isolation.
  • Dependency inversion through interfaces has removed direct couplings to Unity and game-singleton state, allowing for flexible testing and future changes. The high-level logic now depends on abstractions, and the concrete game details are injected from outside.
  • We preserved all serializable data layouts, so the Unity engine and game saves remain compatible with the refactored code.
  • Comprehensive xUnit tests now cover the behavior of each method, acting as a safety net for future refactoring and as documentation of expected outcomes.
  • All public APIs are documented in English with XML comments, following MSDN conventions, which will help both the current team and open-source contributors to understand and use the module effectively.
  • The original project is cleaned up to use the new module via clear interfaces, reducing clutter and confusion, and preparing the codebase for potential reuse in other contexts without pulling in the entire Assembly-CSharp.

This refactoring sets the stage for easier maintenance and extension of the battle command logic. New features can be added to the Memoria.BattleCommands module and tested in isolation. Other systems (AI, UI, etc.) can interact with it through well-defined interfaces, making the overall architecture more robust. By investing in this separation now, we make the Memoria project more adaptable to future changes (such as upgrading Unity versions or incorporating new mods) while minimizing the risk of regressions, thanks to the unit test coverage.

Albeoris avatar May 31 '25 17:05 Albeoris