Question Is my code all clunky and unoptimized?

Hi, there. After having watched a few basic Unity tutorials in the past and being comfortable with procedural coding, I've started working on my first "real" game. I'm managing just fine - I can, for the most part, get the stuff done. But I'm very commited to writing good code and develop good practice habits. And for that exact reason, I feel like the code I'm writing is all terribly clunky. Please help me identify if this is the case or not, and what I can change (I'm fine having to rework everything).

Some context: it's an isometric hero-based tower defense RPG. I have a GridManager game object with a GridManager script, that handles spawning all the tile prefabs at runtime, based on the info I saved in a tilemap scriptable object that I create in a map editor I've made (it is run as a separate scene). This GridManager saves every tile game object I instantiate into a GameObject[i, j] Grid array. Everything in the scene has a GridPosition script, based on what position they sit in the grid (also contains information on height and offsets to create the illusion on 3D with sorting layers and such).

The problem comes from the fact that there are a ton of proprieties I need to keep track and access. So a lot of times I write stuff like this:

Code (CSharp):

1. GameObject baseTile = GameObject.Find("GridManager").GetComponent<GridManager2>().Grid[this.gameObject.GetComponent<GridPosition>().i, this.gameObject.GetComponent<GridPosition>().j];

Using a million GetComponents and a Find sounds super clunky to me. Is it the case though, or is that just fine? It's supposed to return a simple Grid[i, j] game object, but its so much nonsense to get there. And my code is full of these!

I made a function to handle instantiating things at a certain tile.

Code (CSharp):

1. GameObject instancedDeployTile = GameObject.Find("GridManager").GetComponent<GridManager2>().InstantiateAtTile(deployTile, tile, deployHighlightTileParent, 1, -10, false);

Now I need to set its sorting layer value.

Code (CSharp):

1. GameObject.Find("GM").GetComponent<SpriteRenderOrder>().SetOrder(instantiatedObject, baseSortingOrder);

Do you get what I'm saying? I'm so clueless that I dont even know if I'm geting OOP all wrong and writing nonsense, or if this kind of logic is completely normal. I'm also so confused as to how to organize everything. Should the units prefab have a .Deploy() function, or should that be handled by an external script? Should I have a script that keeps track of all instantiated heroes and their stats, or just keep all that in the objects themselves? Those kinds of questions. But anyway, please shed me some light on all this. And thanks in advance!

 

@azeews27

"Using a million GetComponents and a Find sounds super clunky to me."

I recommend you watch some official Unity tutorials, those will help a lot with this issue.

Instead of finding MonoBehaviours when you need them every time, you could get your MBs in Start or such and store those into fields for later use. Or create public or serialized fields for your MBs and assign those from Inspector. Then call them like this:

myStoredGridManager.DoStuff();

 

GetComponent and GameObject.Find are not fast methods. You really don't want to be using them each frame.

Where possible, store a reference to the objects. You can do this in the start method or at the begging of the method call they are used in if need be.

For example. something like this could be stored in the class

Code (CSharp):

1. this.gameObject.GetComponent<GridPosition>()

like so

Code (CSharp):

1.         private GridPosition _gridPos;
2.  
3.     private void Start()
4.     {
5.         _gridPos = GetComponent<GridPosition>();
6.     }

then instead of this

Code (CSharp):

1.  GameObject baseTile = GameObject.Find("GridManager").GetComponent<GridManager2>().Grid[this.gameObject.GetComponent<GridPosition>().i, this.gameObject.GetComponent<GridPosition>().j];

you could write

Code (CSharp):

1.  GameObject baseTile = GameObject.Find("GridManager").GetComponent<GridManager2>().Grid[_gridPos.i, _gridPos.j];

Also, assuming the gridmanager is already present in the scene when that component starts, you could store a reference to it the same way.

As for your logic, just use what makes sense to you. Personally, I like to setup objects when they spawn and usually have some sort of initialse method.

 

Yeah, "clunky" is a good word for it. I wouldn't worry about "unoptimized" - the Find's and GetComponent's aren't the fastest way to go about it sure, but premature optimization will lead you down many dark paths. However, the bigger problems are reliability, especially with GameObject.Find. Using GO.F, you can never change the way your things are organized, you can never have even the tiniest typo, and errors will occur during runtime and not compile-time. It's these problems more than the speed problems that lead to my own opinion that there is never a time in runtime code where GameObject.Find is the best solution.

I wrote an article about this, which contains a list of alternatives to GameObject.Find.

In this case, it sounds like you could use a few things (detailed in the link). First is definitely singletons, which should be used with care and intention, but if there is always and ever only going to be one copy of something in your scene, they're appropriate. The GridManager is a perfect example of this.

As far as this code:

Code (csharp):

1.  this.gameObject.GetComponent<GridPosition>().j

I'd need to see more context, but this feels like your component (the script running this, I mean) should just have a permanent reference to its own GridPosition component? Either that, or perhaps it should be derived from a class that contains this information? Hard to say with just this snippet.

 

@eses @WarmedxMints @StarManta
I've reworked a whole lot of things and I think the code is way more solid now. Thanks a lot!

 

Putting aside the issue of performance and reliability of Find() and GetComponent() for a second, there's also an issue of readability with your code. These super long lines aren't really doing anyone a favor. For example:

Code (CSharp):

1. GameObject baseTile = GameObject.Find("GridManager").GetComponent<GridManager2>().Grid[this.gameObject.GetComponent<GridPosition>().i, this.gameObject.GetComponent<GridPosition>().j];

It's really hard to tell what's going on here. Throw some intermediate variables into the mix and break it up a bit. it will be a lot more understandable:

Code (CSharp):

1. GridManager gridManager = GameObject.Find("GridManager").GetComponent<GridManager2>();
2. GridPosition position = GetComponent<GridPosition>();
3.  
4. GameObject baseTile = gridManager.Grid[position.i, position.j];

It's a little more wordy overall, and yes more lines of code, but it's so much more readable! It will also run just as fast (if not faster because it's one fewer GetComponent() call).

Beyond readability, Imagine if you have a NullReferenceException somewhere in that huge line of code. The line number would barely tell you anything, since so much is happening in the one line. When you break it up it will be a lot more obvious what the error is.

 

In addition to the readability point @PraetorBlue mentioned, splitting long statements into multiple lines also helps a lot with debugging errors.
Suppose you get a null/unassigned reference exception and the error message is pointing to this line:

Code (CSharp):

1. GameObject baseTile = GameObject.Find("GridManager").GetComponent<GridManager2>().Grid[this.gameObject.GetComponent<GridPosition>().i, this.gameObject.GetComponent<GridPosition>().j];

There are three possible things here that could be null:

• The "GridManager" GameObject.
• The "GridManager2" component on the "GridManager" GameObject.
• The "GridPosition" component on this GameObject.

If it's not immediately obvious which one is null, then your only real option is to just guess and keep guessing until you find it.

Splitting the statements into multiple lines like this...

Code (CSharp):

1. GridManager gridManager = GameObject.Find("GridManager").GetComponent<GridManager2>();
2. GridPosition position = GetComponent<GridPosition>();
3.  
4. GameObject baseTile = gridManager.Grid[position.i, position.j];

...And it becomes instantly clear which reference would be null, since the line that the null reference is on would correspond with the line that the error message is pointing to.