https://github.com/tuanlc

Making the code review process enjoyable

Keys takeaway

• Doing coding reviews is one of the fastest ways of spreading knowledge among team members
• Bring an open mind to learning and sharing to build a good culture in your team, making the code review process enjoyable for both reviewers and reviewees.
• Address problems rather than take them personally
• Answer questions Why? What? and When? before looking at How?
• Always be prepared to express gratitude or offer praise when you gain new insights or come across exemplary work.

What probably went wrong?

When you do a code review, what do you expect? Are they:

• Find bugs
• Spot violation of coding conventions
• Optimize performance
• Shortest lines of code
• Making code reusable

Yeah, they are good expectations but they aren’t sufficient. Sometimes, they end up with endless debate without a clear outcome of the pull requests and the feedback.

Have you fallen into a debate about using the proper method for iteration? Trying to optimize the time complexity from O(n*m) to O(n) without caring about the fact that there are just a few items in the iterating array.

Have you tried to correct business logic and test for a pull request? And after that, you figured out that there was a better approach and threw everything you had done before. That not only throws your work but also costs the company money.

There are many examples like that, it is not because we do it wrong, it’s about our philosophy and our expectation when reviewing code is not appropriate.

A better approach

It is a narrow mind which cannot look at a subject from various points of view.

— George Eliot, Middlemarch —

Let’s adjust the expectations:

• Every solution should bring real value to the users and revenue to the company
• The main objective of doing a code review is not about finding bugs, it’s about sharing knowledge

Keeping the expectations in mind you will end up with a better approach and steps when reviewing code.

Code review process

Context is the most important

Before going to look at the pull request, you need to answer or have answers to the following questions:

• What is the pull request trying to solve?
• Is the problem problem at all? Is this urgent?
• Why does the pull request go that way?
• Is there any alternative solution?

Take a look at the ticket or any related documentation to understand the context of the problem that the pull request is trying to solve. Sometimes, you can have better suggestions without looking at the pull request.

For example, the team is trying to implement an in-house job queue solution. With the context, and based on the team resources, the better approach could be using a 3rd party solution for the queue.

Or even better, sometimes, the solving problem is not a problem at all. Challenging requirements from products and users is essential, it helps us to clarify the reasons, sometimes, we can propose another approach or replace the problem with a smaller problem.

It’s a not good signal if there is only one solution

There is no perfect solution, only trade-offs and good enough solutions

_Someone in the world_

Every solution should come from the actual user value and the company revenue. If they don’t, then ignore them as soon as possible.

When defining a solution, many factors impact the final decision:

• Delivery time
• Budget
• Resources (size of the team, strength of the team, etc.)
• Technologies
• Technical debts
• etc

Architecture decision records are awesome documentation for any important decisions. Then in the future, we always understand the context when the decisions were made.

Therefore, looking at solutions, their trade-offs and the final decision should be done before looking at the pull request is highly recommended.

Additionally, don’t hesitate to pose goofy questions or challenge the solutions. It is a good practice to build a nice culture company, and sometimes, it contributes to improving or changing the solutions.

Having the overview of the actual result (Design)

To give good feedback or learn from pull requests, you need to know what the result looks like. For example:

• If a pull request is implementing a new API, let’s look at the API design including methods, routes, request body, response body, authentication, authorization, etc. You should probably give good feedback on the change and the whole remaining pull request will be adjusted accordingly.
• If a pull request is implementing a front-end view, take a look at the UI design, you might have in your head about the components, existing icons, code structure, etc. They are useful when you review the implementation.
• If a pull request is introducing a new database table, then the database schema is important to see the relationships and columns. You might find a glitch in the design or a missing relationship to another entity that you worked on.
• If a pull request is producing a new entity, does the new entity belong to the correct domains and boundaries?

Therefore, having the overview of the actual result gives you clues on how things will be implemented, sometimes, saving time for you and reviewers by giving feedback for design before implementation.

Business logic is the core fundamental of a system

The next important thing to focus on is the business logic in pull requests. Business logic is about the domain knowledge and the relationship among entities. They are the core fundamentals of a software system. Ensure to have the same page from both reviewers and reviewees making follow-up feedback and discussions short and focused. Otherwise, people misunderstand each other and say different things, then costs time and energy.

Good feedback on the business logic could lead to a change in the implementation and then a change in corresponding tests. Having a clear business logic defines use cases when writing tests. Let’s take a look at the following example:

• The business logic of education software does not allow a student to see the scores of all his classmates. Then in the pull request, if the rule is not guaranteed, it is a red signal and should be fixed before going to check tests.

Testing

Tests are inevitable for any software development team. Tests make us confident about any change, new releases, etc. Therefore, writing good and correct tests is important and required. Depending on the design and the business logic of the pull request, it requires the proper kinds of tests and test cases.

When looking at tests, one of the right approaches is collecting the test cases without looking at the tests’ details. This helps us to see if a pull request has covered enough business logic, edge cases, and good types of tests. For example:

• If the pull request is building a new API, integration tests are essential to challenge authentication, authorization, business logic and the response format
• If the pull request is changing business logic, unit tests are essential to ensure the change and edge cases.

Coding convention

In each team or project, there are conventions of naming, coding, structuring, patterns, etc. It’s style guidelines for programming, and it mitigates the endless debate among team members about good practices 😉

Correct coding convention or understanding the convention is important to ensure the team is on the same page and totally focused on development.

Coding optimization

Yes, this should be the last part of the reviewing process. Once the implementation is reasonable, coding optimization is the thing that we could take a look at. Common optimization are:

• Space complexity
• Time complexity
• Reusable code

However, we should take this part with care. You might end up with premature optimization. Let’s take a look at the following questions:

• Is optimization measurable?
• Is real-life data big enough to see the difference in the optimization?
• Is code used in multiple places? Is it straightforward to make it reusable?

Optimization by itself requires the improvement of the performance, resources and developer experience. If there is minimal or unclear benefit, then there’s no justification for undertaking it.

Is that all?

Reviewing code is not only finding bugs. Always be prepared to express gratitude or offer praise when you gain new insights or come across exemplary work.

Conclusion

The process looks verbose, isn’t it? Yes and no, if you build a good practice and you have some experience with the system, you can do it conveniently. I’ve applied this process every day and believe me, it won’t take you an hour a day.

Lastly, folks, as software engineers, just writing code isn’t the only thing that shows our value, especially now that AI is getting better at it. We’ve got to beef up our game with skills like better communication, working well with others, and being open to new ideas. It’s all about adapting to this changing world of software development.