Oct 9, 2016

Spring cleaning

So in my previous post I discussed how I had returned to this project after a 7 month hiatus. To get back on track I decided to do a bit of code spring-cleaning. Its actually spring here in the southern hemisphere, so it seems appropriate.

Before beginning work again on the game proper, one of the aspects I wanted to improve was the testability of the codebase and eliminate the global references that have spread through the codebase like cancer. Note: my codebase is written in C# in which global objects are defined as "static", so where I say "static", it's the same as global.

First step; make the static classes instantiable!


It turns out that I converted my EntityManager class to not be static at some point, so that was a huge advantage. However, it is created once, early in the life-cycle of the application and is stored in a global that is referenced throughout the app... bugger... about 500 times... double bugger.

Also I have a mixture of local server and client code that refers to the same references but use them in different ways, and some references that use [ThreadStatic] as a workaround to correct problems where different threads needs different versions of the same object. Truly the stuff of nightmares!

Because of that I'd written some truly horrific unit tests that had to start up most of the game engine in order to test something simple like the serialisation of a component. Seemed like an excellent place to start my clean up!

Dependencies to the rescue!


I refactored a lot of code to accept an IEntityManager instead of using the global reference, and over time my unit tests cleaned up considerably. I applied the same logic to other manager objects and eventually my unit tests became completely independent of the rest of the codebase, relying on mocked versions of the important managers, injected as dependencies.

I continued this initiative and carried it over to other globals, and I've managed to completely remove some of the  references entirely. The client and server code is now safely segregated and the ThreadStatics have been removed.

The aforementioned EntityManager global still has 200 references to it, but that's a great improvement on the 500+ that were there when I started this effort. Furthermore, the AssetTemplateManager is down from over 100 to just 27 references. There's still some way to go - the target is to remove all global references..

So, lessons to be learnt here? Static/Global references are bad and once introduced to your codebase they tend to spread unchecked. I've been working on this codebase for over 5 years, and I've learnt a great deal about C# and development in general during that time. Anything in the project older than a year makes me cringe, and there's stuff still in use that was bedded down in the first few months of the project. I had no idea about dependency injection or why global references were a bad idea, and now I've seen the error of my ways.

On a side note, I've discovered that Squarepusher is great for refactoring!

No comments:

Post a Comment