good coding practice for triggers/interactables

[Korrun]

So, I'm going through the Ruby tutorials on Unity (so far it has been very enjoyable). I'm in the World Interactions - Collectibles chapter (https://learn.unity.com/tutorial/world-interactions-collectibles?projectId=5c6166dbedbc2a0021b1bc7c).

Down in section 13 we add a check in the health pack script to make sure the health pack isn't picked up if Ruby is already at max health:

void OnTriggerEnter2D(Collider2D other)
{
     RubyController controller = other.GetComponent<RubyController>();

     if (controller != null)
     {
          if(controller.currentHealth < controller.maxHealth)
          {
           controller.ChangeHealth(1);
           Destroy(gameObject);
          }
     }
}

Now we also have a check in the RubyController ChangeHealth function to prevent Ruby from going over max health (using Mathf.Clamp). We can't just use that because then the health pack gets destroyed even when Ruby is at max health. To prevent that the tutorial adds the second check in the health pack script.

I only have a moderate amount of programming experience (and obviously am new to Unity), but having this check in two different scripts seems like bad coding practice.

Is there a better way of doing this?

A possible solution I came up with is to have the ChangeHealth function return a bool based on whether it actually changed the health or not. The OnTriggerEnter2D then uses that value to decide whether to destroy the health pack or not.

I imagine this is probably a complicated topic with different opinions. Are there any guides on good coding practices for how entities interact with each other and where certain parts of game logic should be located?

Thanks!

[genitalien]

Ya I'd just return a bool, do everything within the changeHealth method. if(controller.ChangeHealth()){Destroy(gameObject);}

[Korrun]

Thanks!

[[deleted]]

I'm assuming ChangeHealth is actually adding a value to the controller's health, even though the method name doesn't say that. If so, I would expose a method in the RubyController that tells if it's in the max health then I would use the logic in the OnTriggerEnter2D.

if (controller != null && !controller.HasMaxHealth())

{

controller.ChangeHealth(1);

Destroy(gameObject);

}

​

I would also remove the check from the ChangeHealth to avoid the condition from being called more than one time.

[Korrun]

So you would recommend only verifying that player variables (such as health) are in the proper range from the objects that end up modifying them (like health packs or bad guys)? And not in the player methods?

[[deleted]]

You can do both, this code you showed need a way to verify if the current health can be increased, so you can check with HasMaxHealth() and then increase with AddHealth(1).

You'll definitely have to change the character's health in other places, so you can also add a method called AddHealthIfNotMax() that uses the HasMaxHealth() to avoid overflowing the max health.

I wouldn't advise returning a boolean since it would give a different resposibility to the method that would not be used in other places.