The Code Review Process and Checklist
Narendra Kumar
Solution Architect| Team Mentor | TECHNIA (Addnode India) | PSM 1 | Oracle Certified Associate | DS Certified
Code Review:
Code Review or peer review is a continuous and systematic process to investigate. the code to find weak spots, faults in the initial development phase, improving the overall quality of application and developer’s skills.
The benefit of Code Review:
“Software testing alone has limited effectiveness — the average defect detection rate is only 25 percent for unit testing, 35 percent for function testing, and 45 percent for integration testing. In contrast, the average effectiveness of design and code inspections are 55 and 60 percent” -Code Complete
The main purpose of code review to ensure that the code that is being produced has sufficient quality to be released with industry code standards. So, it is an effective litmus test for the quality of code.
The second purpose is as a teaching tool to help developers on how to apply the techniques to improve code quality and giving the opportunity to learn different and potentially better ways of coding.
The study of the Microsoft code review team shows the various aspects of code review in the below Chart.
Code Review Process:
When a developer is ready with his/her code changes Create a Pull Request in a web-based version control repository tool like bitbucket and add reviewers to review the changes.
Reviewers review the code changes and provide feedback as either approve the PR if changes meet the code review guidelines or make comments with some questions or suggestions to the author. The primary developer updates the code or addresses the comment accordingly with his/her belief if any comments on PR. Reviewers re-review the updated code and provide his/her feedback. This process is continuing until the reviewer and developer both agreed with the changes. The reviewer merges the code changes in the common code base when code fits to go.
Code Review Checklist:
Checklists are a highly recommended way to find the things you forget to do and are useful for both developers and reviewers. A checklist is the single best way to combat the problem, as it reminds the reviewer or developer to take the time to look for something that might be missing.
Every software organization has its own code review guidelines according to what technology and framework they adopt in software development. There is some common checklist that is followed by most of the software companies regardless of language.
1.Objective-based
Make sure the changes achieve their purpose. In simple terms, it does what it is supposed to.
2. Follows architecture
Ensure that the code changes are according to the approved architecture/design throughout the application, if any design changes required, ensure that these are documented, baselined, and approved before implementing them in the existing code. Understand the problem and context before using the design pattern if you need, consult with the team.
- Divide the code into multiple layers and tiers as per requirements (Presentation, Business and Data layers)
- Make files respectively (Java, JSP, HTML, ECMA Script, CSS, etc) instead of keeping everything in a single file.
3. Unit Tested
The code passes the unit test and enables the integration testing for every core method with the tools like JUnit or another tool depends on the language.
4. Reusable
All functions/methods serve a limited and clear purpose (follows DRY principle). Functions/methods are reusable wherever applicable and written in such a way that they can be re-used in future implementations. Logics make use without ambiguity and no duplication.
5. Performance-oriented
The code follows all performance principles and scalable to handle large amounts of the data in the future and ease to implement of upcoming features. No long delays between requests and responses. Focus on the 20% of optimizations that produce 80% of results. Below are some ideas to deal with performance issues.
- Use StringBuilder instead of String concatenation and StringBuffer (if not applicable)
- Avoid setTimeOut function in ECMA Script.
- Database queries should be fastened and use prepared statements. Handle the DB queries according to scenario clubbing the DB queries or breakdown into smaller ones depends on the queried data size. Do not hit DB queries in the loop it is very expensive.
- Lazy loading, asynchronous, and parallel processing.
- Caching and session application data.
6. Secured
Code deals with user input, address security vulnerabilities such as cross-site scripting, SQL injection, and do input sanitization and validation. It securely handled and stored the user credential and other sensitive information.
7. Meets coding styles & standards
Verify that code follows the coding guidelines, standards for naming conventions (Pascal, camelCase, and no Hungarian notation). There are no hard-coded values like user credential, product version information. It should be configured through the configuration/properties file. The entire team uses standard Java formatter to meet standards/format (remove unused imports, line length, white space, etc).
- Deal with language-Specific Issues
- Do not use the Deprecated APIs
8. Unbreakable
The code handled validation wherever necessary. The code never breaks under any circumstances. The code should handle edge cases and sanitize before taking it further. Every object is checked for its actual data existence before accessing its properties.
9. Error handling and Data formatting
Every response that is returned by the server must be properly handled, not only the error messages. It should have necessary headers, response messages, error codes, and any other necessary details attached to it in the required format.
10. Follow object-oriented Analysis & Design Principles
- Single Responsibility Principle: A class/function shouldn’t handle multiple responsibilities, refactor into classes/functions based on a single responsibility.
- Open Closed Principle: New functionality should be introduced in new classes and functions instead of modifying existing ones. modification of existing classes/functions might be expensive and require extra effort to validate all changes and reduce the maintainability in the near future.
- Liskov substitutability principle: The child class can be used as a substitute for a parent class. It should not change the parent class behavior.
- Interface segregation: Interfaces should be specific rather than doing many and different things. Implementing class will only implement the specific needy interfaces rather than being forced to implement methods that it does not need it. So, large interfaces should be decomposed into smaller, more specific ones.
- Dependency Injection: Prefer dependencies injection instead of hard coding.
11. Manageable
The code should be easily manageable, readable, and requires the least amount of effort to support in the near future.
- Readable: The code should be self-explanatory, no need to add an extra comment to explain its behavior. Use appropriate names for variables, functions, and classes so everyone can understand easily like s/he is reading a story. Don't use Hungarian notation for the reference variable name it really hard to remember after a few lines of code what reference it holds.
- Testable: The code should be easy to test. Use interfaces while talking to other layers, as interfaces can be mocked easily in testing tools like JUnit. Avoid static functions, singleton classes if possible, as these are not easily testable by mocks.
- Debuggable: Provide full support to log the flow of control, parameter data, and exception details to find the root cause easily when application turns on debugging mode.
- Configurable: Make code configurable and generic so It can be reused from multiple places on passing the value from config files like XML file. so no code changes are required for handling the frequently changing value.
12. Memory handling
Some Resources that are not automatically released after usage is freed. Connections, ports are closed properly. Use try with resources to auto-close the files in Java. Keep in mind if you are storing the huge value in the collection then it should be freed after use.
13. Traceability/Logging
Log every transaction or the ones that require logging. They are stored in a repository (as a file) as well as in the database (as text). Logging in different stages for different purposes can be enabled/disabled in the configuration file (like web.config)
14. Code Efficiency
Well-developed programming codes should be able to handle complex algorithms. The coding efficiency can achieve by applying the above principles such as performance, error handling, unbreakable, and keeping tracing/logging. Code coverage is as important as the unit test cases passing. 90% of the code is covered that means 90% code is tested via unit test cases.
15. No Warnings/Console logs
Avoid writing any application-related information in the browser console and it is not throwing any warnings. developing logs are cleared are not throwing in the browser console.
16. Legal usage of third-party tools/Libraries (Licencing)
External libraries are used only if proven necessary for the application. If there are third-party tools or libraries used, then the licenses and legalities are verified and complaint.
Assign Severity for code reviews issues:
Assign the severity of each code review issue. The reviewer must focus on issues with High severity first and then to Medium severity and then Low severity issues.
