This is a follow up of my previous blog post on code standards.
So, Being part of a Code review process is very important for me and trust me I take it very seriously. This is required not just for the team but for an individual learning as well.
Code reviews are very crucial for knowledge transfer and to avoid making small/common mistakes and of course maintaining best practices throughout the dev team. So Let’s take my team for example: we are around 12-15 developers in the team, all producing code which needs to be reviewed. So basically yeah that’s a whole lot of code!
Why It’s Important
Pushing code to production is easy. Anyone can do it, right? What concerns us that what pieces we are going to deploy.
The code can be completely fine or it can be a piece which can make everything fall apart. To maintain high code quality, we all need to have Peer code reviews. There is nothing negative here, we all are on the same team and we have a common goal that is to deliver the highest quality product.
You must be thinking, Is it worth it? No? Yes?
Definitely yes, Not having code review integrated into projects can result in big problems. You may have heard of the disaster happened long back: Here you go.
There was a lot of reason why this incident happened and one of the reasons was an absence of a peer code review. After reviewing their source code they found – possible bit flips, task deaths that would disable the fail-safes, memory corruption, single-point failures, inadequate protections against stack overflow and buffer overflow, single-fault containment regions, thousands of global variables. The list of deficiencies in process and product was lengthy.
How we roll things
General Development Process
FYI ;)
- Green line: Determines pass status.
- Red line: Determines fail status.
This is our general flow which is being followed in most of the projects, of course, columns may vary depending on the different aspects, clients, and projects. Hope it’s very clear 😓, if not I tried my best!! Haha 😄
Github Code review flow
Now it’s really easy maintaining your code review process just a few clicks away on GitHub (And it’s really awesome!). We can create new projects on our repo and use it as we want.
Generally, we have around four columns which are usually enough:
- Ready for review: You can add cards (Pull requests) to this column if your code is ready to be reviewed.
- In review: Now its code reviewer’s responsibility to move the card to "In review" column so that it gets updated on the branch and the concerned dev understands that his/her ticket is currently being reviewed.
- Change Requested: Again this is code reviewer’s responsibility to move the card to this column if the review has failed standards. Then concerned dev will do the required fixed and need to push the ticket back to “Ready for review”.
- Closed/Done: If the card is lying there in “Closed” column, that means the PR has passed standards it should have.
What to look for when reviewing
There are certain aspects which we consider while reviewing code and some of them are the following:
- Best practices pertaining to Drupal standards. Can be found here.
- JS specific coding standards. Can be found here.
- SCSS specific coding standards. Can be found here.
- Anything developed which is adding technical debt. For instance choosing an easy solution making it harder to implement changes later on.
- Security issues
- Potential Impacts which can introduce performance issues/threats to existing solutions
- Solutions being developed which are already available in the codebase and can be reused.
- General standards like formatting, linting issues, comments, naming patterns etc.
Who is responsible
We have set few guidelines on who will be responsible for the code review process. Here are the points:
- There will be one Senior and one Junior Code Reviewer for each ticket.
- Reviews will be done right after the daily standup. (Depending on how long it will take.)
- One person will review at least one ticket in a day (If there is any ticket assigned on the board)
- Tickets should be in respective columns according to the latest update. (Code reviewer & dev's responsibility)
- If there is any feedback from Senior code reviewer, its Junior code reviewer's responsibility to look what he/she has missed.
Making sure the process being followed
We regularly come back and observe how we are doing according to set process and if there is any way how we can improve to make sure it's being followed and not a burden/blocker for any one’s work.
The process is clearly defined and maintained on different platforms depending on client and teams. We usually use Confluence and highly recommend it to anyone who is reading this post.
Learning from our mistakes
We have maintained a code feedback sheet where we mention common mistakes we need to avoid. Everyone has access from the team and can add points on where we can improve, new techniques to achieve certain functionality, strictly avoiding certain coding patterns.
Our goal is to
Enhance the learning of individuals to become better programmers.
Improving the quality of codebase, it gets complex to manage as it grows.
Not to deliver just quantity but quality throughout.
Maintaining discipline within the team and understand the seriousness of spaghetti code.
That’s about it! Few things I may have missed for sure in this. Connect with me at lovehuria@gmail.com if you would like to know more OR just comment down here and save me all the trouble on checking my emails. :D
Visit the original post here. Thanks for reading :)
Top comments (5)
I've followed similar approaches through the years and I agree that code reviews are a tool for both quality and learning.
Looking at your workflow it's very smart to make a pass through a junior dev and then by a senior dev. However, I see that code review comes after QA.
In what way would it impact your development process to first get the code quality to good standards and only then send the feature to QA for validation? Have you tried this before?
It's debatable actually. We think that code review is a critical process and only want to review features which fulfill the acceptance criteria.
So, If it fails then it will have to go through the QA process again.
Though we keep experimenting, what I think is Code Review is done to polish things, making sure standards are being followed, do the refactors if possible etc. But if it's not even meeting the acceptance criteria then its just a waste of code reviewer's time.
I have tried this before and what i explained above is the same reason we switched to this approach.
Test first !! Code Review!! Sanity tests again!! Done!!
Great!!
But in case of code reviews on daily basis, how much time Devs are gonna spend fixing the review comments!!
Don't you think this is gonna impact the Sprint Backlog?
Yes Absolutely, but for a second let's consider that Devs are not fixing the important things they have missed; It will ultimately increase some hygiene work which is also not good for the Sprint Backlog. :)
+1