To Perform Effective Code Reviews

10 Simple Code Review Tips for Effective Code Reviews

Software code review is a process to ensure that the code meets the functional requirements and also helps the developers to adhere to the best coding practices. Additionally, the code review process helps in improving the software quality. 

Code reviews have been proven to improve software quality and save developers’ time in the long run. Also, peer code reviews as a process have increasingly been adopted by engineering teams around the world in all tech companies.

Be respectful, humble, and kind. Finally, the quality of the code review feedback does not only depend on WHAT you are saying, but also on HOW you are saying it. So, the best code review feedback is worth nothing when it isn’t carefully phrased, humble, and kind.

1. Highlight issues in the code

Never force software developers to change the code written by them. It may hurt their ego and they may repeat the same mistake if they do not understand the reason behind the code change recommendation. Highlight the issues in the existing code and its consequences.

Here’s an interesting quote on this point: “If an egg is broken by an outside force, life ends. If broken by inside force, life begins. Great things always begin from inside.” – Jim Kwik, Learning Expert.

2. Explain relevant principles

If software developers hesitate to accept given suggestions/recommendations, then explain them relevant principles such as Separation of Concerns, SRS (Single Responsibility Principle), Open-Closed principle, Cyclomatic complexity. If necessary, discuss with them the Non Functional Requirements (NFR) such as Maintainability, Extensibility, Testability, and Reliability.

3. Discuss relevant quotes

To make the code review process more interesting and engrossing, remind developers of relevant quotes/proverbs:

• “Any stupid can write the program that computer understands but only good programmers write code that humans understand” – Martin Fowler
• “Measuring programming progress by lines of code is like measuring aircraft building progress by weight.” – Bill Gates
• “Fat model and thin controller”, “High cohesion and Low coupling”
• “When debugging, novices insert corrective code; experts remove defective code.” – Richard Pattis

4. Do a few things offline

Instead of explaining the entire solution to developers during the code review process, simply share the links of relevant websites or encourage them to research on the internet by providing keywords. This action would certainly save the code reviewer’s time and energy. And of course, developers would also like it, since they too need some time to assimilate the proposed solution. 

Instead of always sitting next to a developer during the code review process, code reviewers should obtain the code from the source control or shared path, so that it saves developers time. And this would also give code reviewers ample time to recommend the best solution in the context.

5. Consider as an Opportunity to learn best practices

Sometimes software developers may take the code reviewer’s comments personally and defend the code without a valid reason. It then becomes the responsibility of a code reviewer to inform the developer to consider this exercise as an opportunity to learn/discuss best practices, but not to identify issues to criticize. Ideally, code reviewers should inform the managers that code review comments should not be used to assess a software developer’s skills. Code review should always be done in a competitive spirit to find more useful comments.

6. Always be patient and relook if required

Sometimes, developers do not accept suggestions/recommendations and keep debating. A code reviewer may not know the exact context and challenges when the code was written. A code reviewer should understand all the points being made by the developer without losing patience. Furthermore, to make the point crystal clear, a code reviewer can explain the points on a paper or on a whiteboard by comparing the developer’s approach vs the code reviewer’s approach. Every approach has its pros and cons, need to choose the right approach, whichever weighs more after careful evaluation.

Many times, a third approach evolves which is acceptable to both the developer and the code reviewer. If both of them do not come to a conclusion, then stop the discussion by saying “Let’s discuss this tomorrow, after doing some more analysis”. If the same issue is re-looked on another day with a fresh mindset, it is quite likely that a new approach evolves. Always remember that “No problem can be solved from the same level of consciousness that created it.” – Albert Einstein

7. Explain the need for best coding practices

Generally, software developers mention that best coding practices are not followed due to tight project schedules. Developers may feel that it is an acceptable practice. However, code reviewers should educate software developers that as the code size increases or after some time, the application becomes very difficult to maintain. Moreover, if a client verifies the code then poor quality code may give the wrong impression on the team’s/organization’s quality standards. It may also impact awarding new projects or referring an organization to prospective clients.

If the project schedules are too tight then code reviewers should suggest developers perform code refactoring while fixing a defect/adding an enhancement or in the next version. While refactoring the code, some functionality may break accidentally. Code reviewers should convince the project managers by explaining the importance of code refactoring and the need for allocating additional time to plan this activity.

8. Consult a second-level code reviewer (if not convinced)

