My intent in this post is to share my perspective on the subject of “Clean Code” and promote the understanding about the need for this practice. There is an old adage: “If you want to learn a subject, write a book on it.” While I’m not writing a book, this is still is a learning experience for me, and, hopefully, for you too.

Most of the inspiration on this subject is taken from Robert C. Martin (aka Uncle Bob) and his books about clean code (Clean Code and The Clean Coder). If you haven’t read these books or don’t know who Uncle Bob is, I suggest you do so as soon as possible (right after reading this post ;)). Also, check out his blog, which has really interesting topics.

Readability:

Clean code is about how easy it is to read the code that you write. The known joke: “It was hard to write it, it should be hard to read it,” constitutes the main problem of code in general: it is hard to read code. So, our main concern is to make the code readable by people. To make your code more understandable, you’ll need skill, practice and, most importantly, the understanding of how clean code looks.

Naming:

I won’t tell you that variables and functions should have meaningful names–that’s obvious. I will tell you, though, that the name of a function should convey its intent, meaning that when you read the implementation of a function you should say “well, of course it does that.” And, while conveying the method’s purpose is important, don’t use very long names, because even with today’s 23-inch wide screens, a long method name is distracting. Besides, if you do have a long method name, it usually means that the method does too much (see “Small Methods” section later in this post). As for the variable names, they should be short and to the point, but without losing their meaning.

If Statements:

“If” statements should be as simple as possible. Don’t put too many or’s and and’s into it. Try to put the most simple bool variables. If there are a couple of check variables, move them into methods that encapsulate the boolean logic. Reduce the nesting of the “if” to the bare minimum by setting breaks and return statements where possible.

Small Methods:

It can’t be overemphasized enough that all the methods you write should be as small as possible, and should follow the single responsibility principle (Solid). Small methods contribute not only to readability, but to good design practices and code structure in general. It’s easier to understand smaller methods because the smaller it is, the less complexity there is to see. Therefore, it has less cyclomatic complexity.

Comments:

Everyone loves a well documented API, and everyone suffers when it isn’t well documented ( Unity3D developers suffer a lot). But, comments should be used sparingly. A redundant comment hurts readability and, therefore, the understanding of the code. Another reason for writing less comments is the fact that they introduce duplication. As the life of a project progresses, many of the classes and methods change behavior, but the comments accompanying them are often left behind to the point where they become inaccurate, or worse, plain wrong. I’m not against commenting, though. There are times when it is absolutely mandatory, such as: a complex algorithm or other computation of things, a design decision and reasons for it or a public API that will be used by other people. Just use it with caution, because sometimes a well named method or variable is better.

Conventions:

The most important thing about conventions is that everyone on your team should follow the same ones. It is helpful, however, that you follow the standard convention of your language and have tools to enforce it. One of the major reasons is that it promotes clearer communication between developers and removes a “context switch” in your mind when looking at your teammates’ code. Standard conventions also helps new team members integrate more easily into an ongoing project.

Summary

To summarize this post, let’s review the following example (taken from a real world project). It is written in C# in the context of Unity3D, however the ideas behind them are universal and language-agnostic. Consider the following:

[SerializeField]
private List<OneShotEffect> _effectPrefabs = new List<OneShotEffect>();

public void PlayEffect(Effect effectToPlay, Vector3 worldPosition, float? duration = null)
{
    foreach (var oneShotEffect in _effectPrefabs)
	{
		var effectToPlayName = effectToPlay.ToString();
		var checkEffectName = oneShotEffect.name;

		if (string.Compare(checkEffectName, effectToPlayName, StringComparison.InvariantCultureIgnoreCase) == 0)
		{
			var instance = (OneShotEffect)Instantiate(oneShotEffect, worldPosition, Quaternion.identity);
			instance.InitSelfDestruct(duration);
			instance.transform.SetParent(transform);

			instance.Play();
		}
	}
}

At a first glance, this method is not that bad because it is relatively short and readable.Taking a closer look, we can see that the name of the method is pretty straightforward and it conveys its intent. Same goes with the name of the parameters (Effect is an enum if you’re wondering.) The foreach loop iterates through all the effect prefabs, so no problem there. Next, two name variables: they could have been passed as they were to the string.Compare, but are left this way for debugging purposes. Now the if statement: what it does is clear, but when you read the code you need to stop the reading flow and start analyzing: “Ok, we have a string comparison of the two names, and this is a special case of the comparison(StringComparison.InvariantCultureIgnoreCase), oh and equal to zero is probably when the strings are equal (if you don’t remember strcmp from C).” This is a lot of thinking for one single if statement, and while it only takes a second and a half to get, it breaks your flow and interferes with your attempt to understand how the method accomplishes its task. So let’s clean it by extracting a method from the if statement:

private bool AreEffectNamesEqual(string firstEffectName, string secondEffectName)
{
    return string.Compare(firstEffectName, secondEffectName, StringComparison.InvariantCultureIgnoreCase) == 0;
}

Now the if statement looks like this:

if (AreEffectNamesEqual(effectPrefabName, effectToPlayName))

Let’s move on. Here we have the core of this method, the instantiation and init of the found effect, and, like before, we’re going to extract a method from it:

private OneShotEffect CreateAndSetupEffect(OneShotEffect effectPrefab, Vector3 worldPosition, float? duration)
{
    var effectInstance = (OneShotEffect)Instantiate(effectPrefab, worldPosition, Quaternion.identity);
	effectInstance.InitSelfDestruct(duration);
	effectInstance.transform.SetParent(transform);

	return effectInstance;
}

I’ve also changed the AreEffectNamesEqual method to a more sensible string comparison, the name of the Vector3 parameter (to effectWorldPosition) and the name of the foreach loop variable (to effectPrefab). So, the final version looks like this:

[SerializeField]
private List<OneShotEffect> _effectPrefabs = new List<OneShotEffect>();
public void PlayEffect(Effect effectToPlay, Vector3 effectWorldPosition, float? duration = null)
{
    foreach (var effectPrefab in _effectPrefabs)
	{
		var effectToPlayName = effectToPlay.ToString();
		var effectPrefabName = effectPrefab.name;

		if (AreEffectNamesEqual(effectPrefabName, effectToPlayName))
		{
			var effectInstance = CreateAndSetupEffect(effectPrefab, effectWorldPosition, duration);
			effectInstance.Play();
		}
	}
}

private bool AreEffectNamesEqual(string firstEffectName, string secondEffectName)
{
	return firstEffectName.Equals(secondEffectName, StringComparison.InvariantCultureIgnoreCase);
}

private OneShotEffect CreateAndSetupEffect(OneShotEffect effectPrefab, Vector3 effectWorldPosition, float? duration)
{
	var effectInstance = (OneShotEffect)Instantiate(effectPrefab, effectWorldPosition, Quaternion.identity);
	effectInstance.InitSelfDestruct(duration);
	effectInstance.transform.SetParent(transform);

	return effectInstance;
}

Overall, the method looks cleaner. When you read the implementation of “PlayEffect”, you can understand more quickly how it works without going into the nitty gritty details. By removing the extra responsibility of this method (instantiating and initializing the effect instance), we promoted the single responsibility principle of this method. Can we improve this code further? Sure. Should we? Not only should we, but we are obliged to do so because we are professionals, and we always check in code in better shape than we checked it out. Don’t take this too far though, as it’s more of a guideline than a rule, and I’m not inferring here that one should sit and improve every bit of code that he finds, but if you find a name for a method, for example, that suits better, you should change it.

Do you have ideas of how to improve this code further? Write it in the comments.