Table of Content
- Identify data flows inside algorithms
- Divide and Conquer
- Don't limit yourself to visible changes
- Justify your comments
- Question everything, respectfully
- Never underestimate the value of saying "Please" and "Thanks"
Code review is a common and essential practice at The Lifetime Value Company. Sometimes, due to the number of engineers, teams specializing in various domains, different programming languages and business-related context, doing code review can be a cumbersome and complex activity. Nevertheless, code review continues to be a fundamental tool at The Lifetime Value Company as it allows the team to maintain quality and consistency, mentor others and detect bugs before reaching production.
Even though programming languages may be different, there are a set of core principles and guidelines we can follow and apply. In this post, we will list some common ideas and principles that we have found useful in doing effective and timely code review.
For context, the engineering teams at The Lifetime Value Company work in the web and mobile domains, including online payments and public-record background reports from multiple sources, with a healthy dose of A/B testing going into our data-driven decision making process. You can read more about it here.
We don't fixate on a particular technology, and we use different programming languages in different levels of the pipeline for delivering a high-quality product to the customer. Also, we use Git as our source code versioning and tracking tool, so we will use Git-related terminology in this post as it is our primary tool for conducting code review.
The following are some techniques to apply when conducting effective code review:
Identify data flows inside algorithms
Any discrete piece of computer software can qualify as an algorithm. Basically, an algorithm is a set of steps that takes an input, executes the steps, and may or may not return an output. Under this broad definition, anything from implementing an OAuth 2-factor authentication to processing digital images or creating graphics in a customer-facing site can be defined as an algorithm (or a collection of algorithms). Therefore, understanding the fundamental nature of algorithms is key when conducting effective code review.
The number of layers of abstraction that software depends upon adds additional complexity. There is likely additional software running outside the context of the code you are reviewing and understanding it is important. Even further, layers of abstraction, rules, and behaviors vary from language to language. We won't dive into a specific language, rather, we'll list some language-agnostic actions that help identify data flows during code review.
- Identify the input of the code you're reviewing
Understand the parameters being passed into the code under review. Understanding means determining the data types, amount of inputs and the interaction of those inputs inside the code being reviewed. Data types help us understand edge cases, or scenarios where unexpected types might appear when the code is executing. This can be a key factor when doing code reviews with languages that are not strongly typed like Ruby or JavaScript, and where we depend upon tools like documentation or third-party libraries to comprehend the input of the code being modified.
- Be cognizant of side effects
Many languages have the concept of side effects. They typically occur when dealing with memory references inside a specific piece of code (e.g. a method), and code changes are being applied to the way these references act. That's why concepts like immutability are very important. Having mutable pieces of code accessed by multiple actors can result in nasty bugs that are hard to reproduce and fix. Always favor immutability, isolation, and encapsulation when conducting a code review. This is particularly critical with parallelism and concurrency, which we won't dive into here since it could be a topic of a blog post unto itself.
- Are there other entities using the code that is changing?
A classic example is changing the signature of a method (e.g., adding or deleting a parameter) and not updating it in all the places of the codebase where that method is being called. Also, in some languages, frameworks or simple libraries, there are special pieces of code that execute before or after a method (for instance, in Rails we have before_action hooks, and in frameworks like Angular or Ember we have lifecycle events).
- Be vigilant of where and how the output is used
If the code you've reviewed is returning results that will be used by other pieces of code living in different places, the reviewer needs to be sure that these interactions continue working. For example, if a critical method will change, it is a good idea to examine all the places where that method is being called and follow what is happening with the returned results.
Divide and Conquer
In computer science, "Divide and Conquer" is a well-known technique for approaching problems of all kinds. When pull requests are large and/or complex, conducting code review can be daunting. Sharing the workload with other reviewers and the developer themselves can help lessen the burden.
Asking the developer for a good README explaining all the features, as well as adding screenshots of the changes (where possible), provides context and isolates the different files to review. In addition, reviewing commit-by-commit can also help you to follow the developer's train of thought. In order for this to be effective, however, it is important for the developer to maintain a clear, concise and atomic line of commits.
Another approach we've found useful is to begin code review by examining the tests included in the pull request. Through the tests, you can determine:
Data flow as explained in the above.
Possible edge cases that the developer has not yet contemplated (e.g., what happens if I send a null in the second parameter of this function?).
If you are using a web framework such as Rails, Django or Laravel, which separate concerns and responsibilities, and you apply a general "model-view-controller" (MVC) pattern, it can be useful to see the tests that support each layer and identify single failure points (e.g., what happens if something crashes at the model level? Is the controller ready to handle this edge case?).
Once you've examined the tests, you will feel more comfortable reviewing the actual "meat" of the pull request and understanding the data flows you need to review.
Don't limit yourself to visible changes
It's very common to just examine the changes being made in a pull request or diff. However, this can prove insufficient. It is possible that a change can cause unexpected results in the places around or in different places of the codebase. As mentioned previously, be sure to not limit yourself to reviewing the code being changed and instead follow the data flow to be sure that the change is not affecting other areas.
Justify your comments
Some examples of code review comments that do not include justification include:
"Please refactor this using dependency injection instead of instantiating classes inside the constructor."
"Please avoid code duplication by moving this to a module and import the module in all places where the code duplication it's happening."
"We should avoid exceeding 80 characters per line here."
Comments should always include justification, syntax-related or otherwise. Without justification, we lose the opportunity to educate and mentor as part of the code review process. Why do we need to refactor this into a module? Because we can avoid duplicating code and all the pain that comes with it (multiple failure points, double work, etc). Make it clear to the other developer why you feel your suggestion is justified since this provides motivation and an opportunity for the developer to learn and grow.
Until this point, we've discussed the effective code review process as a one-way conversation, but it can (and should) be two ways. The reviewer may not always be right or understand the full context of a particular decision.
For example, when conducting code review on queries created with raw SQL, inside a framework that utilizes an Object Relational Mapper (ORM) like Active Record:
Q: "Why are we using raw SQL? Can we rewrite these queries to leverage ORM features? Using ORM makes the code more readable, and takes advantages of validations that might not be present at the database level …"
The other developer may have implemented it this way for a reason:
A: "I'm using raw SQL for performance reasons. I ran some benchmarking. With the ORM it took X milliseconds and with the raw SQL it took Y…"
This is a good example of effective two-way communication and how important justifying our opinions and actions can be during the code review process.
Question everything, respectfully
This is a core value at The Lifetime Value Company and we apply it to our code review process every day.
Often we see developers taking advantage of methods from a framework, a third party library or other external sources. Questioning whether they are applying those methods in the correct way is important, and we take the time to review the relevant documentation to confirm. For example, a developer working in a Rails application called the Active Record method update_attributes, which can be used to update the values of a row in a database table:
some_model.update_attribute(:some_attribute, 'some value')
some_model.save!
While many of us are not immediately familiar with the entirety of the Rails Active Record API, it is good practice to review the API documentation for the update_attribute method in this example. Upon review, we discovered that this method already calls save on the caller. We then asked the developer to remove the second, unnecessary call, and justified the request with the API documentation, thus saving an extra call to the database.
Never underestimate the value of saying "Please" and "Thanks"
Finally, you should always conduct code review in a professional and respectful manner. The person receiving the reviews is a human being with fears, motivations and emotions. They are on the same path as you, learning something new every day. Keeping code review positive fosters collaboration, and feedback is best provided in a constructive way. As a reviewer, you should always be willing to listen to the other person and understand their reasoning and opinions.
If The Lifetime Value Company sounds like a place you may be interested in working at, have a look at our careers page. Reach out to us if you would like join our team!
Top comments (2)
Great read!
We have written about the same subject a few times.
Here's something we hope it adds value - top 6 items for your code review check list - index below:
Thank you for reading!
Yours look pretty useful, thanks for sharing.