Spiria logo.

Simply Code Reviews

November 20, 2015.

In any project, one of the many accepted tools in software development is the practice of code reviews. The most astounding thing about this practice is that nobody really knows why code reviews are important. Many people resign themselves to performing them just because it is part of the process.

What For?

I, for one, like to know why I am doing something. If there is no valuable reason or logical explanation for an operation, I will eagerly propose that we remove it from the process… If it serves no purpose, it is a waste of time and energy.

Reason #1 — Finding Bugs

All agree that finding a bug early in the development process is infinitely cheaper than finding it during QA, UAT or, worst of all, during production. A well-performed code review may help uncover things which, if addressed in a timely manner, will save time, money and energy.

Examples:

  • Simple mistakes & typos;
  • Missing/forgotten use-case;
  • Inconspicuous issues with implementation (buffer overflow, security flaws, threading issues, etc.).

Reason #2 — Sharing Knowledge

On a software team, we cannot all be on an [insert name of your favourite sport athlete] level of awesomeness in all areas of the software. We all have our areas of expertise, our sandbox, our walled garden of awesomeness. But in order to grow as a team, we need to leave our comfort zone and see how other people do things: learn from them and their code.

A code review is a great way to share knowledge. It allows our colleagues to see a part of the project that they would have not otherwise worked on, learn new tricks of the trade, and eventually take their technical and business knowledge a little further, thus bringing them closer to awesomeness.

It is also a great way to prevent the “vacation” problem, where a whole project is stalled because only one person, who is on vacation, knows how Module Omega works, and nobody else wants to fiddle with it. I, for one, as someone who has been called in during my vacation for an emergency bug fix, like to enjoy my vacation undisturbed.

Reason #3 — Continuity

New technology, new practices, new coding standards and design patterns keep getting invented every day and are theoretically good things to learn and integrate in a software project. But within the life cycle of a project, homogeneity should be preferred in order to keep the project scope under control.

Code reviews are the final safeguard against unbridled software entropy, because any new standards, practices, patterns or technology should be adopted by the entire team, not introduced unilaterally by a rogue developer, or experimented with just for the fun of it.

Etiquette

There is etiquette to good code reviewing, so that, regardless of the tool you use, the code review is a pleasant exercise for all parties involved. And in case you’re wondering, there are plenty of tools out there for code reviews, but that would be the subject of another article.

Etiquette Rule #1 — Objectivity

Your team probably has a Coding Standard, with a pre-defined coding style, a set of good practices, etc.

Nothing is more antagonizing than receiving comments in a code review that are subjective to the reviewer, and reflect his or her personal taste in coding style, practices or patterns — not the team’s coding style, practices or patterns.

Example of a subjective comment: “These 5 lines of code could be rewritten in a single line with chaining.”

This is highly subjective, and no two programmers will agree on which form is better. As a reviewer, it is your duty to suppress the urge to enforce your personal preference or style in such a controversial area. If you (or the reviewee) feel that this is important, then summon the team into a room, expose the issue, agree that it is actually a problem, then brainstorm on an acceptable solution, debate it on its actual merits (this may trigger spikes, it is an inherent hazard of meetings after all), and finally make the decision as a team. Perhaps the decision will not be to your personal liking, but at least it will have been made by the team, which is infinitely better than the alternative (i.e.: eternal nitpicking).

Etiquette Rule #2 — Provide Good Feedback

When you see something neat and well crafted, write it up in the code review; everybody enjoys being recognized for his or her good work.

Alternatively, when you see something that could be improved or done more efficiently, write it down in the code review, explaining how and why it could be improved, and communicate with the reviewee on alternative designs or solutions. The goal is not to fix the code for them, but to give them enough information so that they can improve the code themselves and learn from the experience (and from you!). Finally, know that the resulting solution may not be exactly what you would have done, and that’s OK: as long as it meets the business requirements and the coding standards of the team, it will be fine.

Case study:

You see a code review with a unit test invoking the bitly.com url-shortening service every time it runs.

Bad feedback:

“This is wrong, this should not be a unit test.”

Good feedback:

“This test will fail:

  • if the service is unavailable;
  • if the daily quota is exceeded;
  • if the API changes;
  • if the API token expires;
  • if the network is down.”

“If we used the mocking library to mock all calls to that service, then our test would be foolproof.”

“Furthermore, if we need to test the actual integration with Bitly, it should be added in the integration test suite.”

Etiquette Rule #3 — Share Your Stories

We all have our own experiences working on different projects and with various technologies, and particularly interesting past experiences can be conveyed through a code review, and get conversation going. Added bonus: Everybody enjoys a good story!

Case study:

You see a code review with a unit test that starts a thread, sleeps for 3 seconds, and then checks the results from the thread.

Here is my story:

“In a previous project, we had tests like that, and we had our fair share of false failures when the build machine was particularly slow, which basically destroyed our confidence in our test suite. After that, we refactored our test suite to be deterministic as much as possible, and not to depend on the speed of the build machine”

Final Words

When teams do code reviews right, they create more valuable software, with fewer bugs (or even no bugs?), and they grow both as individuals and as a team.