How to keep clean code with linked classes?

Unity Engine
,
1

Hi everyone,

Since a while, I have tried to figure out how to keep my code clean and easy to change with many linked classes. Here is my problem.

I have an InteractionController class, a Player class, an Inventory class and an Equipment class.

What I would like to do is, the InteractionController trigger an intraction like a pickup item on ground. This is send to the player to add it to its inventory. Before adding it, he must check if it won’t be too heavy for him to carry this new item or these new items. So he asks for the inventory its total weight and the same for the equiment. If the addition of the current weight of both and the new items is under his limit, he adds it to its inventory.

My question here is, I have seen both of these videos Connecting scripts on the same object in Unity & Breaking up Code in Unity (Important game dev tips for beginners) and many others.

They look pretty good, I tried to replicate the second one by breaking up my player into fewer scripts like the InteractionController, Inventory, Equipment which are all Monobehaviour classes.

But with what I want to do, it seems that I must write a new function into the player class to add an item to its inventory and make all the validation steps to see if it is possible or not.

Here is a quick overview of what I have in mind, does it seem correct to you. I am scared of using this method because of the spaghetti code mentioned many times. I really would like to get your opinion on this.

public class InteractionController : MonoBehaviour
{
    Player player;

    public void Interact(GroundObject groundObject)
    {
        player.AddToInventory(groundObject.Item, groundObject.Amount);
    }
}

public class Player : MonoBehaviour
{
    float maxWeight;
    Inventory inventory;
    Equipment equipment;

    public void AddToInventory(Item item, int amount)
    {
        if (item.Weight * amount + inventory.GetTotalWeight() + equipment.GetTotalWeight() < maxWeight)
            inventory.AddToInventory(item, amount);
    }
}

public class Inventory : MonoBehaviour
{
    public void AddToInventory(Item item, int amount)
    {

    }

    public float GetTotalWeight()
    {
        return totalWeight;
    }
}

public class Equipment : MonoBehaviour
{
    public float GetTotalWeight()
    {
        return totalWeight;
    }
}
2

Consider creating a simple, reusable abstraction like IInteractable, and writing the InteractionController to work around this in a general manner, without knowing anything about GroundObject in specific. GroundObject could then just be one of many implementations of the IInteractable abstraction, and new ones could be added to the game without having to always go add more and more methods to InteractionController.

For example, something like this:

// contract that any interactable component must fulfill
public interface IInteractable
{
    bool CanInteract(Player player);
    void Interact(Player player);
}

public sealed class GroundObject : MonoBehaviour, IInteractable
{
    [SerializeField] Item item;
    [SerializeField] int amount;

    // GroundObject fulfills the contract
    public bool CanInteract(Player player) => item.Weight * amount + player.Inventory.GetTotalWeight() < player.Stats.MaxCarryWeight;
    public void Interact(Player player) => player.Inventory.Add(item, amount);
}

public sealed class InteractionController : MonoBehaviour
{
    IInteractable activeInteractable;

    void OnInteractionInputGiven()
    {
        //  InteractionController can work with any interactable, knows nothing about GroundObject
        if(activeInteractable != null && activeInteractable.CanInteract(player))
        {
            activeInteractable.Interact(player);
        }
    }
}

// Player could contain little to no logic, just references to other objects related to the player,
// to avoid it becoming a bloated god object with too many responsibilities.
public sealed class Player : MonoBehaviour
{
    [SerializeField] Inventory inventory;
    [SerializeField] Stats stats;

    public Inventory Inventory => inventory;
    public Stats Stats => stats;
}

[CreateAssetMenu]
public sealed class Stats : ScriptableObject
{
    [SerializeField] float maxWeight;

    public float MaxCarryWeight => maxWeight;
}

The key to making scalable systems that are easy to work with, is usually having the ability to extend them with new behaviour, without having to modify the existing classes (open-closed principle).

3 Likes
3

Thank you for your answer !!!

I did not write/explained it but it is what I have done for the GroundObject class by adding an interface. The only thing I didn’t do was to put the player as a parameter, which is dumb so I added it, thanks ;).

I have 2 questions, for your exemple, you only check for the inventory weight but I would like to check for the Inventory and Equipment so I guess what I can do is the following:

public interface IInteractable
{
    bool CanInteract(Player player);
    void Interact(Player player);
}

