Sins Of Code Readability

In the course of my career, some people have occasionally asked me for advice on software development. It is frequently hard for me to explain my approach. Part of this is because any programmer’s approach is a constant evolution. Would I look back at my code from six months ago and consider it to be Good Code? Perhaps not. But why is that? Is it because I designed the code poorly? Or is it because I didn’t spend sufficient time on it? Or is it because I didn’t really understand the problem until I had solved it? The answer to this is usually: Yes. 😀

Writing code that passes as Good Code six months down the road is harder than it looks. I find it a good metric. But why use six months as a metric? Frequently at that point I have forgotten most of what I’ve written. And looking at it with fresh eyes I can better ascertain how good a job I did.

Easy To Read Code

Robert C. Martin, aka Uncle Bob, wrote:

Indeed, the ratio of time spent reading versus writing is well over 10 to 1. We are constantly reading old code as part of the effort to write new code. …[Therefore,] making it easy to read makes it easier to write.

Clean Code: A Handbook of Agile Software Craftsmanship

So above all, the most important aspect of writing code…is reading it. Does it make sense? Does the function name and parameters accurately describe it? Does it do exactly what it says without side effects?

So based on my experience, what are the characteristics of Good Code? I’d submit to you that Good Code is:

  • Code that is easy to read.
  • Code that can be updated easily.
  • Code that is well tested.

In this article we’ll tackle some big sins of code readability!

Sin #1 Favoring Cleverness over Clarity.

Probably one of the top enemies of readability is our own need to look cool. Perhaps we want to save a few cycles on the cpu. Perhaps we found a cool new design pattern or some syntactic sugar that we are excited about. But frequently optimization doesn’t matter. The syntactic sugar is unnecessary or complicated.

Dictionary<int, string> lookup = getLookup();
char key = (myChar | ' ');
if(lookup.TryGetValue(key, out var myStr))
{
	print('found it!');
}

It isn’t really obvious above that (myChar | ' ') is an old-school hack to lowercase a string. But hey now that you know it, doesn’t that sound really neat? Maybe I just blew your mind a bit. I guess I should feel pretty clever right now, but I don’t.

Sin #1a: Brevity is not clever-ity. 

var output = (p-A)*(D-C)/(B-A) + C;

Do you recognize the above function? If so, kudos! But most of you who aren’t writing shaders are probably scratching their heads. Do you recognize it now:

public float MapValueBetweenRanges(float x, float inputMin, 
float inputMax, float outputMin, float outputMax)
{
  return (x - inputRangeMin)*(outputMax - outputMin)/(inputMax - inputMin) + outputMin;
}

The great thing about these changes is even if you don’t recognize it, you can at least more easily reason about it.

Sin #2: Overly Generic Function Signatures

public void Attack();

One of the key criteria for readability is not the actual function itself, but the function’s signature. The function above is probably a function that will cause an attack to happen. But what does it really do? What are the inputs and outputs? Its overly genericized to be meaningless. Let’s look inside the function:

public void Attack()
{
  CurrentTarget.Health -= Damage;
}

Ok, so what we have found out is that Attack() is about subtracting Damage from our current target’s health. So maybe first we could rename this to DealDamage instead of Attack().

public void DealDamage();

But this changes only part of the signature. Perhaps we could look into defining the function’s inputs and outputs more clearly. This gives us a much clearer about what our dependencies are, and what data we are effecting.

public void DealDamage(Target enemy, int damage)
{
  Enemy.Health -= Damage;
}

This code is not only more readable, but also more versatile (which I’ll get into more in later posts).

Sin #3: Comments Inside Functions.

public int DoCombo(Target enemy, int damage,int requiredComboPoints, int comboPoints)
{
  // Only do the combo points if we have enough points.
  if(comboPoints >= requiredComboPoints)
  {
    DealDamage(enemy, damage);
	// subtract the combo points.
    return comboPoints - requiredComboPoints;
  }
  // If we don't do the combo, just return the player's current combo points.
  return comboPoints;
}

The above comments read a bit like ‘stream of consciousness’ commenting. The programmer is adding comments as he goes, providing notes to themself in the form of small thoughts that they may find useful in the future.

Frequently I find these notes to be completely redundant. What I want as a programmer is to not have to read a function to understand what it does. Again: inputs and outputs. The function works as a black box doing one thing that is obvious and well explained.

