sigtrap

Sixth in a series of posts on Software Design for Unity. Read the introduction here.

We’re finally back to the topic that prompted this whole thing. I didn’t want to start off by being too controversial, but I think you know what I’m about by now.

So, even more than usual: OPINION WARNING!

What is Factoring?

First off – it’s not refactoring. That’s entirely separate^[1]Refactoring, loosely, is rewriting code to change how it works internally without changing the way it interfaces with the rest of your codebase.. Factoring is the process or practice of putting code into functions or methods. Simple. Now, a lot of coding wisdom says that factoring is basically a thing you should do all the time, particularly with a view to following the “single responsibility principle”. This principle states that any one unit of code should perform one task, and one task only.

What a “task” means depends on what the “unit” is – a single “task” can be composed of multiple smaller tasks. For example, the “task” of a Bullet class (yes, I use this example a lot) is to travel forward, detect collisions, and damage things it hits. Clearly that can be broken down further – therefore it makes sense to have “units” of code within the Bullet class (most likely methods) which perform sub-tasks. Each sub-unit then should perform one sub-task – for instance, working out how much damage to actually do.

So the single responsibility principle is open to interpretation, depending on what you class as a “single” responsibility. I’m a big believer in the principle, but in my opinion there’s a tendency to take it too far.

Dealing with Long Code

Consider the following code.

public class Bullet : MonoBehaviour {
	public float speed;
	public float damage;

	private void Update(){
		// Store previous position for raycast
		Vector3 prevPos = transform.position;
		// Travel forward
		transform.position += transform.forward * speed * Time.deltaTime;
		// Raycast for collision detection
		RaycastHit hit;
		if (Physics.Linecast(prevPos, transform.position, out hit)){
			// If collide with something damageable, do damageable
			Damageable d = hit.collider.GetComponent();
			if (d){
				d.Damage(damage);
			}
			// Destroy bullet
			Destroy(gameObject);
		}
	}
}

This is, as ever, pretty simple, and it’s easy to see what’s going on even without comments. As usual though I’ll ask you to imagine it getting more complex somehow. Perhaps the movement calculation has to account for other factors such as acceleration or force fields, or the damage code needs a calculation based on current speed, angle, etc etc. Basically, imagine that Update() has swelled to, say, five times the length.

At this point, conventional wisdom would say it’s time to factor. Rather than having one long Update() method, you should break it up into smaller sub-methods to make it easier to read, understand and maintain. So it might look something like this:

public class Bullet : MonoBehaviour {
	public float speed;
	public float damage;
	private Vector3 _prevPosition;
	private bool _haveHitSomething;
	private Damageable _thingToDamage;
	private float _damageToDo;

	private void Update(){
		// Nice and clean!
		Move();
		CheckCollision();
		CalculateDamage();
		DoDamage();
		DestroySelf();
		StorePreviousPosition();
	}
	
	private void Move(){
		// Do loads of complex movement
	}
	private void CheckCollision(){
		// Do complicated collision detection using _prevPos for raycast
		// Set _haveHitSomething and _thingToDamage if needed
	}
	private float CalculateDamage(){
		// Calculate all sorts of stuff for _damageToDo, if _thingToDamage exists
	}
	private void DoDamage(){
		// Apply damage in a tricky way to _thingToDamage, if it exists, using _damageToDo
	}
	private void DestroySelf(){
		// Check _haveHitSomething and destroy self if so
	}
	private void StorePreviousPosition(){
		// Store previous position for raycast in _prevPos
	}
}

Reasons Not to Factor

The question is this. Is this more readable and easy to maintain than just having all of that code inline in Update? I’d argue not^[2]I’ll admit that I’ve purposefully gone to extremes. Even if each of those methods is fairly complex, it’s unlikely you have, for instance, StorePreviousPosition as a separate method. And you’d probably roll CalculateDamage and DoDamage into one. But hear me out..

For a start, note that we’ve now got four new fields in the class, to store the temporary data generated by each sub-method. Always, always avoid adding statefulness if you can – state bugs are easy to make and hard to find. We can, of course, get rid of this – we can make CheckCollision return a bool, only running the damage and destroy code methods if true, and have an out Damageable parameter. Likewise we can make CalculateDamage to take that Damageable as a parameter, and return a float. We can then pass that float, and the Damageable, into DoDamage.

public class Bullet : MonoBehaviour {
	public float speed;
	public float damage;
	
	private void Update(){
		Vector3 prevPos = transform.position;
		Move();
		Damageable d;
		bool collide = CheckCollision(out d);
		if (d){
			float dmg = CalculateDamage(d);
			DoDamage(dmg, d);
		}
		if (collide){
			DestroySelf();
		}
	}
	...
}

