How to keep high coding standards without losing speed?
Code review ideas covered in this article:
- A formal code critique with the demo
- An informal code critique with the demo
- Review based on the GitHub request
- Approach with one designated reviewer
- Pair programming, an informal code discussion between two developers
Formal code critique with the demo
I have joined the "team" in April of 2019. The same month I've experienced my first formal code review. And I was terrified.
The meeting started at 9:15, after a daily standup, and one by one, developers walked into the small room with the big square table. There were only six chairs around the table, and with me joining the team, seven developers. Space started to feel crowded. There was no facilitator; the developer whose card was the first in the "Code review" column was the first to begin.
To say: "the engagement was high" was indeed an understatement, and the fact they called it "code review" instead of "code critique" is what additionally caught me off guard.
The first developer would take the lead, and others would patiently wait, eyes wide open. With the Trello card visible, devs would compare requirements with whatever was on the screen. The purpose of the demo was to find any flaws in the working solution. Then, a GitHub pull request followed. If a dev could have written a line in another way, well, the team would let him know it. If a prototype did not fully match the requirements, well, he would quickly learn to read those specs to the letter. And if something were so apparent that two or more team members would notice it at the same time, they would ecstatically yell at him, pointing to the problem. It was indeed a blood bath with only one purpose: to find something, anything.
The system indeed had some pros:
- Transparency of every pull request ensured that every new line followed established coding standards.
- Since we were reviewing code every day, this meeting worked like a twisted version of team building.
- Fresh employees would get directly educated in the expected means of writing software.
- An effective way to get into other people's code and understand the reasons behind decisions and different coding styles.
- Making sure managers get what they wrote in requirements and that software is working according to the specs even before getting to QA.
Cons were also plentiful:
- Seven developers would sit on a meeting every day, even if they could not add anything to the discussion.
- Long code reviews made it hard to stay focused and fully pay attention to the end.
- More devs in the room meant more questions and requested changes, frequently basing those requests on personal preferences.
- Slowing down the whole production process.
It was "the more, the merrier" type of a problem, where the more developers were in a room, the more updates they requested. Our highest number was nine developers on the daily review. The good thing about that period was clarity on what we need to change. Over time the number of developers decreased, and we started to rationalize how we use our time and what changes we are requesting. Once our team fell to only three devs, the detailed critique started working a lot better.
Let me fast forward to the present day.
Almost two years later, we still have a formal code review preceded by a demo. It's still happening at 9:15, but the team is smaller, there is less nitpicking, and we are more focused on functionality. We all work remotely and use Slack for reviews. We are still going through pull requests line by line, or at the very least, section by section. It is always a challenge to see the big picture, but keeping the pull requests small helps a lot. To summarize, we kept all the reasons for formal code reviews and cut off most of the reasons against it.
Conclusion
The detailed daily demo followed by line-by-line critique can work well in a smaller well-coordinated team. If done appropriately, this meeting functions as a daily check-in, an opportunity to ask questions, and the primary mechanism to keep your work in sync with others.
An informal code critique
Another meeting we use to keep our coding standards high is an informal code critique. In its form, this review is the same as a standard version. The main difference is in its ad-hoc nature.
This meeting comes in handy when:
- Team member needs approvals, before starting a new, related task on the same day.
- There is a pressuring deadline.
- Another assignment depends on this solution and can't move forward until this code is approved.
We are using Slack "@codereviewers" mention in our GitHub channel to assemble the team. It's a good practice to leave some room and schedule a call 15 - 30 minutes in advance. The last difference worth noting is; while for the formal code critique, we wait for the whole team to assemble, for the informal one, we only wait for the two developers that make a quorum.
Review based on the GitHub request
One of my favorite ways of getting and giving feedback is through GitHub or Bitbucket. It arrives in a written form attached to the pull request, followed by a quick call when needed. Both applications made this process easy, with features explicitly built for this purpose.
There are a couple of ways we can make this work.
Option A:
A developer creates a pull request and asks for a review. Other devs examine the code before starting with their day and leave comments behind. A face-to-face meeting follows if a discussion is needed. Otherwise, she makes changes, and reviewers approve her work.
Option B:
A slightly different than option A, option B starts in the same way, with a pull request. Other devs on the team get automatically notified. The ones whose opinions are relevant for a specific task will inspect, leave comments, or give their approvals.
The difference between these two options is in the review request. In the first case, a developer is actively asking for a review from specific experts. In the second case, stakeholders will review and comment on their own. Both options provide a straightforward and efficient way of keeping the codebase clean. A downside is minimal in-person communication only when developers can't agree on something and make a point through short comments.
Who should be a "reviewer" is a question to answer? I found that the best combination of efficiency while keeping quality happens when peers with similar skillsets and levels of expertise review each other's code. Here are some examples:
A full-stack developer reviews a request from a junior front-end developer (FED from now on).
Two junior back-end developers could check a senior BED code.
Two BED's could review each other's pull requests.
There might be a situation where the beforementioned examples won't work. For instance, I'm a FED but not proficient in React. Another FED writes all React code on our team, and she is fantastic at it. It would be expected from me to review her code in the normal circumstances, but since my understanding is limited, a senior full-stack developer jumps in, which brings us to the next option.
One designated reviewer
Scenario when a senior developer examines and approves all pull requests
Most teams out there have that one teammate who has been with the company for eternity. And for a good reason. He understands business needs better than anyone and is well-versed in many different fields. This individual is well known and respected as a problem solver and leader. Even though he is a great coder, he spends more time examining, debugging, and approving others' work. He might be a team-lead, where interpreting business requirements falls on his shoulders together with other managerial duties. Of course, the actual responsibilities of an employee of this caliber will vary based on many factors, some of which might be the team's size, the number of projects, career plans, and aspirations.
As it might sound, this approach is very straightforward. Junior and mid-level developers write the code, and a senior developer reads, debugs, and approves the work. As with everything else, there are some reasons for and against assigning extra work to a designated developer.
Pros:
- Competence. The most competent developer gets to decide what fits and what does not fit in the code standards. If there is a bug, this person will find it.
- Hierarchy. There is a clear hierarchy between devs doing the work and the authority that approves it. The primary outcome should be a product of higher quality.
- Time management. One meeting less for other devs on the team.
Cons:
- A large workload might prevent the best developer from coding. * The most complex problems need the brightest brains working on them, and with this approach, that might not be possible.
- Not fun. Inspecting other people's code for extended periods is not for everyone.
- Learning opportunities. Reading code is one of the best ways of learning how to write code. If younger developers are not doing it, we are taking that opportunity away from them.
Worth noting is that a lighter and possibly better version of this method would involve two or more senior developers effectively sharing the workload.
Pair programming
Two sets of eyes will always catch more details, spot more obstacles, and find better solutions faster than one set of eyes would. The most common use of the term is in the phase of "solutioning." I found the same concept very useful and practical for code reviews, so I'm adding it to the list.
Developers can collaborate directly through tools such as Visual Studio Professional or Microsoft Teams. I found that even a screen share without giving control works. On our team, we usually do it through Slack or Teams. We are both problem-solving or presenting a solution. I find it useful to show a solution and get feedback before an official code review.
On its own, this type of code review is embedded in the workflow more than other options. Since the observer still needs to take a break from his work, it is convenient to append this after another meeting or schedule it in advance. Due to its informal structure and a lack of urgency, it still breaks the workflow less than other code review types.
Talking exclusively about front-end development CodeSandbox is a new tool that takes pair programming to another level. CodeSandbox works in the same way as the Codepen, with a couple of extra features in the free plan. The most important one is the live pair programming mode, which runs smoothly, and so far, it has been very stable. Another option that I love is the ability to assemble a team. In that case, pair programming is given and a part of a regular workflow.
I believe this type has more pros than cons, but let's try to define some of them.
Pros:
- The informal nature of the meeting improves teamwork by making the connection between developers stronger.
- Developers are explaining, reviewing, and problem-solving at the same time.
- It is easier to see the bigger picture in this environment than during the formal code review.
Cons:
- Time management. Working in pairs on the same stuff can get lengthy and exhausting.
I would love to learn how other teams are reviewing their code, products, and designs. Please reach out with your ideas or leave a comment.
Top comments (0)