Question Cleaning Up Code

So this is my snippet of code for setting a weapon's stats to a script that handles all of the attacking. For the most part, it looks fine (Although I suppose stats like the damage and attack speed could be consolidated into the parent class), but I feel like there is a way I could make this a lot cleaner.

 

Please use code tags.

Looks plenty good, probably not worth worrying about it unless you need to rework it anyway and see a good opportunity to improve it.

Just a small suggestion, if you have an

ItemStats

subclass for every

WeaponClass

, you could just switch on the object to avoid needing an explicit cast:

Code (CSharp):

1. switch (weaponInfo.itemStats)
2. {
3.     case MeleeItemStats meleeItemStats:
4.         // ...
5.         break;
6.     case RangedItemStats rangedItemStats:
7.         // ...
8.         break;
9.     case MagicItemStats magicItemStats:
10.         // ...
11.         break;
12. }

 

Oops, will make sure next time to do that lol.

Otherwise, thanks for the help! Didn't know you could use switch statements like that.

 

Even better, if you have interfaces for that:

Code (CSharp):

1. public interface IHaveAttackSpeed {
2.   float attackSpeed;
3. }
4.  
5. public interface IHaveWeaponDamage {
6.   float damageMin;
7.   float damageMax;
8. }
9.  
10. public interface IHaveAttackRange {
11.   float attackRange;
12. }
13.  
14. public class MeleeItemStats : IHaveAttackSpeed, IHaveAttackRange,IHaveWeaponDamage {}
15. public class RangedItemStats : IHaveAttackSpeed {}
16. public class MagicItemStats : IHaveAttackSpeed, IHaveAttackRange, IHaveWeaponDamage {}
17.  
18. if (weaponInfo.itemStats is IHaveAttackSpeed speedStats) {
19.     weaponAttackHandler.setAttackSpeed(speedStats.attackSpeed);
20. }
21.  
22. if (weaponInfo.itemStats is IHaveAttackRange rangeStats) {
23.     weaponAttackHandler.setAttackRange(rangeStats.attackRange);
24. }
25.  
26. if (weaponInfo.itemStats is IHaveWeaponDamage damageStats) {
27.     weaponAttackHandler.setWeaponDamage(damageStats.damageMin, damageStats.damageMax);
28. }
29.  

This way you dont care about the actual implementation and can easily add new stuff (as long as you dont need new interfaces, every new classes using at least on of those interfaces will work without further code change)

Edit: use if instead of switch

 

Ah, I think this is what I was looking for. I was looking into using generics, but I don't think that was really the proper way to go about it.

 

It's not quite clear where the shown code is actually located. It seems your "weaponInfo.itemStats" is some sort of base class and you have different implementations with different stats. In such a case I would probably go for a polymorphic approach since each concrete type knows what kind of stats it has and wants / needs to apply. So when you have a virtual "Apply" method in your base class, you could simplifiy your code to something like:

Code (CSharp):

1. weaponInfo.itemStats.ApplyTo(weaponAttackHandler);

So each concrete class could override the virtual method and implement the instance specific logic. Maybe you even have something that is the same for all classes (that is already defined in the base class, maybe the AttackSpeed?) so you could implement that in the base method so when overriding the method you simply call the base implementation. Well, just simple OOP.

Interfaces can be great, depending on the usecase. Note that a switch case will always only hit a single case, the first one that matches. So if the object implements several interfaces, you would only ever set a single value. Your pattern would make more sense as separate if statements

Code (CSharp):

1.  
2. if (weaponInfo.itemStats is IHaveAttackSpeed speedStats)
3.     weaponAttackHandler.setAttackSpeed(speedStats.attackSpeed);
4. if (weaponInfo.itemStats is IHaveAttackRange rangeStats)
5.     weaponAttackHandler.setAttackRange(rangeStats.attackRange);
6. if (weaponInfo.itemStats is IHaveWeaponDamage damageStats)
7.     weaponAttackHandler.setWeaponDamage(damageStats.damageMin, damageStats.damageMax);

 

Yep, fixed it up with some virtual/override functions, and I cut it down all to this.

Code (CSharp):

