Why Code Review?
Most developers will advocate for code reviews. In the world of OSS, code reviews are where changes happen and all bugs become shallow, right? Wrong! Developers don’t read through code and pick out complex bugs. You might find a small syntax error or misspelling, but bugs are not discovered by reading code. Computers execute code and computers are where bugs become real. So, if we should execute code to find bugs (aka run automated tests), lets talk about what code review is actually good for.
Outside of Open Source, code review helps teams communicate what is going on. As you work on features and get reviews, your team members get some visibility into what you’re working on. You can discuss coding patterns or techniques and learn something new or help someone else see something that person was missing. When there is an incident, the code reviews help to create context on what might have changed.
Code review also helps to coalesce towards defining what code should look like. As you review and talk about code, a team should eventually start writing code that looks similar. Aesthetic elements such as indentation, variable naming and logical flow preferences can become standardized, making everyone on the team more efficient at reading the code. This extends to other patterns such as testing and code organization as well.
Where code review goes wrong is when it becomes a blocker to getting things done. There are a few ways this can happen, but they all misconstrue the act of code review as a way for people to assert the code is correct.
The Stamp of Approval
Often times organizations (and some certifications) require that code is reviewed before being merge or pushed to the central repository. This sounds great on paper, but it is really only the beginning of the story. Often times this requirement puts developers in the position of putting a stamp of approval on something. That developer may not feel comfortable with that code and choose to withhold a “LGTM”.
The problem with holding back a stamp of approval is that no code will be released unless it fits the model and understanding of the reviewer. It is impossible to understand all the code, requirements and long term implementation of every code you ever review. Instead, you should assume the need for the code is real and give feedback that is actionable. If you are confused, ask questions and get answers. But don’t withhold a +1 simply because something doesn’t look like something you would do.
Asking for changes in a code review should be meaningful. Catching a typo or misspelling are obvious examples to ask for changes. Code formatting is something to ask for changes, but it really should only happen when new people join the team. Asking for tests is generally valuable as it pushes more validation where computers can catch bugs. The important thing to remember when requesting changes is that they are requests.
If a reviewer is demanding changes, the code review process get quickly move from productive, valuable changes to asynchronous pair programming. A review ends up with the reviewer rewriting the code to meet that persons vision of what the code should look like, often times without the context of author. Tests are one area where this can happen without much work. No one wants to say no to tests, so when the reviewer asks for testing around some feature it is hard to argue. The problem comes when code might have a deadline or the code is meant as a proof of concept. The test, while valuable, may be difficult to implement and provides a needless protection.
In order to avoid demanding changes, only review the code that exists and clarify any extra context. Organizational context is important, so ask for it. Reading a ticket or specification can greatly clarify the context of the code. If that context is missing, that is a great thing to ask for.
Wait for the Ideal Solution
Code review can go wrong when the reviewer feels a solution is not ideal. Again, context makes a difference here, but often times a reviewer will see a hack and avoid approval or ask for major changes with the intention that it is a better solution. Waiting for an ideal solution can be difficult to realize as it often takes the form of suggesting an upgrade or “simpler” code.
The problem with suggestions that are ideal is that it often strays from good software engineering practices by mixing a feature change with an API change. The code in question most likely is fixing a bug or adding a feature. You don’t want to fix the bug and along side some major refactoring. When refactoring code, you want to set up the refactoring, make the tests pass, update the code to use new code, make the tests pass, remove the old code, make the tests pass, remove any refactoring helpers, and make the tests pass. This pattern needs multiple code reviews alongside CI assertions to be done safely. Demanding the ideal solution makes this refactoring pattern near impossible.
Code review is a communication tool. While it can catch bugs, it should not be depended on to do so. Tests should catch bugs and tests should happen before and after a merge. When reviewing code
- ask questions
- get context
- understand what is happening
- don’t act as a gate
- trust the work needs to be done
- use the code review to help improve the result
I should also mention this is primarily targeted at code review within an organization, but it does have implications in Open Source projects as well. The difficulty in the Open Source world is often that people submit work that may not be a good fit and the use cases the code must meet are much larger and generic. Requiring an issue is a good way to get consensus on what is helpful and sets the tone for requesting changes to meet use cases. The big difference is that in your organization, deployment and support is part of the process, which means owning code can be a radically different beast than in an Open Source project. With that in mind, assuming an idea has been fleshed out before a code review begins, the process of reviewing the code can take the same tact with regard to working to make things better.