I love the idea of a coding Kata. Even better, a refactoring one. Having a small project to tinker with and developing an improved version scratches a specific part of my brain. Refactoring Katas are a great way to practice best practices and learn new concepts. I had the idea to do one Kata I hadn't done before and write my thought process. This proved more difficult than I anticipated, and this post will be longer than usual. The result might not be the best, but I hope it will be fun to follow along. You can check my solution here, I tried to break the commits to make it easier to follow, I hope it helps. I have picked the Fantasy Battle Refactoring Kata from this repo because I enjoyed the RPG context that is settled in and it teaches the Law of Demeter, which is something that I don't know about. So, let's embark on this refactoring journey together!
It's always a good idea to read the README file from a Kata, it often gives you directions and requirements to guide you through. By reading the one provided, we know we must start by trying to implement a unit test and reflecting about it.
There are two unfinished tests in the PlayerTest class. Let's implement the DamageCalculationsWithMocks() method and remove the other one since having both doesn't make sense. If I execute the unit test as it is, it will fail because the Equipment property returns null on the Inventory mock. So let's set it to return something:
var inventory = new Mock<Inventory>();
inventory.Setup(i => i.Equipment).Returns(
new Equipment(
new BasicItem("round shield", 0, 1.4f),
new BasicItem("excalibur", 20, 1.5f),
new BasicItem("helmet of swiftness", 0, 1.2f),
new BasicItem("boots", 0, 0.1f),
new BasicItem("breastplate of steel", 0, 1.4f)
)
);
Let's update the expected damage to 114 since it's the current value, and we can assume that the production code is working. Here is the complete test:
[Fact]
public void DamageCalculations()
{
var inventory = new Mock<Inventory>();
inventory.Setup(i => i.Equipment).Returns(
new Equipment(
new BasicItem("round shield", 0, 1.4f),
new BasicItem("excalibur", 20, 1.5f),
new BasicItem("helmet of swiftness", 0, 1.2f),
new BasicItem("boots", 0, 0.1f),
new BasicItem("breastplate of steel", 0, 1.4f)
)
);
var stats = new Stats(1);
var target = new Mock<Target>();
var damage = new Player(inventory.Object, stats).CalculateDamage(target.Object);
Assert.Equal(114, damage.Amount);
}
Let's take a quick look at the Player class. It implements all damage calculation logic. The description of this Kata explains that we should learn the Law of Demeter during this exercise. I've never heard about this law, so I'll do a quick read. Just a sec... By reading, I understood that the central concept is to eliminate coupling.
A good phrase that I came upon during some research is: "Talk to your friends, not your friends' friends." So, in our case, the Player is talking to a stranger. Equipment is a stranger to Player since it's an Inventory property. And we even go further: we access equipment.LeftHand and leftHand.BaseDamage. We hide this coupling since we grab each equipment slot as a variable. To access the base damage of the left hand, we call Inventory.Equipment.LeftHand.BaseDamage. So we are talking to our friend's friend's friend's friend. This way, our Player class is tightly coupled to the Item interface.
Imagine if we had to rename the BaseDamage property to BruteDamage. Then, we must rename all usages in the Player class. Or even worse. Imagine that an Item can now have magic damage, and then we would add a MagicDamage property to the Item interface. To calculate the Total damage, we would need to add a new method for calculating the magic damage in the Player class.
By doing this change, we will remove one level of nesting and coupling between the Player and the Item class. But since, according to the description, this code is part of a larger fantasy battle game. We can't alter the Player's public methods. Because of that, I will keep CalculateDamage, but it will call a new public method in the Equipment class.
Let's create the EquipmentTest class with the following test.
[Fact]
public void CalculateBaseDamage_ShouldReturn_WhenFullSet()
{
// Arrange
var equipment = new Equipment(
new BasicItem("round shield", 0, 1.4f),
new BasicItem("excalibur", 20, 1.5f),
new BasicItem("helmet of swiftness", 0, 1.2f),
new BasicItem("boots", 0, 0.1f),
new BasicItem("breastplate of steel", 0, 1.4f)
);
// Act
var damage = equipment.GetBaseDamage();
// Assert
Assert.Equal(20, damage);
}
This code will not compile because Equipment does not yet have the CalculateBaseDamage method. So let's grab it from the Player, make some minor modifications and make it public.
namespace FantasyBattle
{
public class Equipment
{
// TODO add a ring item that may be equipped
// that may also add damage modifier
public Item LeftHand { get; }
public Item RightHand { get; }
public Item Head { get; }
public Item Feet { get; }
public Item Chest { get; }
public Equipment(Item leftHand, Item rightHand, Item head, Item feet, Item chest)
{
LeftHand = leftHand;
RightHand = rightHand;
Head = head;
Feet = feet;
Chest = chest;
}
public int GetBaseDamage()
{
return LeftHand.BaseDamage +
RightHand.BaseDamage +
Head.BaseDamage +
Feet.BaseDamage +
Chest.BaseDamage;
}
}
}
Now, we need to change the Player class to use this new method on the Equipment one.
public Damage CalculateDamage(Target other)
{
int baseDamage = Inventory.Equipment.CalculateBaseDamage();
float damageModifier = CalculateDamageModifier();
int totalDamage = (int)Math.Round(baseDamage * damageModifier, 0);
int soak = GetSoak(other, totalDamage);
return new Damage(Math.Max(0, totalDamage - soak));
}
Now all tests should pass, including our old PlayerTest.
I'll do the same process with the CalculateDamageModifier method. I'll not get into details because it is a similar process, but you can take a look at the commit diff on the project source if you have any problems. Here is the modified version of the Player class:
public class Player : Target
{
...
public Damage CalculateDamage(Target other)
{
int baseDamage = Inventory.Equipment.CalculateBaseDamage();
float damageModifier = CalculateDamageModifier();
int totalDamage = (int)Math.Round(baseDamage * damageModifier, 0);
int soak = GetSoak(other, totalDamage);
return new Damage(Math.Max(0, totalDamage - soak));
}
private float CalculateDamageModifier()
{
var equipmentDamageModifier = Inventory.Equipment.CalculateDamageModifier();
float strengthModifier = Stats.Strength * 0.1f;
return strengthModifier + equipmentDamageModifier;
}
...
}
I've decided to keep part of the DamageModifier in the player class because it uses the Player's stats. You could pass it as an argument like Equipment.CalculateDamageModifier(float strengthModifier). However, the Equipment class should not know how to handle stats modifiers.
Currently, the Player needs to have a complete set of equipment to perform an attack. The program will crash if the Player forgets to equip something on his feet. And that could be frustrating for the user. We should not change the system behavior during a Kata, but this new feature that we need to add in the future gives the idea that Items can be equipped or not.
"TODO add a ring item that may be equipped that may also add damage modifier".
I'll add a test scenario in the EquipmentTest class.
[Fact]
public void CalculateBaseDamage_ShouldReturnZero_WhenNoEquipment()
{
// Arrange
var equipment = new Equipment(null, null, null, null, null);
// Act
var damage = equipment.CalculateBaseDamage();
// Assert
Assert.Equal(0, damage);
}
This test will fail because of a NullReferenceException. So let's fix it. A quick fix would be to implement the following method in the equipment class:
private static int GetBaseDamage(Item item)
{
return item is null ? 0 : item.BaseDamage;
}
And use it for each item in CalculateBaseDamage, like so:
public int CalculateBaseDamage()
{
return GetBaseDamage(LeftHand) +
GetBaseDamage(RightHand) +
GetBaseDamage(Head) +
GetBaseDamage(Feet) +
GetBaseDamage(Chest);
}
It's not the prettiest, but it's OK for now. Our new test should pass. Let's do the same with CalculateDamageModifier.
All tests are passing, and we already made good progress. Let's take a look at our Player class.
public class Player : Target
{
public Inventory Inventory { get; }
public Stats Stats { get; }
public Player(Inventory inventory, Stats stats)
{
Inventory = inventory;
Stats = stats;
}
public Damage CalculateDamage(Target other)
{
int baseDamage = Inventory.Equipment.CalculateBaseDamage();
float damageModifier = CalculateDamageModifier();
int totalDamage = (int)Math.Round(baseDamage * damageModifier, 0);
int soak = GetSoak(other, totalDamage);
return new Damage(Math.Max(0, totalDamage - soak));
}
private float CalculateDamageModifier()
{
var equipmentDamageModifier = Inventory.Equipment.CalculateDamageModifier();
float strengthModifier = Stats.Strength * 0.1f;
return strengthModifier + equipmentDamageModifier;
}
private int GetSoak(Target other, int totalDamage)
{
int soak = 0;
if (other is Player)
{
// TODO: Not implemented yet
// Add friendly fire
soak = totalDamage;
}
else if (other is SimpleEnemy simpleEnemy)
{
soak = (int)Math.Round(
simpleEnemy.Armor.DamageSoak *
(
simpleEnemy.Buffs.Select(x => x.SoakModifier).Sum() + 1
), 0
);
}
return soak;
}
}
We are still talking with strangers and getting the base damage and equipment damage modifier through Inventory.Equipment.CalculateBaseDamage(); and Inventory.Equipment.CalculateDamageModifier(); respectively.
We could move the Equipment class to the Player, which makes sense because a piece of equipment is something that the Player has. But since this Kata says that this is part of a larger system, I'll not change this, and I'll put some accessors in the Inventory class instead.
public int EquipmentBaseDamage()
{
return Equipment.CalculateBaseDamage();
}
public float EquipmentDamageModifier()
{
return Equipment.CalculateDamageModifier();
}
Now, we can get the base damage and equipment modifier using this method:
public Damage CalculateDamage(Target other)
{
int baseDamage = Inventory.EquipmentBaseDamage();
...
}
private float CalculateDamageModifier()
{
var equipmentDamageModifier = Inventory.EquipmentDamageModifier();
...
}
To make testing easier, I'll create an interface IIventory so we can mock those methods.
namespace FantasyBattle
{
public interface IInventory
{
int EquipmentBaseDamage();
float EquipmentDamageModifier();
}
}
And we can modify our existing test to something like this:
[Fact]
public void DamageCalculations()
{
var inventory = new Mock();
inventory.Setup(i => i.EquipmentBaseDamage()).Returns(20);
inventory.Setup(i => i.EquipmentDamageModifier()).Returns(5.6F);
var stats = new Stats(1);
var target = new Mock();
var damage = new Player(inventory.Object, stats).CalculateDamage(target.Object);
Assert.Equal(114, damage.Amount);
}
Most of the Player class now respects the Law of Demeter but the GetSoak method. But we didn't cover GetSoak with tests, so I'm not confident in refactoring it. Let's add some tests in PlayerTest that use a SimpleEnemy as a target with some Armour and Buffs.
[Fact]
public void CalculateDamage_ShouldConsiderArmor_WhenTargetIsSimpleEnemy()
{
var inventory = new Mock();
inventory.Setup(i => i.EquipmentBaseDamage()).Returns(20);
inventory.Setup(i => i.EquipmentDamageModifier()).Returns(1.0F);
var stats = new Stats(1);
var target = new SimpleEnemy(
new SimpleArmor(5),
new List<Buff>(){ new BasicBuff(1.0F, 1.0F)
});
var player = new Player(inventory.Object, stats);
var damage = player.CalculateDamage(target);
Assert.Equal(12, damage.Amount);
}
Now, we can start refactoring the GetSoak method. One way to simplify is by adding a CalculateSoak to the SimpleEnemy class. But if you look closer both SimpleEnemy and Player are children of the abstract class Target. Putting a CalculateSoak method in the Target class is tempting, but Player and SimpleEnemy could have different implementations. In the future, we could implement different kinds of enemies. So, a better approach would be transforming Target into an Interface and adding the CalculateSoak on its contract. That way, we are forced to implement both Player and SimpleEnemy implementations.
namespace FantasyBattle
{
public interface ITarget
{
int CalculateSoak(int totalDamage);
}
}
Now our Player class looks like this:
public class Player : ITarget
{
public IInventory Inventory { get; }
public Stats Stats { get; }
public Player(IInventory inventory, Stats stats)
{
Inventory = inventory;
Stats = stats;
}
public Damage CalculateDamage(ITarget other)
{
int baseDamage = Inventory.EquipmentBaseDamage();
float damageModifier = CalculateDamageModifier();
int totalDamage = (int)Math.Round(baseDamage * damageModifier, 0);
int soak = GetSoak(other, totalDamage);
return new Damage(Math.Max(0, totalDamage - soak));
}
// NEW: Player ITarget implentation
public int CalculateSoak(int totalDamage)
{
// TODO: Not implemented yet
// Add friendly fire
return totalDamage;
}
private float CalculateDamageModifier()
{
var equipmentDamageModifier = Inventory.EquipmentDamageModifier();
float strengthModifier = Stats.Strength * 0.1f;
return strengthModifier + equipmentDamageModifier;
}
private int GetSoak(ITarget other, int totalDamage)
{
return other.CalculateSoak(totalDamage);
}
}
Notice that we can remove that ugly if-else statement from GetSoak and call other.CalculateSoak instead. And we added the Player's implementation for ITarget. Here is our implementation for SimpleEnemy:
public class SimpleEnemy : ITarget //Extends ITarget
{
public virtual Armor Armor { get; }
public virtual List<Buff> Buffs { get; }
public SimpleEnemy(Armor armor, List<Buff> buffs)
{
Armor = armor;
Buffs = buffs;
}
//NEW: SimpleEnemy ITarget implementation
public int CalculateSoak(int totalDamage)
{
return (int)Math.Round(
Armor.DamageSoak *
(
Buffs.Select(x => x.SoakModifier).Sum() + 1
), 0
);
}
}
All tests should still pass. You can look at the source on GitHub if you have any problems.
We currently have some problems related to project structure:
SimpleEnemy.cs contains the definition of the SimpleEnemy class, Buff, and Armor interfaces.Now that we have refactored most of the code, we can start implementing the new features written in the comments.
I'll add a new Item property called Ring as an optional parameter in the constructor. Then, I'll consider it in the damage modifier calculation. Finally, we can add a unit test on the EquipmentTest that verifies that rings are only considered in the damage modifier.
I'll start by adding the Dexterity property to the Stats class:
public class Stats
{
public virtual int Strength { get; }
public virtual int Dexterity { get; }
public Stats(){}
public Stats(int strength, int dexterity)
{
Strength = strength;
Dexterity = dexterity;
}
}
Now we can change the CalculateDamageModifier and CalculateSoak methods:
public int CalculateSoak(int totalDamage)
{
var dexterityModifier = Stats.Dexterity * 0.05f;
return totalDamage - (int)dexterityModifier; // NEW
}
private float CalculateDamageModifier()
{
var equipmentDamageModifier = Inventory.EquipmentDamageModifier();
float strengthModifier = Stats.Strength * 0.1f;
float dexterityModifier = Stats.Dexterity * 0.05f; // NEW
return strengthModifier + dexterityModifier + equipmentDamageModifier;
}
There is a better way to implement it. We are still talking to our friend's friend, and if we need a new stat, we need to change the player class. That's not good since it violates the Open-Closed principle from solid. I'll leave it as it is for now, and we can refactor it after adding some unit tests.
We already have some unit tests in the PlayerTest class that cover the CalculateDamage method. Let's change a bit to use Dexterity as well.
[Fact]
public void CalculateDamage_ShouldConsiderArmor_WhenTargetIsSimpleEnemy()
{
var inventory = new Mock<IInventory>();
inventory.Setup(i => i.EquipmentBaseDamage()).Returns(20);
inventory.Setup(i => i.EquipmentDamageModifier()).Returns(1.0F);
var stats = new Stats(2, 5); //MODIFIED: Added more strength and Dexterity
var target = new SimpleEnemy(
new SimpleArmor(5),
new List<Buff>(){ new BasicBuff(1.0F, 1.0F)
});
var player = new Player(inventory.Object, stats);
var damage = player.CalculateDamage(target);
Assert.Equal(19, damage.Amount); //MODIFIED: The damage value increase due to our new stats! =)
}
I'll add the CalculateDamageModifier on the Stats class and use it on our Player one.
public class Stats
{
...
public float CalculateDamageModifier()
{
float strengthModifier = Strength * 0.1f;
float dexterityModifier = Dexterity * 0.05f;
return strengthModifier + dexterityModifier;
}
}
Our modified Player class
public class Player : ITarget
{
...
private float CalculateDamageModifier()
{
var equipmentDamageModifier = Inventory.EquipmentDamageModifier();
return Stats.CalculateDamageModifier() + equipmentDamageModifier;
}
}
This looks much better. We can add any stats we want, and we don't need to change the Player class to impact the damage. Currently, the dexterity stat is not affecting the Damage Soak. I'll implement this on the next feature, the friendly fire.
If we attacked another Player, the total damage would be 0 because CalculateSoak returns the total damage.
public int CalculateSoak(int totalDamage)
{
// TODO: Not implemented yet
// Add friendly fire
return totalDamage;
}
This new feature has no requirements, so I'll keep it simple to avoid making this blog post any longer. I'll reduce the damage by half and apply the Dexterity soak modifier, and that's it.
public int CalculateSoak(int totalDamage)
{
return totalDamage / 2 + (int)Stats.CalculateSoak();
}
We can verify that this is working by creating a unit test for it.
[Fact]
public void CalculateDamage_CanAttackAnotherPlayer()
{
// Arrange
var inventory = new Mock<IInventory>();
inventory.Setup(i => i.EquipmentBaseDamage()).Returns(20);
inventory.Setup(i => i.EquipmentDamageModifier()).Returns(1.0F);
var stats = new Stats(2, 5);
var player = new Player(inventory.Object, stats);
var target = new Player(null,new Stats(0, 20));
// Act
var damage = player.CalculateDamage(target);
// Assert
Assert.Equal(14, damage.Amount);
}
You can go even further and consider the Player's Armor for soaking the damage. This will be a more significant refactoring, but you can do it!
That's concludes our refactoring Kata, this was really fun and there is a lot of ways you can go further in the refactoring. We discovered the importance of the Law of Demeter, which guided us to reduce coupling and create more maintainable code. We did a series of refactorings and we finished with a more maintainable solution.
I hope you had fun and enjoyed the process, Keep on learning!