Group code reviews

Posted: 2023-01-29 16:03:38 by Alasdair Keyes

Direct Link |

---

Whist working with a customer of mine, I was introduced to a way of code review that I hadn't seen before. I'm not sure if it's particularly unique to them, but in my experience it's fairly rare... group code review.

In all previous companies I've worked at/with, a pull request (PR) is created and has some key devs added for Code Review. For a speedy review a link to the PR was often thrown into the Dev Slack channel with a little prodding or begging to get the task done.

Now, most people in dev teams are busy; or at least if they're not too busy, they find almost anything more interesting than reviewing a PR. It can be tedious and if it's touching code that you're not familiar with, you can often end up spending some time trying to work out both what the code was doing and what the change is supposed to be doing. In the end it was often team leaders who ended up picking up the slack and performing the reviews to get code out the door. This puts an excessive burden on them when it could be shared more equally.

The process

This particular customer had group code reviews once in the morning after stand-up and then an optional code review mid afternoon if required. My first thought was that this would cause undue disruption to work flow but over time I saw immense benefit in this approach and very little drawback.

The developer that wrote the code would create a PR as normal. At the review, they would share their screen on a famous git-hosting provider. Usually they would give a sentence about the ticket and what they were trying to achieve. Due to stand-ups and backlog planning, everyone usually had a fairly good idea on this already. The dev would proceed to go through the changes on their branch, giving a run down of the code as they went. Questions or points could be made by anyone and a discussion could be had amongst the group. If new tests were made, they would also be run through briefly so that other devs could see what was being tested, whether it was suitable/adequate and any edge cases that might fall through the cracks and need to be added. (It also required that the tests passed!)

Any changes that were required and agreed upon by the team were added to the PR and the dev would go back and implement the changes.

If the changes needed were minor, they were often approved by a senior dev and scheduled for release. Larger changes would have to have a new PR created and go back into the code review process. The coder would only have to go through the changes from the last code review, not the whole shebang.

The benefits...

• Ensures domain experts see the code - The developer may not have much experience at the code being changed or a full understanding of the system. Other members who have this knowledge can ensure that the changes would fit in with the design and not break anything.
• Ensures non-domain experts see new code - This is fantastic, especially for new members. Seeing the changes and how they integrate with the existing code allows people to get a feel for how the system works without having to delve into the code personally. They can easily pick up any design patterns that may be in use and understand the code/class hierarchy and layout. Over time they will gain a general awareness of the code even though they've not touched it directly. A kind of learning by osmosis.
• Team leaders can assess team members - This provides rolling assessments of the knowledge and skill of their team without the need for any separate review process. Team leaders can make notes on progress and growth for one-on-one meetings and also use the knowledge to help assign future tickets to team members.
• Code practices can be debated and decided - It's a single time/place where code practices can be hammered out. Getting consensus on the way things should be done can be difficult, especially in any group larger than 4 or 5. Devs can explain why they've chosen to tackle a problem in a specific way. Others can dispute it and code-practices and styles can be decided on for the future. Everyone (or almost everyone) is there and can have their say and a consensus can be reached.
• A chance to explain - When reviewing code, you often just see the end product and may have ideas on why it's wrong or how it could be improved. Maybe you feel the wrong design pattern was used? When these issues are raised, a group setting gives devs an arena to explain why changes were made the way they were. They can explain thought processes and previous attempts at a solution and why they were discarded. Just looking at a code review on the screen doesn't explain the knowledge and thought that went into the code and why X process was adopted instead of Y to solve the problem.
• Reduce time-to-release - This method reduces the time that PRs are sat around waiting for review by forcing them to be reviewed. Code reviews are dull. Unless you're the sadistic type that likes to just tear into other people's code, you can always think of better ways to spend your time. This way devs are not having to hassle people to get code reviews to release to production and wasting time whilst waiting.
• More equality in the burden of code review - That developer who always ends up reviewing code because no-one else does finally gets a break. No more "You've been added to review code" emails and the responsibility gets shared amongst the team.

Things you think are bad... but they're not as bad as you think...

• Minimal break in flow - No one wants to be disturbed and have to stop coding when they're making progress. Having the morning review directly after the stand-up means that no-one has to break their concentration again as they were already in the stand-up.
• Minimal break in flow... again - The afternoon review is optional and based on whether there are reviews needed. Usually the message Code reviews? was thrown into Slack and others would only reply if they had one. If there are only small reviews that aren't too important and not holding anyone back, they could be done at the next morning review. Everyone can then continue with their work uninterrupted. Only break flow if there is a need.
• Time loss - You might do some quick maths and work out that 10 developers spending 15 minutes is 2.5 hours lost each day. That's true, but it's only 3% of dev time each day ()assuming an 8 hour day) and the benefits listed above are well worth it. And lets face it, we probably all spend more than 15 minutes a day getting coffee, smoking/vaping, eating snacks... it's not a huge loss.
• Introduction to public speaking - Very few people like standing up in front of peers to talk, even less so when you feel you're having to explain yourself and your work; but it's not a bad thing to get used to. Explaining things and defending your position are good life skills in general. After a few reviews, you realise it's not all that bad and it builds team cohesion, trust and over-all confidence in your code and abilities.
• Introduction to tact and useful criticism - The flip side of the above point is that it requires the other developers to show tact and diplomacy when reviewing code... especially with junior or newer members. Also remember that often devs won't just have knocked out a big commit of code without any input from the team. The implementation may have already been discussed between the team so there's not going to be anything too damning to criticise.
• People playing with laptops - A group code review like this often causes laptops to be in the code review. We all know that if there's a computer in front of you, there's a temptation to use it. Some ground rules can be set about closing machines when not specifically used for the review.

So that's my review on group code-review. If you're looking for possible improvements to your process I strongly advise you give it a shot. Do it for a month or so and see how you get on. You can always tweak the process based on your needs and fall back to your previous ways if you don't like it.

If you found this useful, please feel free to donate via bitcoin to 1NT2ErDzLDBPB8CDLk6j1qUdT6FmxkMmNz