process

Should I Reject a Code Review for Typos?

TL;DR: Yes!

Code reviews are essential to good software development. In a code review, peers read each others’ code and vote to approve or reject the changes before committing them to the main code base. Code reviews provide a platform for constructive feedback, accountability, and even learning opportunities. To make them effective, a team must establish best practices – not only for the code itself, but also for the review process. Good guides can be found here, here, and here for reference. Some rules, such as “no personal attacks” and “focus only on the changes at hand,” are universally agreeable. Other rules, however, can cause controversy.

One such controversial rule is the title of this blog post: Should a code review be rejected for typos? I use the word “typos” here to broadly include any sort of typographical shortcoming: misspellings, incorrect grammar, poor formatting, and even improper spacing. For example, a variable named somehting would be a typo.

There are valid reasons why not to reject code reviews despite typos. The code itself will still compile and run, so long as the use of typo’ed identifiers is consistent. Requiring corrections takes extra time, which in business costs more money. Authors may also take offense, especially if English (or the language of dialogue) is their second language.

Nevertheless, I strongly believe that yes, code reviews should be rejected for typos. Below are five reasons why:

It corrects carelessness. Typos mean carelessness. Mistakes are bound to happen, but pervasive typos indicate a deeper, systemic problem. Reviews are a measure of accountability between peer engineers to prevent carelessness. Being tough on small things encourages engineers to straighten-up on all things.

It prevents future frustration. People expect things to be spelled and formatted the right way. Compiler error messages are often cryptic and may not intuitively point to typos. Imagine trying to call the do_stuff method, only to discover that the original method was named do_stuf after an hour of hair-pulling, fist-banging, and cursing at the screen. Frustrating is especially acute when BDD Gherkin steps have typos. Allowing typos to be committed to the code base increases the chances of this type of frustration.

It improves readability. Typos and poor formatting are distracting. They make it harder to read code. For example, I remember once reading a Perl source file in which every single line had an arbitrary number of indent spaces. It was impossible to visually align function bodies, if statements, and loops. I had to reformat it before I could work on it.

It boosts confidence in the code and in the team. Imagine if you saw typos in this blog post. Each typo you find would lower your confidence in my writing skills. The same is true for software: as a reviewer, when I see typos, I lose confidence in the quality of the code and in the author’s skills because I see carelessness. Eliminating typos not only makes code better, but it also makes people think better of the code.

It reinforces high standards. Quality is not limited to functionality. Poorly-written code may run correctly, but it will not be maintainable. Upholding high standards in code review will result in overall better code output. Letting small things slip through will, over time, atrophy a code base.

If “reject” sounds like a harsh word, it may be beneficial for a review process to indicate the severity of feedback points. For example, broken code could be “critical”, while typos could be “minor.” Nevertheless, typos should not be committed to the main code base, and thus their code reviews should be rejected.

The Behavior-Driven Three Amigos

Recently, my manager said to me, “Andy, your BDD documentation looks great, but could you please mention The Three Amigos?” A brief flash of panic came over me – I had never heard of “The Three Amigos” before. My immediate thought was the 1986 film of the same name or the Disney film The Three Caballeros. After a little research, I knew exactly who they were; I just didn’t know they had a name.

Who are “The Three Amigos”?

The Three Amigos” refers to a meeting of the minds of the three primary roles involved in producing software:

  1. Business – Often named the “business analyst” (BA) or “product owner” (PO), the business role provides what problem must be solved. They provide requirements for the solution. Typically, the business role is non-technical.
  2. Development – The developer role provides how the solution to the problem will be implemented. They build the software and must be very technical.
  3. Testing – The testing role, sometimes named “quality assurance” (QA), verifies that the delivered software product works correctly. They also try to find defects. The tester role must be somewhat technical.

During software development, The Three Amigos should meet regularly to discuss how the product will be developed. It is a shift left practice to avoid misunderstandings (like a game of telephone), thus improving quality and avoiding missed deadlines. The discussions should include only the individuals who will actually work on the specific deliverable, not the whole team.

While The Three Amigos seems most popular in Agile, it can be applied to any software development process. Some (here and here) advocate regularly scheduled formal meetings. Others (here and here) interpret it as an attitude instead of a process, in which the roles continuously collaborate. Regardless of implementation, The Three Amigos need to touch base before development begins.

Applying BDD

The Three Amigos fits perfectly into behavior-driven development, especially as part of BDD with Agile. Behavior scenarios are meant to foster collaboration between technical and non-technical roles because they are simple, high-level, and written in plain language. Given-When-Then provides a common format for discussion.

Ideally, when The Three Amigos meet during grooming and planning, they would formalize acceptance criteria as Gherkin features. Those feature files are then used directly by the developer for direction and the tester for automation. They act like a receipt of purchase for the business role – the feature file says, “This is what you ordered.”

A great technique for Three Amigos collaboration is Example Mapping – it efficiently identifies rules for acceptance criteria, behavior examples, and open questions. Examples can easily be turned into Gherkin scenarios either during or after the meeting.

Since BDD relies on feature files as artifacts, The Behavior-Driven Three Amigos must be more than just an attitude. The point of the collaboration is to produce feature files early for process efficiency. Less formal meetings could quickly devolve into all-talk-no-action.

Don’t Presume Anything

Don’t presume that the three roles will naturally collaborate on their own. I’ve seen teams in which the testers don’t participate in planning. I’ve also seen organizations in which automation engineers don’t help to write the test cases that they need to automate! Developers often abdicate responsibility for testing considerations, because “that’s QA’s job.” And, specifically for BDD, I’ve noticed that product owners resist writing acceptance criteria in Gherkin because they think it is too technical and beyond their role.

The Three Amigos exists as a named practice because collaboration between roles does not always happen. It is an accountability measure. Remember, the ultimate purpose for The Three Amigos is higher quality in both the product and the process. Nobody wants more meetings on their calendar, but everyone can agree that quality is necessary.