public sealed class GroundObject : MonoBehaviour, IInteractable
{
    [SerializeField] Item item;
    [SerializeField] int amount;

    public bool CanInteract(Player player) => player.CanAdd(item, amount);
    public void Interact(Player player) => player.Inventory.Add(item, amount);
}

[CreateAssetMenu]
public sealed class Stats : ScriptableObject
{
    [SerializeField] float maxWeight;

    public float MaxCarryWeight => maxWeight;
}

public sealed class Player : MonoBehaviour
{
    [SerializeField] Stats stats;
    [SerializeField] Inventory inventory;
    [SerializeField] Equipment equipment;

    public Inventory Inventory => inventory;
    public Stats Stats => stats;

    public bool CanAdd(Item item, int amount)
    {
        return item.Weight * amount + inventory.GetTotalWeight() + equipment.GetTotalWeight() < stats.MaxCarryWeight;
    }
}

public class Equipment : MonoBehaviour
{

}

public class Inventory : MonoBehaviour
{
    public void Add(Item item, int amount)
    {
        // Adding it to the inventory
    }
}

The second question is about the IInteractable which has 2 methods, Interact which is OK to me and CanInteract and I am asking if is it really necessary? It is not possible to set it directly into the player like this?

public interface IInteractable
{
    void Interact(Player player);
}

public sealed class GroundObject : MonoBehaviour, IInteractable
{
    [SerializeField] Item item;
    [SerializeField] int amount;

    public void Interact(Player player) => player.AddToInventory(item, amount);
}

[CreateAssetMenu]
public sealed class Stats : ScriptableObject
{
    [SerializeField] float maxWeight;

    public float MaxCarryWeight => maxWeight;
}

public sealed class Player : MonoBehaviour
{
    [SerializeField] Stats stats;
    [SerializeField] Inventory inventory;
    [SerializeField] Equipment equipment;

    public Inventory Inventory => inventory;
    public Stats Stats => stats;

    public bool AddToInventory(Item item, int amount)
    {
        if (CanAdd(item, amount))
        {
            inventory.Add(item, amount);
            return true;
        }

        return false;
    }

    public bool CanAdd(Item item, int amount)
    {
        return item.Weight * amount + inventory.GetTotalWeight() + equipment.GetTotalWeight() < stats.MaxCarryWeight;
    }
}

public class Equipment : MonoBehaviour
{

}

public class Inventory : MonoBehaviour
{
    public void Add(Item item, int amount)
    {
        // Adding it to the inventory
    }
}

Which really looks like what I wrote at first, isn’t it?

4

I would remove the Player.CanAdd method, and move the logic into GroundObject.CanInteract, to avoid the Player class getting bloated over time. Just add a Player.Equipment property, and then the GroundObject can pull all the information it needs and determine if the interaction is possible.

What the optimal abstraction is depends on the needs of your game. If for example you want to grey out an icon in your UI when an interaction isn’t possible, then you need to able to determine whether or not an interaction is possible, without also triggering it.

Simplicity is great; the simpler the interface can be made, the better! But like Mr. Einstein said, “Make things as simple as possible, but no simpler.”

2 Likes
5

To further clarify why I think it’s bad to centralize all the logic in Player, is that I think it can easily become a pain point once the number of interactions in the game increases. With just a few additional interactions, the number of methods in the Player class could already grow really long:

Player
{
    AddToInventory
    CanAdd
    Attack
    CanAttack
    Climb
    CanClimb
    Enter
    CanEnter
    Sit
    CanSit
    EquipGear
    CanEquipGear
    UnequipGear
    CanUnequipGear
    LevelUp
    IncreaseStat
}

When all this code is instead spread out to all the different interactable objects, the project can continue to be easily readable and extendable even when there are dozens of interactables.

2 Likes
6

I totally agree with that and that is what I would like to avoid having everything inside my player script.

What I am scared about with the spread of the logic into differents scripts is repetition. It means that before adding something to the inventory, each time I will need to check. If I create a new script for a merchant, for example, which adds items to the player, I need to write twice the same code:

// contract that any interactable component must fulfill
public interface IInteractable
{
    bool CanInteract(Player player);
    void Interact(Player player);
}

public sealed class GroundObject : MonoBehaviour, IInteractable
{
    [SerializeField] Item item;
    [SerializeField] int amount;

