Every week I host at least one Code review meeting. When my team first started opening pull requests and reviewing code, it felt like a chore. It began well but began to turn into one person nagging the rest to finish their reviews. As more and more pull requests started to come in, our team decided to host weekly code review meetings instead.
In these meetings, we get together a small group of different engineers and data scientists to look over any open pull requests (PRs) ranging from analytics to tool development. This group includes the original developer opening the pull request, along with a few senior members. In the meeting, we read through the code, comment on the work, discuss the design decisions, and determine the next steps. Have these reviews gone smoothly? In the beginning, it was a bit rocky, but through a regular cadence, it has become a much smoother process. With that, I want to discuss two critical areas of code reviews we typically review and what I have learned from them since I started hosting these reviews. These areas are code ownership and design decisions.
Code Ownership
Let’s first discuss code ownership and how you can approach your code changes to someone else’s code.
- Code belongs to the team and not one individual.
Code ownership can be a hard conversation to have. I believe in the idea of collective ownership in which the code belongs to the team, and they share ownership over that code. The Agile Alliance defines collective code ownership as
…the explicit convention that "every" team member is not only allowed, but in fact has a positive duty, to make changes to "any" code file as necessary: either to complete a development task, to repair a defect, or even to improve the code’s overall structure.
Under this code development model, you reduce the risk of a developer being absent, which can stall or slow work. Collective ownership encourages each developer to feel responsible for the quality of the code base as a whole and be able to participate in updating or adding to different parts of the codebase.
On my team, code can change hands often as people work on different code base aspects during various projects. Code ownership can often come up in a Code Review when one developer has altered the code that another developer previously wrote. I am not saying you cannot have a primary developer who works on that code more often than others, but others should feel that they can contribute to the work and have open discussions on how to improve it.
2. Code reviews are not about you; they are about the code.
In the code reviews I host, I invite a core group of reviewers alongside any developer whose PR we will be discussing. The critical thing to remember in these discussions is the comments are about your code, not you. Developers are trying to make sure the code is functionally doing as intended and that no issues enter the codebase. It is helpful to be open-minded when approaching these discussions as it can introduce some great learning opportunities.
As you are writing comments on a pull request, think to yourself:
- Is this comment providing constructive criticism on a technique and why another may be better? Or is it pointing out a logical error?
- Does the comment teach the other developer something?
- Is this is a nit-picky comment based on style or preference, but not functionality?
If you answered yes to questions 1 or 2, then provide insight into how the developer can improve their code. If you answered yes to question 3, consider why you are making the suggestion and determine if the comment should be made or not. Personal preferences of coding style or techniques that do not improve the code’s functionality are ones I tend to leave out. The exception to this are changes that would improve readability.
3. Having review meetings with more than one other developer can ease tensions on code ownership.
Having multiple people in a code review group can also ease tensions between the original developer who created the code and the new developer who alerts the code. Our team has specifically called out working agreements for code reviews that detail how such reviews will be held and resolves any disputes. Since the introduction of the code review meetings and working agreements, there have been no significant disputes on code changes. These working agreements outline a coding standard the team follows, instructions for creating a pull request and holding a code review, and a detailed conflict management guide that explains what happens in the event of a dispute.
4. Understand the difference between a necessary change and a suggestion.
At these meetings, developers are open to discussing any changes in the code. Some changes are standards we have outlined in our working agreements, such as what libraries we use or naming conventions to follow. If it is not explicitly called out as a standard or explained, ask if it is a necessary change or a suggestion.
For these discussions, it is beneficial to distinguish between what requirements to meet for the code to pass a code review and what comments are suggestions for future thought or implementation. It is good to clarify your action items before leaving the meeting to understand your next steps. I find it best to mark some comments as Won’t Fix if they are suggestions or considerations for later.
What are your thoughts on code ownership? Does your team have a set of standards they follow for code changes?
Design Decisions
Another aspect of code review meetings that I have found beneficial is that these meetings allow for robust discussions on design decisions and future use case opportunities.
5. Talking through your code design can help others understand the decisions you made.
Another reason we have code review meetings is they provide a platform where developers can walk through their code and discuss why they developed the code in this way. This has been beneficial not only for the developer but also for others on the team to understand the code at a deeper level. These conversations allow for discussions around why the developer wrote the code that way and what use cases they considered. Through these discussions, developers’ design decisions can be made clear, and others can advocate more clearly for changes. Meeting with other developers on the code structure can aid in learning best practices around code architecture.
In the article Making Everyday Decisions Easier, Oleksii Fedorov made a good point on how code reviews helped his team:
Code review helped us to give each other feedback on micro design decisions that we were making in the code. For example, readability of a name, whether or not we should split a class in two, too large functions, and so on.
This feedback is necessary as it aids in readability and design decisions and gives insight into best practices.
6. Others may offer new use cases you didn’t consider for your functions or classes.
Another aspect of these design discussions that have sparked interesting engagements is new use cases. Commonly, developers come into a meeting with their one or two use cases for the developed code. For example, someone comes into a code review with a function that calculates a specific value. The developer may not be aware of use cases outside of their own, but other developers can discuss this during a code review. These use cases may determine where the code is located within the library. Rolling code to standard utility files or classes has been a common occurrence in our code review meetings. Functions are moved as they provide use for more than one analytic or could give new analytical concepts in the future. This practice has helped developers think about how they can generalize their functions for use by others.
I recently had a code review where I explained a function that I created to clean a specific Data type. I designed the function for one use case that utilized this data for developing analytics. It became clear through discussions with other developers that other use cases for this function would require some additional input parameters. Talking through the design decisions, as in point 5, helped showcase other use cases for the function, as in point 6. The code review gave me concrete feedback on adapting my function to work with my use cases and others that I had initially not considered.
7. Defining a set of working agreements around code decisions can aid in making recommendations.
Remember the working agreements I mentioned earlier? These agreements were not just for handling code reviews and difficult decisions but also for how to style code. My team maintains a style guide for standard libraries used, naming conventions, and more, such as commenting styles and documentation. Along with the coding standard, we have a linter that checks the code when a pull request is opened. Having this set of working agreements aids in making recommendations during code reviews as it gives a standard the team follows. Therefore, if we use a library for a specific function, such as checking for null values in data using numpy, everyone should use the same library function to keep the code’s consistency.
Have code reviews helped you see other use cases for your code or brought out discussions on the original design decisions?
Final Thoughts
As you traverse your code reviews:
- Understand these reviews are not meant to be personal. Focus the reviews on the quality and design of the code. Reviewers are trying to give feedback that will aid you in your work and improve its quality.
- Developing an ownership model on your team will aid in understanding who is primarily responsible for the code. Ownership will also help in identifying how you accept changes and modifications to code.
- Not everyone will agree with how to structure the code. Listen to the other person’s side and then explain your reasoning. Work towards a resolution.
What have you found most beneficial during your code reviews? Have you learned something interesting about yourself or your code through them?
Additional Reading
- "What is Collective Code Ownership~searchTerm~’~sort~false~sortDirection~’asc~page~1))" by Agile Alliance
- "Code Ownership and Software Quality" by Microsoft
- "Design Decisions" by OMAR ELGABRY
- "Software Developer’s Guid to Making Everyday Decisions Easier" by Oleksii Fedorov
- "Software Architecture as a Set of Architectural Design Decisions" by Anton Jansen and Jan Bosch
If you would like to read more, check out some of my other articles below!
Top 8 Most Common Questions I Get Asked as a Data Scientist
Effective Data Science Requires Strong Collaboration with Data Engineering
Agile in Data Science: How can Scrum Work Effectively for Your Team?
