Best practices on keeping your codebase clean

Krzysztof Szromek CEO
Productivity over time for bad quality code
 

Writing bad code is easy. You just make things work and move on to next task, not worrying about the code quality. But what you don’t realize is that it’s going to become unmanageable very quickly — to others at first, eventually to you as well. That’s a good reason to try to write good code.

What is a good code?

It’s hard to define what is good code, but here’s the definition from Michael Feathers which seems to explain it pretty well.

Clean code always looks like it was written by someone who cares. There is nothing obvious that you can do to make it better. All of those things were thought about by the code’s author, and if you try to imagine improvements, you are led back to where you are, sitting in appreciation of the code someone left for you — code written by someone who cared deeply about the craft.

So how do we write a good code?

Well, we start with a test.

It’s a waste of time to write test for everything, I’ll just add integration tests afterwards — you may say. But the test is a base for ensuring that you didn’t break your code during refactoring.

Refactoring? I know what code I want to write, why would I refactor it afterwards? — writing great code upfront is hard and not really efficient. Would you rather spend an hour developing great module just to find out that it’s not a great concept after all after seeing it in action, or rather make it work in 15 minutes and then spend some more time on refactoring if it indeed prove itself useful? Not to mention that small improvements to an existing code are easy. Especially when test ensures us that the code is still working properly.

TDD cycle

TDD cycle

 

Meaningful naming

After reading a function or variable name we should know what exactly is it responsible for. Consider following example

def getThem
  list1 = []
  theList.each do |x|
    if x[0] == 4
      list1 << x
    end
  end
  list1
end

We see that function returns some filtered list, but what kind of list does it apply to or what is the condition of filter is not clear at all. Now let’s look at the same function with meaningful names.

def getFlaggedCells
  flaggedCells = []
  gameBoard.each do |cell|
    if cell.isFlagged()
      flaggedCells << cell
    end
  end
  flaggedCells
end

Besides meaningful names we extracted logic that was not this function main concern — checking whether cell is flagged — to cell object.

Another thing to keep in mind is to avoid shortcuts and pronounceable names. genTmstmp is a few characters shorter than generateTimestamp but for someone who has to skim through a bunch of code that looks like this it may make things hard.

Functions

Functions should do one thing. They should do it well. They should do it only.

The rule actually means that a function should only do those things that are on one level of abstraction below the stated named of the function — i.e., it should not include calls to functions at other levels of abstraction, so the essential/high level concepts should not be mixed with lower-level details.

Another way to know that a function is “doing more than one thing” is if you can extract another function from it with a name that is meaningful.

“They should do it only” part means that not only they should do one thing, but they should have no side effects, as these are really easy to omit and can be hard to debug.

Comments

The best code has none. The goal is to explain yourself in code. Code is always timely, where comments can get out of date, can get misplaced and the compiler won’t complain. If you feel you need to add a comment then you most probably should improve the code.

Formatting

Formatting guidelines come and go, but there’s one rule that is timely and ensures that the codebase you work on will be clean and readable:

Every programmer has his own favorite formatting rules but if he works in a team then the team rules.

Exceptions handling

If you’re afraid of throwing exceptions and prefer to return example error codes instead, think twice.

if (deletePage(page) == E_OK) {
  if (registry.deleteReference(page.name) == E_OK) {
    if (configKeys.deleteKey(page.name.makeKey()) == E_OK){
      logger.log("page deleted");
    } else {
      logger.log("configKey not deleted");
    }
  } else {
    logger.log("deleteReference from registry failed");
  }
} else {
  logger.log("delete failed");
  return E_ERROR;
}

Noticed the level of indentation we reached here to catch returned errors?
The exception-based alternative says it all.

try {
  deletePage(page);
  registry.deleteReference(page.name);
  configKeys.deleteKey(page.name.makeKey());
}
catch (Exception e) {
  logger.log(e.getMessage());
}

Tests

No matter what type of tests you prefer (hopefully it’s not integration tests), keep in mind the F.I.R.S.T. rule set:

  • Fast — you won’t run tests that run for 5 minutes frequently enough to catch errors on time.
  • Isolated — each test should have a single reason to fail.
  • Repeatable — you should obtain the same results every time you run a test.
  • Self-validating — you should not need to analyze the test result to know whether it has passed or not. PASS or FAIL should be the result.
  • Thorough and Timely — keep’em up to date.
  •  

Summary + Boy scout rule

This article is kind of a list of good rules and you may feel that some of these are overkills or just useless. I thought so about some of these too, until I read the Clean Code book by Robert C. Martins, known as Uncle Bob and I recommend you doing the same.

There’s just one more rule I think is worth mentioning, it’s Boy scout role and it says:

Always leave the campground cleaner than you found it.

All it means that you should always try to update at least a little piece of code when you do (change) something, which by small incremental changes to the codebase lead to a better code.

I hope your code will be at least a little better from now on 😉

References

https://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882