    // GroundObject fulfills the contract
    public bool CanInteract(Player player)
    {
        return item.Weight * amount + player.Inventory.GetTotalWeight() + player.Equipment.GetTotalWeight() < player.Stats.MaxCarryWeight;
    }
    public void Interact(Player player)
    {
        player.Inventory.Add(item, amount);
    }
}

public sealed class Merchant : MonoBehaviour
{
    Item[] items;
    int[] amounts;

    public void CanSell(Player player)
    {
        return items[0].Weight * amounts[0] + player.Inventory.GetTotalWeight() + player.Equipment.GetTotalWeight() < player.Stats.MaxCarryWeight;
    }

    public void Sell(Player player)
    {
        player.Inventory.Add(items[0].Weight, amounts[0]);
    }
}

public sealed class InteractionController : MonoBehaviour
{
    void OnInteractionInputGiven(IInteractable activeInteractable)
    {
        if (activeInteractable == null) return;

        //  InteractionController can work with any interactable, knows nothing about GroundObject
        if (activeInteractable.CanInteract(player))
            activeInteractable.Interact(player);
    }
}

[CreateAssetMenu]
public sealed class Stats : ScriptableObject
{
    [SerializeField] float maxWeight;

    public float MaxCarryWeight => maxWeight;
}

// Player could contain little to no logic, just references to other objects related to the player,
// to avoid it becoming a bloated god object with too many responsibilities.
public sealed class Player : MonoBehaviour
{
    [SerializeField] Stats stats;
    [SerializeField] Inventory inventory;
    [SerializeField] Equipment equipment;

    public Stats Stats => stats;
    public Inventory Inventory => inventory;
    public Equipment Equipment => equipment;
}

public class Equipment
{

}

public class Inventory
{

}

I really like the fact that each interaction is looking for its capability which seems pretty good and as you said everything is spread out among different scripts. But it is a trade with rewrite many times the same thing.

Or may be there is a better way to do what I have done (I guess yes).

7

The Inventory itself could definitely contain a bool TryAdd type method instead of void Add. Trying to design the public API of your classes in such a way that exceptions are impossible to occur is great practice.

As I envisioned it, IInteractable would only be used by the InteractionController. All other classes that need to interact with the Inventory for example, could just call its methods directly, without doing it through the interactable classes. So the public API for Inventory and IInteractable could be very different.

That is a good point.

If you find yourself in a situation, where two different classes need to check exactly the same thing, then it is usually a good idea to centralize that logic in one place, and have both classes use the same code to calculate the result. This ensures that if you make modifications to the logic in the future, all the relevant systems will be automatically updated to reflect the change and can’t go out-of-sync (single source of truth).

However, sometimes repeating identical code in multiple places isn’t a bad thing. There is a risk when multiple classes make use of a shared method, that when changes are made to this shared method for the benefit of one class, logic in any of the other classes could break. When there’s this sort of tight coupling between pretty unrelated classes, it can make it more difficult to make modifications to any of the classes without introducing bugs, and your project can become more fragile.

So it’s important to differentiate between classes that just happen to have some identical logic at this moment, but they could have slightly different logic in the future, and classes that should inherently always have exactly the same logic, and you want them to always remain synchronized.

It’s not always easy to spot the difference, but it’s good to keep in mind that code repetition is not always inherently bad, and can sometimes it’s much better than the alternative.

As for placing methods related to inventory management into the Player class, it kind of makes sense in that it has easy access to all the relevant classes… but it just feels wrong to me in terms of cohesion. It feels like things related to adding items should rather be part of the API of the Inventory, not the Player class. A method related to comparing the weight of an Inventory, Equipment and an Item to a maximum carry weight feels like it could be part of the Item’s public API, or a method in a static utility class ItemUtilility, but not in Player.

You could consider merging Inventory and Equipment into one class, if it feels like they are very tightly coupled together, and any methods you add into one needs to reference things in the other one. Then you could just use Inventory.GetTotalWeight() instead of Inventory.GetTotalWeight() + Equipment.GetTotalWeight() everywhere. Splitting classes into smaller ones isn’t always a good idea either, if it just ends up increasing overall complexity in the project.

You could also add a new struct like WeightLimit, and use that in Stats instead of a primitive float. That struct could then contain a method like WeightLimit.IsExceededBy(params[ ] float[ ] weights), which both Merchant and GroundObject could make use of.

