Which option follows better coding principles?

Hello, sorry in advance for the long read. I tried formatting so it's easy to follow :)

I'm building a grid-based game. I have a service called the GridService, as well as a TileManager monobehavior.

Both have their corresponding interfaces to fulfill the Dependency inversion principle I'll go into in a bit.

GridService is a single permanent class that only gets created when the game loads. It persists between scenes.

TileManager is a private Singleton. This means that the static Instance cannot be accessed like a normal singleton, but it does force only a single TileManager per scene (which is the goal).

I am trying to assign the (only) TileManager per scene to the GridService. It is NOT possible to do the following:

  • Assign the TileManager in the inspector because it changes when scenes are switched
  • Create a new instance of TileManager. It is a monobehavior that has prefab references

I have come up with two ways to do it.

Option 1: Register
This option lets each TileManager register itself with the GridService.

TileManager.cs

protected override void Awake()
{
     base.Awake();
     var service = ServiceLocator.Instance.GetService<IGridService>();
     service.RegisterTileManager(this); 
}

GridService.cs

public void RegisterTileManager(ITileManager tileManager)
{
    _tileManager = tileManager;
}

public void Generate()
{
     _grid = new Grid(_gridWidth, _gridHeight);
     _path = new Path(_grid, _maxNumberOfLoops);

     _tileManager.SpawnTiles(_grid);
}

At first the screamed "Circular dependency" to me. GridService calls TileManager which also calls GridService. However, after looking more in-depth I read something that dependency inversion via interfaces prevents the above from being a circular dependency. If this is true, this is the way I will probably go.

Option 2: FindObjectByType
Using FindObjectByType<> (or less ideally, GameObject.Find()) to fetch the TileManager:

public void Generate()
{
     _grid = new Grid(_gridWidth, _gridHeight);
     _path = new Path(_grid, _maxNumberOfLoops);

     var tileManager = FindObjectOfType<TileManager>();
     tileManager.SpawnTiles(_grid);
}

I really don't like this method for 2 reasons:

  • It hurts performance, however, the generate function is only called on game load so I'm not worried
  • It can't be used with interfaces which means GridService would have a direct dependency on TileManager. Again, not really a problem since TileManager no longer calls GridService but it still irks me when it has an interface.

Which option is better? Is there a 3 option that is more ideal for this scenario?
Thank you :)

Your first example is essentially the S tier option, through code only. Singletons aren't BAD, if they fit the circumstances that are solved by singletons. You have done that analysis.

Your second example is... one of the others.9799671--1406613--tier-list.png

2 Likes

Then that's not really a Singleton but rather a Multiton.
A Singleton is instantiated once, never again. When switching scenes you have two singletons, if only for a short time during which both scenes exist.

It's best to design the singleton to be truly singular. It should react to the scene having changed, and get or request or be told that there's new data to work with but keep the singleton instance itself in DontDestroyOnLoad.

This solves your two key problems. GridService can call TileManager.Singleton.Something() any time it likes.

Also, what about Tiles needs "managing"? ;)
Is it a collection of Tiles? Then it's a TileCollection.
Does it allow you to test tiles for collisions? Then it's a TileCollider.
And so on ... generic noun + "Manager" suffix rarely makes for a class name that actually spells out what the class' purpose is. The same could be said about "GridService" although I think it's just not a service but you meant to call it GridLayout.

Thank you for all this feedback. It really helps

So TileManager is not going to be used as a singleton in the usual sense. Instance is private so
TileManager.Instance.Something()
is not possible. It's just a safety to ensure only one can exist at a time or it gets destroyed.

If this is true, then I'll probably just remove the singleton nature of TileManager and just put a
Debug.LogWarning("TileManager already exists");
or something

This project is actually for a game I'm challenging myself to learn various advanced techniques and how arcitectural patterns can be used in Unity. I want to avoid overuse of the Singleton pattern. Currently, I'm only using it for the ServiceLocator to get services.

So, this is a tower defense game. When starting the game in the main menu you can see the procedural map generated, adjust # of loops, and hit regenerate. In this case, the TileManager has references to UI prefabs and spawns them in a canvas. Once you are happy with the grid, the game starts, now, the TileManager that is in-world takes over and spawns the 3D prefabs in the world.

9890700--1427607--upload_2024-6-14_9-7-26.png

My goal was separation of concerns and the SRP. TileManager is unique enough that it could be moved from GridService and have two separate use cases to make it expandable via interfaces (aka, not just have a single TileManager that handles both)

Technically, TileSpawner is a more accurate name. But TileManager sounds better lol.

I only named it that to meet my naming conventions of things created via the ServiceLocator. Its goal is simply to create the grid and path in C# (so only classes and structs, no unity representation of this grid).

I hope this clarifies some stuff :)

9890700--1427607--upload_2024-6-14_9-7-26.png
9890700--1427607--upload_2024-6-14_9-7-26.png

Thank you for that tier list. That's actually extremely helpful.

I agree, I also like the first option.

Is this statement I read true? The only thing holding me back from using the first option is that this looks like a circular dependency. Which is more of a potential problem to me than some lost performance.