If a code reviewer recommends a few suggestions, but the developer hesitates to accept these, then discuss them out with the developer. It is quite possible that the code reviewer may not know the entire context. If the developer is still not convinced with the recommendations of the code reviewer, it is perfectly all right to consult a second-level code reviewer. However, the developer should ensure that the second code reviewer’s suggestions are forwarded to the first code reviewer to ensure that everyone is on the same page.

9. Capture the enhancements and technical debt

It is quite likely that some code review suggestions cannot be implemented during the current release. However, a code reviewer should ensure that all accepted recommendations are clearly documented in a shared code review document so that these are implemented at an appropriate time in the future. Additionally, the code reviewer should identify and capture all the enhancements from a technology and business perspective. Once the project is completed, all captured enhancements can be considered for implementation, instead of searching at that moment. Finding enhancements during code reviews is more efficient than finding them separately at the end of the project.

10. Document all code review comments

Document all code review comments in an email, word document, excel, or any standard tool used by the organization. Making a mistake for the first time is acceptable, but it is not a good sign to repeat the same mistake. The code review document helps software developers to cross-check the highlighted issues and avoid making similar mistakes in the future. Additionally, maintaining a code review document is a mandatory part of the Capability Maturity Model Integration (CMMI) level process.

Code Review Checklist

1. Functionality

Checklist:

• Does the PR work as expected?
• Does the new feature add value or is it a sign of feature-creep?
• Will it impact existing functionality?
• Is it going to create inconsistency at other places?

2. Readability, Code syntax & formatting

Checklist:

• Is the code clear and concise?
• Does it comply with PEP-8 / best practices?
• Are all language and project conventions followed?
• Are identifiers given meaningful and style guide-compliant names?
• Ensure that proper naming conventions (Pascal, CamelCase, etc.) have been followed.
• Ensure code is properly indented and the team follows the same rule (two spaces/tab) in the project.
• Does the PR add test cases for the modified code?

3. Design Principle

Checklist:

• Is the code properly planned and designed?
• Separation of Concerns followed?
• Is code in sync with existing code patterns/technologies?
• Did we think about reusability?
• Is the code well organized in terms of the placement of components?
• Did we explore existing design principle that suits the need?

4. Patterns, idioms & best practices

Checklist:

• No hard coding, use constants/configuration values.
• Does the code keep with the idioms and code patterns of the language?
• Does the code make use of the language features and standard libraries?

5. Documentation and maintainability

Checklist:

• Is the code self-documenting or well-documented?
• Did you add Comments mentioning the reason for the change, todo, workaround, hacks did in the code?
• Is the code free of obfuscation and unnecessary complexity?
• Are the control flow and component relationship clear to understand?

6. Debuggability, Testability and Configurability

Checklist:

• Are we logging exceptions, the flow of control, user behavior for better debugging, and consumer behavior understanding?
• Is code testable?
• Is code configurable enough, to avoid changes in business or view layers or even code changes?

7. Performance, reliability, and scalability

Checklist:

• Is the code optimized for in terms of time and space complexity?
• Does it scale as per the need?
• Does it cover failure scenarios?
• Does it have instrumentation like reporting for metrics and alerting for failures?

8. Security

Checklist:

• Is the code free of implementation bugs that could be exploited?
• Have all the new dependencies been audited for vulnerabilities?
• Does it have Authentication, authorization, input data validation against security threats?

9. Etiquettes

Checklist:

• Is the PR atomic?
• Does the PR follow the single concern principle?
• Are the commit messages well-written?

10. Notice What’s Missing

Checklist:

• Did you try using app/functionality as end-user?
• Does it cover loading, error handling, edge cases, and unexpected input handling?
• Will it work in all support environments OS, browsers, platforms, etc?
• Does it need feature flag control?
• Does it have proper instrumentation?

Code review doesn’t just improve the project’s code, but the trust between project and it’s consumers in long terms.

Another Point of view checklist :

1. Code formatting

While going through the code, check the code formatting to improve readability and ensure that there are no blockers:

a) Use alignments (left margin), proper white space. Also, ensure that the code block starting point and ending point are easily identifiable.

b) Ensure that proper naming conventions (Pascal, CamelCase, etc.) have been followed. 

c) Code should fit in the standard 14-inch laptop screen. There shouldn’t be a need to scroll horizontally to view the code. In a 21 inch monitor, other windows (toolbox, properties, etc.) can be opened while modifying code, so always write code keeping in view a 14-inch monitor.

