Circular dependency follow up

[Cat_Hill]

Brian, I saw your comments on resolving the circular dependency. I was a little uncomfortable with the idea of doing GetComponent on a passed in GameObject.

I also like to use [RequireComponent(typeof(whatever))] to help keep me catch my own stupid mistakes. A side benefit is that it seems to help with catching otherwise hidden dependencies.

My idea - create a MonoBehaviour called something like “Character” and put the [RequireComponent] attribute there for Fighter, Mover, Health, etc. Character can live in RPG.Core. Then we can pass around a Character instead of a PlayerController or GameObject. Character can even have wrappers for the public fighter, mover, etc methods to create even more abstraction.

Curious if any parts of the above are good ideas, especially in a team environment where there may be multiple contributors.

References:

1. Dependency Graph for RPG project
2. Is putting namespace Control on CombatTarget causing a circular dependency?

[Brian_Trotter]

@Cat_Hill

I was a little uncomfortable with the idea of doing GetComponent on a passed in GameObject.

Don’t be. This is how things should generally be done. Your HandleRaycast only needs to know about the components it’s dealing with directly.

@Cat_Hill

My idea - create a MonoBehaviour called something like “Character” and put the [RequireComponent] attribute there for Fighter, Mover, Health, etc. Character can live in RPG.Core

This part is ok, though I would put it in a whole new namespace RPG.Characters. Core should know nothing about ANY other RPG namespaces. In fact, I refactor out Core in my personal projects, because the scripts in Core are ones that most of the namespaces need. Instead, I put those in the RPG namespace. Here’s why: If the script is in namespace RPG; then RPG.Combat has access to it automatically, as well as all other RPG.myNameSpace namespaces.

Here’s specifically why putting Character inside RPG.Core or RPG is a bad idea:

using RPG.Combat;
using RPG.Attributes;
using RPG.Movement;

namespace RPG.Core;
{
    [RequireComponent(typeof(Health))]
    [RequireComponent(typeof(Fighter))]
    [RequireComponent(typeof(Mover))]
    public class Character : MonoBehaviour
    {
    }
}

Now, if RPG.Fighter, RPG.Mover, or RPG.Attributes needs access to Core, you’ve created a direct cross dependency between the namespaces.

@Cat_Hill

Then we can pass around a Character instead of a PlayerController or GameObject.

Stick to passing around GameObjects. See dependency issue above.

@Cat_Hill

Character can even have wrappers for the public fighter, mover, etc methods to create even more abstraction.

This is how you create tightly coupled hidden cross dependencies… If your class depends on Character, it now depends on every class Character depends on. We’re after loosely coupled classes as independent as possible. Now any class that uses Character will be dependent on any other class that Character depends on. You’ve not created more abstraction, you’ve removed abstraction from the equation.

[Cat_Hill]

@Brian_Trotter

Core should know nothing about ANY other RPG namespaces.

Oh right. Yeah I wasn’t thinking straight here and didn’t see the dependencies I would have created. That would have been a silly mistake

@Brian_Trotter

@Cat_Hill

I was a little uncomfortable with the idea of doing GetComponent on a passed in GameObject.

Don’t be. This is how things should generally be done. Your HandleRaycast only needs to know about the components it’s dealing with directly.

Hmmm OK. But if I look at the snippet below there’s nothing in there that tells me its OK to rely on the fact that the passed in parameter has a Fighter component. If it fails, it will fail with a null pointer exception where it won’t be obvious what happened.

I could always add some null checking below, but what would you suggest as a preventative approach that doesn’t add unnecessary coupling?

I’m imaging a situation where person A writes the IRaycastable interface, “person B” writes a class that implements IRaycastable and “person C” writes code that calls HandleRaycast.

public bool HandleRaycast(GameObject caller)
{
   Fighter fighter = caller.GetComponent<Fighter>();
   if (!fighter.CanAttack(gameObject))
   {
      return false;
   }
}

[Brian_Trotter]

@Cat_Hill

I could always add some null checking below, but what would you suggest as a preventative approach that doesn’t add unnecessary coupling?

Null checks and logging.

@Cat_Hill

I’m imaging a situation where person A writes the IRaycastable interface, “person B” writes a class that implements IRaycastable and “person C” writes code that calls HandleRaycast.

public bool HandleRaycast(GameObject caller)
{
     if(caller.TryGetComponent(out Fighter fighter))
     {
          if(!fighter.CanAttack(gameObject))
          {
               return false;
          }
          return true;
     }
     Debug.LogWarning($"Configuration error: {caller.name} does not have a Fighter Component!");
     return false;
}

[Cat_Hill]

Got it. So the conclusion would be against having anything in the interface.