1.  
2.         ItemStats newItem = weaponInventory[i].GetComponent<WeaponInfo>().itemStats;
3.         WeaponInfo weaponInfo = weaponInventory[i].GetComponent<WeaponInfo>();
4.  
5.  
6.         weaponAttackHandler.setWeaponClass(newItem.weaponClass);
7.         weaponAttackHandler.setWeaponTip(weaponInfo.weaponTip);
8.         weaponAttackHandler.setProjectile(weaponInfo.projectile);
9.  
10.         weaponAttackHandler.setAttackRange(newItem.getAttackRange());
11.         weaponAttackHandler.setAttackSpeed(newItem.getAttackSpeed());
12.         weaponAttackHandler.setWeaponDamage(newItem.getDamageMin(), newItem.getDamageMax());

Gave up on the interface method, because I had 0 clue how to work those with variables, but at least for functions, I think I might find a use later in my project.

 

Why not invert the relationship and avoid the switch all together?

Code (csharp):

1. public class ItemStats
2. {
3.  
4.     public virtual void SyncStats(WeaponAttackHandler weaponAttackHandler) {}
5.  
6. }
7.  
8. public class MeleeItemStats : ItemStats
9. {
10.  
11.     public float attackSpeed;
12.     public float attackRange;
13.     public float damageMin;
14.     public float damageMax;
15.  
16.     public override void SyncStats(WeaponAttackHandler weaponAttackHandler)
17.     {
18.         weaponAttackHandler.setAttackSpeed(attackSpeed);
19.         weaponAttackHandler.setAttackRange(attackRange);
20.         weaponAttackHandler.setWeaponDamage(damageMin, damageMax);
21.     }
22.  
23. }
24.  
25. public class RangedItemStats : ItemStats
26. {
27.  
28.     public float attackSpeed;
29.  
30.     public override void SyncStats(WeaponAttackHandler weaponAttackHandler)
31.     {
32.         weaponAttackHandler.setAttackSpeed(attackSpeed);
33.     }
34.  
35. }
36.  
37. public class MagicItemStats : ItemStats
38. {
39.  
40.     public float attackSpeed;
41.     public float attackRange;
42.     public float damageMin;
43.     public float damageMax;
44.  
45.     public override void SyncStats(WeaponAttackHandler weaponAttackHandler)
46.     {
47.         weaponAttackHandler.setAttackSpeed(attackSpeed);
48.         weaponAttackHandler.setAttackRange(attackRange);
49.         weaponAttackHandler.setWeaponDamage(damageMin, damageMax);
50.     }
51.  
52. }
53.  
54.  
55. //your original post context
56. weaponInfo.itemStats.SyncStats(weaponAttackHandler);

Now if you need to add more ItemStat types the code that consumes it doesn't need to care about it. The new ItemStats class just implements the necessary methods to make it work.

You know... use inheritance for what it's intended for.

Note...this could also be abstracted into an interface to decouple inheritance hierarchies:

Code (csharp):

1. public class IItemStats
2. {
3.  
4.     void SyncStats(WeaponAttackHandler weaponAttackHandler);
5.  
6. }
7.  
8. public class MeleeItemStats : IItemStats
9. {
10.  
11.     public float attackSpeed;
12.     public float attackRange;
13.     public float damageMin;
14.     public float damageMax;
15.  
16.     public void SyncStats(WeaponAttackHandler weaponAttackHandler)
17.     {
18.         weaponAttackHandler.setAttackSpeed(attackSpeed);
19.         weaponAttackHandler.setAttackRange(attackRange);
20.         weaponAttackHandler.setWeaponDamage(damageMin, damageMax);
21.     }
22.  
23. }
24.  
25. public class RangedItemStats : IItemStats
26. {
27.  
28.     public float attackSpeed;
29.  
30.     public void SyncStats(WeaponAttackHandler weaponAttackHandler)
31.     {
32.         weaponAttackHandler.setAttackSpeed(attackSpeed);
33.     }
34.  
35. }
36.  
37. public class MagicItemStats : IItemStats
38. {
39.  
40.     public float attackSpeed;
41.     public float attackRange;
42.     public float damageMin;
43.     public float damageMax;
44.  
45.     public void SyncStats(WeaponAttackHandler weaponAttackHandler)
46.     {
47.         weaponAttackHandler.setAttackSpeed(attackSpeed);
48.         weaponAttackHandler.setAttackRange(attackRange);
49.         weaponAttackHandler.setWeaponDamage(damageMin, damageMax);
50.     }
51.  
52. }
53.  
54.  
55. //your original post context
56. weaponInfo.itemStats.SyncStats(weaponAttackHandler);

edit: Just noticed this is what Bunny83 said too.