d) Remove the commented code as this is always a blocker while going through the code. Commented code can be obtained from Source Control (like SVN) if required.

2. Architecture

a) The code should follow the defined architecture.

1. Separation of Concerns followed

• Split into multiple layers and tiers as per requirements (Presentation, Business, and Data layers).
• Split into respective files (HTML, JavaScript, and CSS).

1. Code is in sync with existing code patterns/technologies.
2. Design patterns: Use appropriate design patterns (if it helps), after completely understanding the problem and context.

3. Coding best practices

1. No hard coding, use constants/configuration values.
2. Group similar values under an enumeration (enum).
3. Comments – Do not write comments for what you are doing, instead write comments on why you are doing. Specify any hacks, workaround, and temporary fixes. Additionally, mention pending tasks in your to-do comments, which can be tracked easily.
4. Avoid multiple if/else blocks.
5. Use framework features, wherever possible instead of writing custom code.

4. Non Functional requirements

a) Maintainability (Supportability) – The application should require the least amount of effort to support in near future. It should be easy to identify and fix a defect.

1. Readability: Code should be self-explanatory. Get a feel of story reading, while going through the code. Use appropriate names for variables, functions, and classes. If you are taking more time to understand the code, then either code needs refactoring or at least comments to have to be written to make it clear.
2. Testability: The code should be easy to test. Refactor into a separate function (if required). Use interfaces while talking to other layers, as interfaces can be mocked easily. Try to avoid static functions, singleton classes as these are not easily testable by mocks.
3. Debuggability: Provide support to log the flow of control, parameter data, and exception details to find the root cause easily. If you are using Log4Net like component then add support for database logging also, as querying the log table is easy.
4. Configurability: Keep the configurable values in place (XML file, database table) so that no code changes are required if the data is changed frequently.

b) Reusability

1. DRY (Do not Repeat Yourself) principle: The same code should not be repeated more than twice.
2. Consider reusable services, functions, and components.
3. Consider generic functions and classes.

c) Reliability – Exception handling and cleanup (dispose of) of resources.

d) Extensibility – Easy to add enhancements with minimal changes to the existing code. One component should be easily replaceable with a better component.

e) Security – Authentication, authorization, input data validation against security threats such as SQL injections and Cross-Site Scripting (XSS), encrypting the sensitive data (passwords, credit card information, etc.)

f) Performance

1. Use a data type that best suits the needs such as StringBuilder, generic collection classes.
2. Lazy loading, asynchronous and parallel processing.
3. Caching and session/application data.

g) Scalability – Consider if it supports a large user base/data? Can this be deployed into web farms?

h) Usability – Put yourself in the shoes of an end-user and ascertain, if the user interface/API is easy to understand and use. If you are not convinced with the user interface design, then start discussing your ideas with the business analyst.

5. Object-Oriented Analysis and Design (OOAD) Principles

1. Single Responsibility Principle (SRS): Do not place more than one responsibility into a single class or function, refactor into separate classes and functions.
2. Open Closed Principle: While adding new functionality, existing code should not be modified. New functionality should be written in new classes and functions.
3. Liskov substitutability principle: The child class should not change the behavior (meaning) of the parent class. The child class can be used as a substitute for a base class.
4. Interface segregation: Do not create lengthy interfaces, instead of split them into smaller interfaces based on the functionality. The interface should not contain any dependencies (parameters), which are not required for the expected functionality.
5. Dependency Injection: Do not hardcode the dependencies, instead inject them.

In most cases, the principles are interrelated, following one principle automatically satisfies other principles. For e.g: if the ‘Single Responsibility Principle is followed, then Reusability and Testability will automatically increase.

In a few cases, one requirement may contradict another requirement. So need to trade-off based on the importance of the weight-age, e.g. Performance vs Security. Too many checks and logging at multiple layers (UI, Middle tier, Database) would decrease the performance of an application. But few applications, especially relating to finance and banking require multiple checks, audit logging, etc. So it is ok to compromise a little on performance to provide enhanced security.

Tools for Code Reviews

1. The first step while assessing the code quality of the entire project is through a static code analysis tool. Use the tools (based on technology) such as SonarQube, NDepend, FxCop, TFS code analysis rules. There is a myth that static code analysis tools are only for managers.
2. Use plug-ins such as Resharper, which suggests the best practices in Visual studio.
3. To track the code review comments use the tools like Crucible, Bitbucket, and the TFS code review process.

Thanks TO: SURENDER REDDY GUTHA & Chirag Goel