If I’m commenting inside the class, I ask myself ‘is this information that I need as a consumer of the function’ vs. ‘is this information I need as a developer’. If its the former it should most likely be outside the function.

Using function summaries and describing the inputs and outputs above the function provides more clarity. Sometimes this documentation is also redundant so only use as needed.

/// <summary>
/// Deals damage via a combo if the player has sufficient combo points.
/// </summary>
/// <param name="enemy">The unit we will deal damage to.</param>
/// <param name="damage">The amount of damage to deal.</param>
/// <param name="requiredComboPoints">If <paramref name="comboPoints"> are greater than or equal to this value,
/// then the combo will occur.</param>
/// <param name="comboPoints">The player's current combo points total</param>
/// <returns>The number of combo points the player has, regardless of whether the combo occurred.</returns>
public int TryDoCombo(Target enemy, int damage,int requiredComboPoints, int comboPoints)
{
  if(comboPoints >= requiredComboPoints)
  {
    DealDamage(enemy, damage);
    Return comboPoints - requiredComboPoints;
  }
  Return comboPoints;
}

Sin #3a : ‘Soft Code’ is a Soft Sin.

Soft code is a term I use to talk about code that isn’t ‘hardened’ yet. It’s still malleable, straight out of the forge, and needs to be hammered into shape. After a few iterations it is hardened, and hopefully in six months I can look back at it and call it Good Code.

But sometimes I ship soft code. Shipping is more important than perfection. And so is it possible to give all the code I write the love it needs to harden? In that case its pretty common to leave comments in for Future Me. Its a sin, but a soft one.

Self Documenting Code

Developers go back and forth on how much you should comment your code. I would argue that DealDamage() is self-documenting, meaning that it doesn’t need comments, it is clear what it does. But not just because of its name. It is concise, the inputs are clear, and so is how the inputs are modified (aka the ‘side effects’).

Creating self-documenting code is the best method of commenting your code. And because its self documenting that means we know what it does when we use it:

unit.DealDamage(enemy, unit.weapon.damage);
unit.PlayVFX(unit.vfx, unit.transform);

Sin #4: Comments Instead of Self-Documentation.

If you feel that you need to add comments to a function, an alternative is to create a separate function that will then be self documenting instead. This function can have summaries if need be and clear responsiibilities.

public int TryDoCombo(Target enemy, int damage,int requiredComboPoints, int comboPoints)
{
  if(CanDoCombo(comboPoints, requiredComboPoints)
  {
    DealDamage(enemy, damage);
    Return comboPoints - requiredComboPoints;
  }
  Return comboPoints;
}

private bool CanDoCombo(int comboPoints, int requiredComboPoints)
{
	return comboPoints >= requiredComboPoints;
}

Sin #5: Failing The Squint Test

I have saved the greatest sin for last. In my opinion, the worst offender for readability is the Squint Test. Look at the below code. Then squint at it and blur it out.

The shape of the code becomes more evident when you squint:

Do you notice the shape of the code? This kind of constant undulating shape of nested conditionals is a sign of code that is hard to read. The above code is from the Gilded Rose Kata. The Squint test is generally attributed to the All The Little Things talk by Sandi Metz. Now lets look at an improved version from the talk (SPOILERS! I recommend you stop reading this now and check out Sandi’s talk! :D)

The flow of this code is much more obvious. Its direct and to the point. It passes the squint test.

But…why is this better code? Sandi’s talk is focused on making smaller things: breaking down large code into manageable chunks. This allows us to break up conditional branches into smaller, linear flows that our human brains handle much better. Writing readable code isn’t just about naming your variables better, or better comments. But also how you code. Which I’ll get into more in later posts.

In Summary

I hope you found this post interesting! I will be elaborating more as I write more. And to summarize, here are the sins in concise list:

  • Sin #1 Favoring Cleverness over Clarity.
    • Sin #1a: Brevity is not clever-ity.
  • Sin #2: Overly Generic Function Signatures.
  • Sin #3: Comments Inside Functions.
    • Sin #3a: Soft Code is a Soft Sin.
  • Sin #4: Comments Instead of Self-Documentation.
  • Sin #5: Failing The Squint Test.