But if it really feels like the best place to put those couple of methods is the Player class, then just go for it. You know your project best. And you can always refactor things later on, once your project has more code in it, and you have a better idea of where everything fits in naturally. You don’t want to get paralyzed with overanalyzing things either :slight_smile:

1 Like
8

I’d like to add that it should never be the responsibility of a ground item or a merchant to check if the one that is about to receive an item has enough capacity to do so. All this should be handlled by the player or the inventory. There are several ways how this could be abstracted. The Merchant and the ground object could just interact with the inventory. That way even NPCs or other things could interact with those entities in the same way. In that case the interface may have a callback to check if the item can be added (besides it’s own checks). So the player object who “owns” the inventory could provide a method and pass that to the inventory to do the weight check when you use the Add / TryAdd of the inventory.

Other objects should be agnostic about what that other thing is they interact with. Restrictions and rules that apply to an object should be enforced by that object and not by others.

2 Likes
9

Thanks for your answer! So it seems closer to what I first thought right?

So what I understand is having a duplicated function of AddToInventory in the Player class and make the check steps inside.

I am very curious of this part and you have any example as piece of code in mind, could you write it cause I struggle to image it.

10

That is a good point… I do agree, it’d be unintuitive if the thing that has a max capacity didn’t itself enforce it in any way.

And if the maximum carry capacity is an attribute of the Player object, it does make sense that it is the Player class that has the logic about how much it can carry. The Inventory itself has theoretically infinite capacity, it’s only when the Player object tries to carry that Inventory around, when the weight limit comes in.

It would be a different story, if it was the Inventory itself that had a maximum capacity, like a Backpack that just can’t fit more than a certain number of items, then the Player class could very well be left out of the equation.

So yeah, maybe adding methods like bool CanTakeItem(Item item) and bool TakeItem(Item item) directly to the Player class makes sense after all.

Then the method could even take into consideration factors other than just weight capacity in the future, like if the player is holding something in its hands and can’t take items because of this, or has frozen status effect or whatever.

11

Actually, I called it inventory but in reality, it is a backpack with a limited size. I did not want to mention it cause I did not want to provide more information than it was already. Also, it was not important to mention since the problem was still the same, my player is constrained by its max weight and if it is ok for him and he can carry the new load, then try to add it to the backpack if there is room for it.

So what I get from all of this is to go with the following:

public interface IInteractable
{
    void Interact(Player player);
}

public sealed class GroundObject : MonoBehaviour, IInteractable
{
    [SerializeField] Item item;
    [SerializeField] int amount;
    public void Interact(Player player)
    {
        if (player.AddToInventory(item, amount))
            Destroy(gameObject);
    }
}

[CreateAssetMenu]
public sealed class Stats : ScriptableObject
{
    [SerializeField] float maxWeight;
    public float MaxCarryWeight => maxWeight;
}

public sealed class Player : MonoBehaviour
{
    [SerializeField] Stats stats;
    [SerializeField] Inventory inventory;
    [SerializeField] Equipment equipment;
    public Inventory Inventory => inventory;
    public Stats Stats => stats;
    public bool AddToInventory(Item item, int amount)
    {
        if (CanAddToInventory(item, amount))
            return inventory.Add(item, amount)
        return false;
    }
    bool CanAddToInventory(Item item, int amount)
    {
        return item.Weight * amount + inventory.GetTotalWeight() + equipment.GetTotalWeight() < stats.MaxCarryWeight;
    }
}

public class Equipment : MonoBehaviour
{
    public float GetTotalWeight() => totalWeight;
}

public class Inventory : MonoBehaviour
{
    [SerializeField] float maxSize;

    public bool Add(Item item, int amount)
    {
        if (CanAdd(item, amount))
        {
            // Adding it to the inventory
            return true;
        }
        return false;
    }

    bool CanAdd(Item item, int amount)
    {
        return item.Size * amount <= maxSize;
    }

    public float GetTotalWeight() => totalWeight;
}
12

Looks good to me! The only thing I’d change is to have Player.CanAddToInventory internally check that Inventory.CanAdd is also true. Otherwise it seems to me that the method is lying. Or if not that, then at least rename the method to something more accurate like CanCarry :slight_smile:

13

Ok I see and that is right, it is more CanCarry than CanAddToInventory for the Player.

Also, what about you wrote in your previous post:

Because, with this method, I am going to reach this point with a Player who has pretty much all the methods that his Equiment, Inventory … have.

14

