The 7 steps to a complete Code Review
Undoubtedly, one of the most important part of coding or web development is Code Review. Code Review can be done by developers, QA engineers, peers, seniors, juniors basically anyone. But why is Code Review important? Is it because it helps detect bugs early on? Is it because it helps in maintaining the quality of the code that you are writing? Besides all these factors, analysis of your code, no matter how strictly you have followed proper coding guidelines, is always a plus point. Another pair of eyes might help find issues where you might have completely missed even if it’s just a typo. Below is a list of some points to consider while performing a Code Review.
1. Correct Syntax
One of the basics of code review is checking for correct syntax. Issues such as incorrect indentation, alignment, extra space, extra line, missing End of File line, missing semicolons might seem insignificant but contribute greatly to the code quality. There might be some commented code that needs to be removed or a forgotten binding.pry/ debugger.
2. Grammar
Are there typos, spelling mistakes in the code? Is proper grammar tense implemented? Correct usage of the English language must be maintained. Do the file, variable, class names make sense?
3. Code Linting Errors
Linting is the process of running a program that will analyse code for potential errors. If your codebase implements linters/code analysers then checking for any linting errors the developer might have missed. There are many code linters available for various programming languages. I have worked with ESLint, a linter tool for identifying and reporting on patterns in JavaScript and Rubocop, a Ruby code style checker based on the community-driven Ruby Style Guide. An example of a Rubocop warning is shown below.
4. Code Reusability/Duplicate Code
DRY (Don’t Repeat Yourself) is a principle to remove repetition of code. Are available methods being used effectively? If similar code is used across multiple files, make a generic method and allow it to be implemented wherever required. Also, all new code should be written with a view for further use. This minimises the lines of code and helps maintain codebase.
5. Technical Quality
This point encompasses a wide range of factors. Many factors contribute to the technical quality of a Pull Request.
i. Code Logic
Is the code’s performance in accordance with the PR description? Does the stated feature work as expected? Is the code logic satisfactory? If just scanning the code is insufficient to verify it then checking out to the related branch and having a look around with it is always a good option.
ii. Naming Convention
Are the files, classes, methods descriptive enough? Does it convey it’s purpose? Is it understandable or not? Are the objects, classed named according to their functionality? Are constants used when required for the objects whose value will not change? Does the code follow naming conventions that your codebase implements such as CamelCase or underscore_case ?
iii. Condensed Code
Are there any unused variables or unused code? Is an assigned variable only used once? Can the number of lines in the code be reduced? For eg:- Using a ternary operator instead of if else block.
iv. Security
Is the code secure? Are there any security vulnerabilities? Has the author implemented authentication, authorisation? Authentication determines who the user is. Authorisation implements access control and ensures your code is only accessible to those users to whom you have allowed access.
Does the code redirect users on denied access? Are sensitive information such as passwords encrypted?
6. Error Handling Mechanism
The code should always design for failure. In an ideal case, the code will give desired output but when code fails, error handling mechanism is necessary. Error handling will catch an error and redirect it to the code you want to implement, instead of shutting down the application. It should handle edge cases such as null or negative values. Things to consider might be if the code has proper error responses for failures. For eg:- API error responses ideally have a message and status code.
7. Test Coverage
TDD (Test Driven Development) is an essential part of programming. Every developer will find this out sooner or later. Ensuring that the PR contains test for all new lines of code introduced into the codebase is necessary. Tests help to catch and minimise bugs if any, checks whether the existing behaviour of the codebase has been altered and if so how and is also an excellent documentation of existing features. It also helps fellow developers add onto existing code. Always check if all tests have passed in the codebase.
Automated Code Analysers
Additionally, there are various static analysis tools available that analyse the quality of your codebase such as CodeFactor, CodeClimate. They provide automated Code Review for Github which checks all the PR’s associated with your codebase for code quality. Besides analysing code errors, static analysers are available such as simplecov for automated test coverage, bundler-audit for database vulnerabilities or brakeman to analyse security vulnerabilities.
Besides the obvious benefit of helping improve code quality, Knowledge sharing among developers is one key benefit of Code Review. Going through PR’s allows us to learn how fellow developers code, what good code or bad code looks like and makes sure the whole team is familiar with the codebase. It is especially beneficial to new engineers as senior team members have the opportunity to impart their knowledge and advice through Code Reviews. Every new PR is a learning experience that allows us to implement better practices in the future.
