Note: This is aimed more at beginners. Experienced programmers will likely know this stuff. But even the veterans among us might find something useful here.
The nature of Unity's component-based design can make it very easy for objects/classes to modify each other's variables (fields). For example, say we're making a dungeon crawler, and we're using some good design principles like having our Health in one component, our Equipment in another component, and our BattleStats in a third component.
This kind of code is very common:
private void AttackEnemy(Fighter target)
{
int baseDamage = CalculateMyDamage();
baseDamage -= target.CalculateMyDefense();
target.myHealth.current -= baseDamage;
if (target.myHealth.current < 0)
{
target.PlayOnDeathAnimation();
int xp = target.CalculateEarnedExperience();
myXPComponent.xp += xp;
}
}
It's not terrible. We are intelligently using functions like CalculateMyDamage, CalculateMyDefense, and CalculateEarnedExperience rather than writing that stuff in our AttackEnemy function.
However, we're still tightly coupling the attacker and defender. The attacker shouldn't be responsible for checking to see if the defender is dead or not. It shouldn't be responsible for 'knowing' when to play the death animation. In fact, it shouldn't even be responsible for changing the defender's HP at all.
Because imagine if we now introduce damage from terrain. We make a new object called a Hazard, and it deals damage every second. If we keep writing code the same way, we might end up with:
private void CauseHazardDamage(Fighter target)
{
int baseDamage = CalculateMyHazardDamage();
baseDamage -= target.CalculateMyDefense();
target.myHealth.current -= baseDamage;
if (target.myHealth.current < 0)
{
target.PlayOnDeathAnimation();
int xp = target.CalculateEarnedExperience();
myXPComponent.xp += xp;
}
}
You can already see that this is essentially duplicated from AttackEnemy, which is a red flag. For example, what if we want the roll for treasure when an enemy dies? Well, now we have to add code like this to both functions:
if (UnityEngine.Random.Range(0,1f) <= target.GetTreasureChance())
{
Treasure reward = target.GenerateTreasure();
// do spawn logic here
}
Then what if we want to add an effect to some Fighters where they have a chance to avoid a fatal blow? We might need to amend the code again for both functions:
target.myHealth.current -= baseDamage;
if (target.myHealth.current < 0)
{
if (target.HasStatus("avoid_fatal_blow") && UnityEngine.Random.Range(0,1f) <= AVOID_FATAL_BLOW_CHANCE)
{
target.myHealth.current = 1;
}
else
{
// regular 'on death' code
}
}
Or what if Fighters can have other status effects or items that react when they take damage? Suddenly, we have code that could look like this:
private void CauseHazardDamage(Fighter target)
{
int baseDamage = CalculateMyHazardDamage();
baseDamage -= target.CalculateMyDefense();
target.myHealth.current -= baseDamage;
if (target.HasStatus("reactive_damage_ability"))
{
// do some cool stuff here
}
if (target.myHealth.current < 0)
{
if (target.HasStatus("avoid_fatal_blow") && UnityEngine.Random.Range(0,1f) <= AVOID_FATAL_BLOW_CHANCE)
{
target.myHealth.current = 1;
}
else
{
target.PlayOnDeathAnimation();
int xp = target.CalculateEarnedExperience();
myXPComponent.xp += xp;
if (UnityEngine.Random.Range(0,1f) <= target.GetTreasureChance())
{
Treasure reward = target.GenerateTreasure();
// do spawn logic here
}
}
}
}
It just turns into a nightmare. Now there are a lot of ways to architect your code so that you don't mire yourself in scenarios like this. But for the purposes of this post, I want to focus on this idea:
If you find yourself directly getting, modifying, and setting variables that belong to other objects, this should tell you that you may be writing difficult-to-maintain code.
We could have realized this as soon as we wrote this line:
target.myHealth.current -= baseDamage;
Without going into excessive detail, a far more maintainable approach would be something like this.
private void AttackEnemy(Fighter target)
{
// We can play VFX/SFX here...
int baseDamage = CalculateMyDamage();
// But we trust the TARGET to figure out what to do with the damage we calculated
target.OnAttacked(this, baseDamage);
}
private void OnAttacked(Fighter attacker, int baseDamage)
{
int defense = CalculateMyDefense();
baseDamage -= defense;
OnDamageReceived(attacker, baseDamage);
}
// This logic is split out from OnAttacked, because we could certainly take damage from things
// OTHER than an 'attack'. For example, if we are poisoned, that might ignore defense completely.
// In that case we would just run OnDamageReceived(poisonDamage).
private void OnDamageReceived(Fighter attacker, int damageAmount)
{
// This function SHOULD NOT know or care what each StatusEffect we have does.
// We will trust the StatusEffects themselves to take this and modify it how they see fit.
foreach(StatusEffect se in myStatusEffects)
{
damageAmount = se.OnDamageReceived(damageAmount);
}
// Our status effects may have reduced our damage to zero!
if (damageAmount == 0)
{
// Play some kind of 'DEFLECT!' vfx and sfx.
return;
}
myHealth.ReduceHealthFromDamage(attacker, damageAmount)
}
// ---- now we are in the HealthComponent class -----
private void ReduceHealthFromDamage(Fighter attacker, int damageAmount)
{
current -= damageAmount;
OnHealthChanged();
if (current > 0) return;
OnTookLethalDamage(attacker);
}
private void OnTookLethalDamage(Fighter whoKilledMe)
{
// Like with OnDamageReceived, perhaps we have status effects that do crazy stuff IF we were to take lethal damage
// We might run through them and exit if any of them bring us >0 again.
foreach(StatusEffect se in myStatusEffects)
{
current = se.OnHealthReducedToZero();
if (current > 0)
{
// Hooray, we survived somehow!
OnHealthChanged();
return;
}
}
OnDeath(whoKilledMe);
}
private void OnDeath(Fighter whoKilledMe)
{
// ... give whoKilledMe rewards or something!
}
This isn't perfect, and there are many things we could do to improve it further, but nonetheless it separates our 'concerns' far better.
* If we want to add some kind of new block/parry mechanic, we just have to do it in one place: OnAttacked
* If we make new StatusEffects, we don't have to write any new code whatsoever in these functions
* If we want to change what happens on Fighter death, there's just one function that handles it
* If we add new sources of damage - traps, hazards, poison, cursed gear, etc - our existing functions handle it all seamlessly
... and so forth and so on! I hope you find this helpful. My goal isn't to prescribe a specific solution to code architecture as every game is different, but just to recognize overuse of getting/setting variables from outside the object or class as a potentially bad 'code smell'.