Yep… having an abstraction like Player in a project very easily leads down this path.

Avoiding this really boils down to coming up with alternative abstractions that encapsulate everything that is needed for a particular interaction.

As a thought exercise, you can try to imagine if you had a blank project, with no concept of a Player class, and you had to only write the bare minimum amount of classes needed to handle a particular interaction.

Following this line of thinking, interactions like “Pick Up Ground Item” and “Equip Inventory Item” could also be handled by a class like this:

//or InventoryController, InventoryHandler, PlayerInventory...
public class Inventory : MonoBehaviour
{
    [SerializeField] InventoryStats stats;
    [SerializeField] Equipment equipment;
    [SerializeField] Backpack backpack;

    public float GetTotalWeight() => equipment.GetTotalWeight() + backpack.GetTotalWeight();
    public float GetMaxWeight() => stats.MaxCarryWeight;

    public bool CanAddToBackpack(Item item, int amount) { ... }
    public bool AddToBackpack(Item item, int amount) { ... }
    public bool Equip(EquippableItem item) { ... }
    public bool Unequip(EquippableItem item) { ... }
}

Similarly you could have classes like ClimbHandler and CombatController, instead of the Player class, be responsible for handling climb and attack interactions respectively. And so on.

15

Ok so the goal is to encapsulate these methods with a new class if I have correclty understood.

So in my case, having a Player class which refer to an Inventory class which contains the Backpack class and the Equipment class, right? I guess it seems logic. It means that to start I can write everithing inside the player class and when it stars to get messy, create new classes to encapsulate some methods.

1 Like
16

Yeah that. Or you could use GetComponent instead of fetching all references through a Player class, if you want to have the interaction system work completely independently from the Player:

public sealed class InteractionController : MonoBehaviour
{
    void OnInteractionInputGiven() => activeInteractable?.Interact(gameObject))
}

public abstract class Interactable<TInteractionHandler> : MonoBehaviour, IInteractable where TInteractionHandler : Component
{
    public void Interact(GameObject interactor)
    {
        if(TryGetHandler(interactor, out TInteractionHandler interactionHandler))
        {
            Interact(interactionHandler);
        }
    }

    protected abstract void Interact(TInteractionHandler interactionHandler);

    protected virtual bool TryGetHandler(GameObject interactor, out TInteractionHandler handler) => interactor.TryGetComponent(out handler);
}

public sealed class GroundObject : Interactable<Inventory>
{
    protected override void Interact(Inventory inventory) => inventory.AddToBackpack(item, amount);
}

Both approaches should work just fine, in terms of avoiding the Player class becoming a huge god object.

2 Likes
17

Ok I see, going with the encapsulate class directly without passing through the Player.

That is a good idea, I have to check if it fits to what I would like to do but I am keeping it in mind.

1 Like
18

Pardon my saying so but the code is needlessly complicated in many places. That makes it more likely that bugs can hide. It doesn’t have to be “wrong” to be a source of problems.

You can see examples of where you use properties and then switch to methods for essentially the same “sort” of data. equipment.GetTotalWeight() vs stats.MaxCarryWeight for instance.

And Inventory can be substantially reduced (though I don’t know where totalWeight comes from in that class.

public class Inventory : MonoBehaviour
{
    [SerializeField]
    private float _maxSize;

    public float TotalWeight => totalWeight;

    public bool CanAdd(Item item, int amount) => item.Size * amount <= _maxSize;
}

Then there is a case where the Player determines CanAddToIventory is true but Inventory.Add determines that it cannot.

And what is the benefit of a private named maxWeight and a public named MaxCarryWeight? Wouldn’t both being named similarly be an improvement?

public sealed class Stats : ScriptableObject
{
    [SerializeField] float maxWeight;
    public float MaxCarryWeight => maxWeight;
}

.

19

Not sure to understand since what you wrote is pretty much the same as the code below right?

Or maybe you are talking about the following one?

20

Not at all :slight_smile: Check the example you posted for the Inventory class and compare it to what I posted. You have a private method that isn’t needed at all for instance.

In any case it isn’t a matter of making code smaller but rather to make it more concise. It should do what it needs to do, no more and no less. More “stuff” generally doesn’t improve the code but it can make it more likely to be misunderstood or to hide (or develop) conditional bugs.

Again I’ll just mention your intermixing of properties and methods for basic values. Stick to one style and you won’t wonder if it is a method or not.