But now we’ve done quite a lot of extra work, and suddenly Update doesn’t look so clean after all. True, it’s not bad, but that’s not the question – is it better than inline code? Well, we’ve still broken things out into their own neat methods, and that’s worth something right?

Well… again, I’d argue not. If anything, I tend to think that breaking it up like that makes it harder to know exactly what’s happening in Update. If all the code is inline (which, remember, also means scope is a bit simpler rather than having to think about what variables are being passed around), you can just read it from top to bottom. If it’s factored, you’ve got to jump to that method, read it and jump back. In doing so, you’ve got to remember the context in which that sub-method is called, and what scope you’re working with. Easy for short bits of code, sure – but short bits of code is not the time conventional wisdom advises you to start factoring.

Compare that to breaking up your long Update function with comments, or #region directives. This way, your code stays in the order it gets called, and the context and scope are always obvious because they’re right there.

When to Factor

As per usual – this isn’t a blanket argument against factoring. Just that I’m wary of doing anything ‘for the sake of it’. Assuming I’ve convinced you – and there’s a good chance I’ve not – what are the good reasons to factor?

Using #region directives breaks up code but retains linearity

Number one is, and always will be, code reuse. If you use Move(), or CheckCollision(), or CalculateDamage() in multiple places, they should absolutely be their own methods. Don’t write anything twice that you could write just once. Duplication encourages errors, frustration, and things just not working how you expect. If you’ve got two separate implementations of your damage code and have forgotten about it, you might change one and then tear your hair out working out why goblins, but nothing else, are still taking too much damage. Take care, though, to heed the above warnings about state. As far as possible, use parameters and returns rather than fields!

A second one, particularly for Unity, is profiling. Since the profiler (when using deep profiling) breaks down performance on a per-method basis, if you really want to drill down you may want to factor specific parts of your code.

Edge cases

There are two commonly-cited arguments for factoring which I sometimes find convincing, sometimes not. The first one is simple – really long code. Obviously, this comes down to personal preference. I still tend to prefer longer code to broken up code, but there are exceptions.

The second one is stack traces. When your code throws an exception, or you use Debug.Log(), Unity will include a helpful stack trace in the output showing what method caused the error, what method called that method, what method called the method that called the method… and so on. This is really useful in tracking the chain of events that caused a problem. The argument then is that more factoring gives you a more granular stack trace, rather than just telling you the error happened in Update().

In my mind there are a couple of problems with this logic. Firstly, in the editor (and development builds) the stack trace won’t just tell you the method, but the line number. So more granularity isn’t really needed – you can go straight to the issue, and as above, reading the chain of events is obvious from the fact that all your code is in one line. The utility of a stack trace is being able to follow all the jumps your method calls have made; if there are no jumps, it’s just linear anyway.

In release builds of course, you don’t get line numbers. So if an exception is thrown, your stack trace may not be a huge amount of use without lots of factoring. This is more persuasive, but in my experience – bold claim – isn’t usually that relevant. If you’re being careful, you should have fairly good error reporting in your public builds already. And bugs that throw exceptions aren’t usually the tricky ones to find or fix, with or without a stack trace. Those which don’t are the hard ones, and the ones likely to find their way into release – strange behaviour that’s totally within the rules of the code you’ve written, but you’re not sure why. In short, stack traces are extremely useful in development (in which environment they don’t benefit from factoring), and generally not a big deal in release builds. This one can go either way though – in practice, some conservative factoring may help here!

Bad Reasons to Not Factor

You may hear that you should avoid factoring because method calls are more expensive than inline code. Ignore that. If factoring genuinely helps readability in a specific case, don’t worry about the overhead of calling a method. Firstly, such overheads are almost always totally insignificant compared to the generally running costs of using a managed language like C# in the first place. Secondly, the compiler is smart. It will quite happily make a method inline if and where it’s advantageous to do so. Thirdly, even if it did aid performance a bit… meh. Readability and maintainability is almost always more valuable than performance^[3]within obvious limits..

Don’t do things ‘just because’

Conclusion

This is a relatively minor example – in all honesty, over-factoring is not likely to cause problems compared to other pitfalls like misuse of singletons or writing code that doesn’t tell you when it’s gone horribly wrong. But this goes beyond single responsibility and factoring.

Don’t do things ‘just because’. The single responsibility principle is there to help you write code. It’s not there to be adhered to blindly. If in a particular case, a particular interpretation of a particular principle will be detrimental to your code – don’t do it. Sounds obvious when it’s stated like that, but it can be very easy to get caught up in one pattern or another and lose sight of why you were using it in the first place.