Ben Summers’ blog

Thinking before committing

I’ve found that good quality code can only be written by thinking very carefully before you start writing it. I’ve also found that thinking about it afterwards is just as important.

Before typing fossil commit, I make a point of thinking carefully about the code I’ve just written, roughly like a mini code review. It’s a very useful habit to pause for 10 minutes or so, and in doing so, I often correct small issues, reduce the size and complexity of code, and make it easier to read.

Here’s my pre-commit checklist:

 

Correctness

  • Have I verified that the code meets the requirements?
  • What edge cases have I forgotten?
  • Have I included only the necessary files and removed redundant files? Are the ignore files up-to-date?
  • Are the comments around the code still accurate?
  • Have I removed all the debugging code?
  • Are there any glaring performance issues?

It’s pretty obvious that the changes must do what they’re supposed to do. The testing during development and the automated tests should have covered everything, but things like accurate comments can’t be checked by tests. You need to check these manually, and it never hurts to run through your code in your head to make sure you haven’t missed anything.

Profiling and optimisation are different steps to writing code, so the last point on performance is more to make sure there’s nothing stupid, rather than checking that the code is optimal.

 

Neatness

  • Have I followed the applicable style guide?
  • Is everything an actual change to logic, with no “whitespace only” changes?
  • Is the diff clear about what’s changed?
  • Does the code read nicely, explaining the intent in the way it’s written?
  • Could I remove any comments, either because they’re unnecessary or by writing the code in a more literate style?
  • Does the commit cover too many changes in one go, and could it be split up?
  • Does the proposed commit message explain what was changed, and why?

I think it’s important that the history of the project in your source control is easy to read and understand, and be free from messy diffs and unnecessary changes.

This isn’t just because of an aesthetic pride in my work. After you’ve written some code, you can be blind to the details because you’ve read it so many times, and just skip over the familiar symbols. I find that by looking at it in detail for another purpose, in this case neatness, I often see things I’d otherwise have missed.

As a bonus, by restricting your changes to real functional changes, you make it easier to merge branches.

 

Complexity

  • Is there a simpler way to write this code?
  • Is interdependent functionality contained within one logical module, with a clear boundary?

Complexity is the enemy of working systems. Not only does it make it easier for bugs to lie undetected, but it makes it harder to understand the code later, storing up problems for the future.

When you’ve been trying hard to solve a problem, the first solution may not be the best. Maybe there’s a simpler way of writing it?

 

Security

  • Is any untrusted information used without sanitising or checking for validity?
  • If information is disclosed or actions are taken, are there checks that the user is authorised?
  • Is it hard to use this code in a way which is insecure?
  • Is it possible to abuse this code in any way?
  • Could this code be open sourced?

You should always be taking a security first approach to writing code. Security flaws are the worst kind of bug, and even though the requirements don’t say “must be secure”, it’s hard to argue that code which isn’t secure can be said to be correct.

Especially with web applications, you need to trace all the data coming into your code, and see if you’re doing anything with it that’s dangerous before you’ve checked that it’s trustworthy. The only thing that you can trust is code running on the server you control. Everything else must be assumed to be compromised and actively trying to subvert your security.

You should also consider text in the database to be untrusted, as it probably came from a user at some point.

The point about open sourcing may seem a little out of place in a section about security, but it’s a very useful thought experiment. It makes sure you don’t include authentication secrets or customer information in the code, and that you’re not relying on security by obscurity.

 

Tests

  • How is this tested?
  • Can I simplify the tests, and by doing so, simplify the code?
  • Is the code in the test clean and easy to read, suggesting that the new code has a sensible and unambiguous API?
  • Are all the edge cases tested?

While automated tests are obviously preferable, it’s possible you may be testing your code by other means. Complex UI code in a web browser might be more efficiently tested by a human running through a test script before each release. Or maybe it’s slightly experimental? But in any case, it’s worth making a conscious decision about how to make sure there’s a test.

I also find it’s interesting to review the test code to see if you’ve written the code in a clean manner. If the tests are fiddly, it may mean that the code has a problematic API, or isn’t modular enough.

 

User interface

  • Is it a good UI?
  • What can I remove from the user interface?
  • Does it handle errors elegantly?

Finally, if there’s any user interface involved, it’s good to take a fresh look at it, and make sure it’s actually usable. I like to look at every element in the UI and ask if it could be removed.

 

A useful pause

This isn’t intended to be a formal checklist. As with anything, it needs to be applied intelligently rather than blindly. Some changes are too small for it to be useful.

For me, the most important thing is to reinforce a habit of pausing, reading the output of fossil|svn|git|hg diff, stopping to think about it, and only when I’m certain it’s right, typing that commit command.

 

I’m curious about your workflow. Do you do anything like this? Is there anything else I should have on my list?

 

 

COMMENTS

blog comments powered by Disqus

 

Hello, I’m Ben.

I’m the Technical Director of Haplo Services, an open source platform for information management.

 

About this blog

 

Twitter: @bensummers

 

Subscribe

Jobs at Haplo
Come and work with me!