Code Reviews at Zenguard
Building and maintaining a clean and structured code base.
As a freshly onboarded intern of the Zenguard team the first weeks were, as with most introduction periods, mainly about getting up to speed, getting familiar with the workflow, setting up your development environment and gaining access to testing and production environments. After this period it is time to develop features, fix bugs and work on tasks, issues and stories. Before any of these features, get deployed to our QA team and after QA to the actual production release, the code gets reviewed… of course.
For someone like me, who just finished his application development studies, and having little to no experience with reviewing code, code reviews can be intimidating and I found it personally hard to start participating in them. Questions like “What do I look for when reviewing code?” and “Would this be actual valid input?” come to mind.
After asking some questions about how we do code reviews eg: “are there any guidelines that I can follow?”, we came to the conclusion that their is room for improvement in our current way of reviewing code. This blog post is about code reviews in general, how we used to do code reviews and how we do our code reviews now.
Code reviews in general
As mentioned before code reviews can be intimidating, especially when you are new to code reviewing in general. The most important point in my opinion is to lose this attitude, and realize what code reviews are really about:
- Improving and maintaining a clean and structured code base.
- Learning what and why changes are made in the code base.
- Giving new programmers the opportunity to learn and ask questions.
- Sharing knowledge.
Based on these points any comments, change requests and criticism on your or somebody else’s code should be appreciated, and will ultimately result in a better code base, growth in expertise and will help to create and maintain a positive code review culture in which finding defects is considered a good thing.
How we used to do it
After my onboarding period and having worked on some tickets, I found that it was time to start participating in reviewing our code. We had some general rules, for example: “Pull requests should be reviewed by at least 2 developers before merging with the base branch” but no concrete processes or scheduled code review sessions.
We do our code reviews on Github and our general flow is as follows:
- The developer finds an issue to work on, then creates a new branch and names it appropriately
- After finishing the issues, this can be a feature, bug fix etc. The developer creates a pull request and the integration tests and code quality tests are ran.
- At this point the pull request is open to be reviewed by our other developers, if the code gets approved by 2 developers the pull request can be merged.
If there are any requests for changes, they will be commented on the concerned file and lines. The developer sees these requests, changes the code appropriately, pushes the changes and the code review process starts again.
How we do it now
Because we are working in a small development team the process that that was already in place worked fine, and instead of changing the process I like to believe that we’ve upgraded it. After doing research on how other companies do code reviews, and there are a lot of resources on this topic online, we’ve scheduled a meeting and came up with the following process to review our new developed code:
- Code should be reviewed by at leas 2 developers before merging.
- Use a checklist and established guidelines while reviewing code, our checklist can be found below.
- Weekly developer jour fix where we discuss technical debts and do code reviews: developers show examples of code what in their opinion can be improved and we discuss the best way to do so.
We are currently still doing or code reviews on Github and so far we have not found it necessary to purchase or use another external service specialized for code reviews. Github makes it easy to to add integration and code quality tests to pull requests and the code review interface makes it easy to comment on, or request changes to code.
Although we already were reviewing code and had some guidelines for doing code reviews, it is always nice to be able to improve these processes and streamlining it which hopefully increases productivity in the long run by keeping a clean and organized code base were developers know what, when and why changes were made it the existing code base.
This is by no means the best way to review code and please feel free to suggest ideas and or improvements, I hope this blog post will be interesting for developers who are new to code reviews or small development teams who are struggling with doing code reviews or establishing processes for it.
Code reviews are important! You should define goals and have standards, but keep in mind what the overall purpose is. Don’t be picky, don’t review code for hours straight, stay polite and as a team try to build a comfortable atmosphere which will result in making your own working life easier.
Our current basic checklist
- Does the code work? Does it perform its intended function, the logic is correct etc.
- Is all the code easily understood?
- Does it conform to our agreed coding conventions?
- Is there any redundant or duplicate code?
- Is the code as modular as possible?
- Is there any commented out code?
- Can any of the code be replaced with library functions?
- Can any logging or debugging code be removed?
- Are all data inputs checked? (for the correct type, length, format, and range)
- Where third-party utilities are used, are returning errors being caught?
- Is all personal and possibly vulnerable data checked and encoded?
- Are invalid parameter values handled?
- Is there any incomplete code? If so, should it be removed or flagged with a suitable marker like ‘TODO’? and even better should it be converted to a ticket?
- Is the documentation limited to a minimum?
- Is the code testable?
- Do tests exist?
- Do unit tests actually test that the code is performing the intended functionality?