Code Quality Design Help

How do we write a test for a private method?

Normally we would test it indirectly through the class public interface but this could be difficult with legacy code.

Feathers discusses this in his excellent book: Working Effectively with Legacy Code - Michael C. Feathers 2005, chapter 20.

This class violates the Single Responsibility Principle.

Player_health : int_mana : int_inventoryItems : List<GameItem>_equippedWeapon : WeaponItem_playerLevel : intTakeDamage(amount: int) : voidCastSpell(spell: Spell) : voidPickupItem(item: GameItem) : voidEquipBestWeapon() : void FindStrongestWeaponInInventory() : WeaponItemCanEquipItem(item: WeaponItem) : booleanCalculateItemEffectiveDamage(item: WeaponItem) : float

It seems that Player has the following responsibilities:

  • Managing core player statistics (health, mana, level).

  • Handling inventory.

  • Managing and calculating logic for equipping items.

  • Handling combat actions.

The Player class has public methods like TakeDamage, PickupItem, and EquipBestWeapon. The detailed logic for finding and validating equipment is hidden in private methods like FindStrongestWeaponInInventory and CanEquipItem. What would the Player class be like if we made these private equipment-related methods public? Well, it would seem pretty odd. Users of the Player might get the idea that they are directly responsible for these low-level equipment decisions, or might misuse them, rather than using a higher-level action like EquipBestWeapon.

It would be odd to have those methods public on the Player class, but it is far less odd—and, actually, perfectly fine—to make them public methods on an EquipmentManager class.

Player and EquipmentManagerPlayer_health : int_mana : int_inventoryItems : List<GameItem>_equippedWeapon : WeaponItem_playerLevel : int_equipmentManager : EquipmentManagerTakeDamage(amount: int) : voidCastSpell(spell: Spell) : voidPickupItem(item: GameItem) : voidEquipBestWeapon() : voidEquipmentManagerFindStrongestWeapon(availableItems: List<GameItem>, playerLevel: int) : WeaponItemCanPlayerEquipItem(item: WeaponItem, playerLevel: int) : booleanCalculateEffectiveDamage(item: WeaponItem, playerLevel: int) : float_equipmentManager

You might notice that during this refactoring, method names can be slightly adjusted (e.g., FindStrongestWeaponInInventory became FindStrongestWeapon). This often occurs because the methods, now in a new class, operate in a different context. They may require explicit parameters for data that was previously implicit (like player level or the specific list of items), leading to names and signatures that better reflect their new, more generalized role in the EquipmentManager.

We moved the responsibility of Equipment management and logic into a different class. We can now test FindStrongestWeapon(), CanPlayerEquipItem(), and CalculateEffectiveDamage() directly since they are now public on EquipmentManager.

Note that applying SRP is iterative. Even after this extraction, Player still manages multiple concerns. Further refactoring could extract additional classes like Inventory, CombatStats, and SpellCaster, each improving testability and simplifying the Player class.

Of course, if EquipmentManager is a volatile dependency then we need to hide it behind an interface to avoid violating the Dependency Inversion Principle.

Player, IEquipmentManager, and EquipmentManagerIEquipmentManagerFindStrongestWeapon(availableItems: List<GameItem>, playerLevel: int) : WeaponItemCanPlayerEquipItem(item: WeaponItem, playerLevel: int) : booleanCalculateEffectiveDamage(item: WeaponItem, playerLevel: int) : floatEquipmentManagerFindStrongestWeapon(availableItems: List<GameItem>, playerLevel: int) : WeaponItemCanPlayerEquipItem(item: WeaponItem, playerLevel: int) : booleanCalculateEffectiveDamage(item: WeaponItem, playerLevel: int) : floatPlayer_health : int_mana : int_inventoryItems : List<GameItem>_equippedWeapon : WeaponItem_playerLevel : int_equipmentManager : IEquipmentManagerTakeDamage(amount: int) : voidCastSpell(spell: Spell) : voidPickupItem(item: GameItem) : voidEquipBestWeapon() : void_equipmentManager

In conclusion, testing private methods can be a challenge, but it is important to have good test coverage for our code. If we need to test a private method, we should make it public or move it to a separate class. This helps to improve the design of the class and make the method more testable. In the case of the Player class, it was found that it had multiple responsibilities and violated the Single Responsibility Principle. By moving the equipment management logic to a separate EquipmentManager class, the design became clearer, and the methods could be tested directly.

See Also:

09 May 2025