Pro Tip
As someone who's used many different code review tools over the years, I can confidently say that Reviewable stands out as one of the best. It's a game-changer that makes code review and feedback easier than ever before. If you believe in using the right tool for the job, I strongly recommend giving it a try. I've been a true fan for five years now, and I'm not affiliated with Reviewable in any way. Give it a try and see for yourself!
About Our Product: Where All the Code Reviews Happen
I work at Kaltura, a leading software company that offers a comprehensive video platform for streaming, uploading, and publishing videos across various platforms and devices. As part of my role, I'm a member of the team developing Kaltura Meetings, a modern application that facilitates live online classes, webinars, and meetings with interactive features. With its complex architecture, and numerous features and integrations, effective code reviews are crucial to ensure the success of our product
Is the Main Purpose of the Code Review Assignee to Approve Code?
Everyone loves the feeling of writing LGTM ("Looks good to me") on a teammate's code review. It shows that their opinion matters because the pull request can be approved and merged thanks to their cooperation.
Code reviews are a wonderful opportunity for developers to provide feedback and influence their teammates' coding styles and implementations, but that's only if the reviewers know how to provide constructive feedback. Sadly, some reviewers tend to dwell on insignificant details like personal coding preferences and can hold up the entire review process. They act like perfectionists, and it can take an eternity for them to be satisfied.
Some people might feel uncomfortable with my statements above, and they should because it is easy to lose sight of the reason people conduct code reviews.
The Importance of Conducting Effective Code Reviews
Conducting a good code review is not about LGTMing someone work. It is also not about making your teammate's code look similar to how you would write it. It is about highlighting some areas in the code that are not aligned with the requirements, might lead to undesired runtime behaviors, impact performance and stability, or introduce security holes.
The Role of Design in Code Reviews
Effective code reviews require a well-defined feature design that has been thoroughly reviewed and approved before the code review stage. If design flaws are discovered during code review, it may indicate that the team missed something earlier in the development process. It's important to ensure that the feature design is sound before moving on to the code review stage to prevent potential delays and issues.
Although technical design meetings are a popular way to approve a design, I usually don't find them necessary. Often, the design simply mirrors the application's existing architecture and those meetings are not needed. When extending the architecture or suggesting a new approach, I usually recommend on a brainstorming sessions and short cycles interactions until the design is approved. When conducting the code review the design should be already known to all the reviewers.
Developers are human and might make design decisions that need to be changed during the code review process, but this should be the exception rather than the norm. As for when to expect design flaws during code review, it's important to consider the developer's level of experience. Beginners or newcomers to the team might be less familiar with the application's architecture and design. It's important to remember that the code review process is a collaborative effort, and developers should strive to give constructive feedback while considering the experience level of their peers.
A Step-by-Step Guide to Conducting Effective Code Reviews
Every developer has their own preferred method of conducting code reviews, and in my case, the following framework has led to more productive outcomes.
Step 1 - Understand the Requirements
The reviewer should start by reading the requirements and making sure they make sense. Understanding the needs will lead to more constructive comments. While reading the requirements, it is the time to put the rigorous glasses, ask questions, and ensure that the requirements are complete, clear, and consistent with the existing behaviors of the application.
Step 2 - Challenge the Application
After confirming the requirements, it's time to dig deeper and get hands-on with the application. As a developer, I recommend putting on your curious glasses, rolling up your sleeves, and playing with the application. To ensure code quality, I suggest reproducing at least 80-90% of the use cases covered in the requirements. This approach helps identify potential issues and ensures that the code is working as intended.
Some people might say this is the QA responsibility and not the a developer's one. As I see it, the developer is responsible for ensuring their deliveries are aligned with the requirements and avoiding apparent discrepancies. Any discrepancies you find between requirements and implementation that are resolved during the code review, will lead to higher delivery quality, shorten the time to production, have more positive interactions with QA people, and have fewer escaping bugs.
This step might require additional setup. There are many ways to automate it. Some people integrate their repository with Vercel or Netlify, services that build and serve your application directly from the pull request. Others are okay with pulling and playing with the repository on their local machine. Whatever option you choose, don't skip this step. Reading hundreds of lines of code and verifying them is for compilers. Humans manage better with visual UI. Playing with the application can help you identify issues that you might miss when just reading tens of files in a diff.
Step 3 - Prefer the red pill
Even if things look legit and working, they still need to function correctly under the hood. They could look okay but fail later due to a wrong application state or unexpected asynced events handling.
If the code review is on the backend parts, you should always have unit tests verifying the outcome and the process that led to that outcome. If you don't have unit tests covering the backend logic, please spend time with the team and discuss about ways to include them in the development process. Unit tests are crucial for backend areas. On the frontend side, it is harder to write unit tests due to the nature of the browser rendering framework, so the decision between cost and value is not as straightforward as in the backend.
If the pull request you are reviewing is on the frontend parts, there is a golden egg that, if used wisely, will make your current and future you much happier. I'm talking about having high logs coverage, letting you fully understand what happened. As frontend applications are based on async flows, and usually, many flows run in parallel, your logs should take it into consideration and reflect it in a way that will later let separate and focus on the events that interest you. This level of logging can help you trace through the logic of your application, find the root cause of issues faster, and reduce the time spent on debugging. While it may take extra effort to implement and maintain, the benefits of having high logs coverage far outweigh the costs. Moreover, it will be easier to maintain as the project scales up and more developers are involved in the project.
Having applicative logs on the frontend application is not heavily adopted by developers for various reasons. I will keep my thoughts private about why it is uncommon, but I encourage you to try it.
My team took it one step forward in our application, Kaltura Meetings. We are not logging things into the console or relying on the network browser devtool tab to try and understand what happens in our application. Instead, we have our own dev tools tailor-made to our needs. Those tools let us read, search, filter, group, and deduce things from the logs.
Assuming your application exposes logs, you should check them when you run the requirements to ensure the events raised and the app state changes reflect the desired outcome.
Step 4 - Review the code
If you follow the previous steps, you now better understand the feature, and your review will be much more productive. Try to focus more on performance, problematic implementations, and areas that are not following the guidelines or are not aligned with the style guides of the team.
Addressing the Most Common Concerns: Time Constraints and Deadlines
One of the most common arguments against code review is that it takes too much time and is not feasible within the constraints of a team's deadlines. While this argument is understandable, it's important to recognize that code review is a key aspect of ensuring high-quality code and avoiding costly errors in the long run. In fact, by catching issues early on, code review can save time and resources down the road. Additionally, there are many ways to streamline the code review process, such as setting clear guidelines and using automated tools to speed up the review process. Ultimately, while code review may require some upfront investment, the benefits it provides in terms of code quality and reliability make it a worthwhile practice for any development team.
Key takeaways from the code review process
If done correctly, code reviewing is essential to the development process. It leads to knowledge sharing, mutual responsibility across the team, and better deliveries. But if done incorrectly, it can lead to a negative atmosphere, lack of trust among the team, and poor deliveries.
I wrote an article about code reviewing two years ago, and I'm happy to see that my framework hasn't changed much over the years. If you're interested in reading more about code reviews, I recommend checking out my previous article, 'The Role of Code Review in the Overall Development Process'. I believe it's still relevant, and might add more insights to this article.
Enjoy the day,
Eran
Thanks to my teammates Anna Yurkevych, Tornike Menabde, and Stas Kogut for making the workdays interesting and meaningful.
Photo by Markus Winkler on Unsplash
Top comments (0)