Effective Code Review

Prevention is better then cure.

This document does not gives you steps to do code review, it tells you some thoughts on how to improve your code review skill.

Why should I do code review, anyways it will be tested?

Yes, it is the cost of fixing the bug increases with time and different stages of software life cycle.

And many times we cant find some issues in our testing, till that special condition executes.

What is code review?

It is not an approval process. It is a mindset to find defect. It is a process of early testing of your software.

Does every code has problem? Below are two big misconception you may have.

• The code is written by an expert/experienced developer, there should not be any defect.

• I am a new developer, how can I find bugs in code written by an expert.

We are human not god….

Even code written by god may fail…

because it has to run in OS written by human…

Remember : Always there is a scope of improvement in code. Every code, document, design document has scope of improvement.

Who can review and how?

• Self review : a self check on your own code

    Pre-requisite - make sure code is compilation error free. Else you may deviate towards error.

    How to do? Is the functionality implemented? If yes, stop coding, take a break and do review.

    “When review, do not fix, just review and find issues.”

• Peer review : review within the team

     Pre-requisite – make sure code is compilation error free, unit tested and do static code analysis (lint).

     1) Author to make sure Reviewer understand the functionality.

     2) Provide design overview and list of test cases

     3) Agreement between Reviewer and Author on comments. Reviewer should be notified.

• Team review - review it together in a meeting room

     1) Team review require more effort, should be done for complex and bigger code changes or limit the team size

     2) Author should explain code to team over a meeting room or online. Note each comment and send over email.

• Architect review - a bigger or important code changes should go to architects for review.

Principles of code review:

• Reviewer -> Consider it as important task, assign time.

  Authors -> Don’t force the reviewer to finish it fast, when you took lot of time to write it.

• If you commit to review code, review it thoroughly!

    Be sure to read the code, don't just skim it, and apply thought to both the code and its style.

• Aim to understand every changed line

     Simple and efficient code, complex should be refactored,

• Don't assume the code works – if needed build and test, cross check behavior from design

• Comments/Documentation –intent of comment should match the logic

• Consider how the code will work in production.

• Keep priorities straight when making suggestions : Functional first, Clean and maintainable second, and Optimization third.

• Reviewers are not perfect! : Author should discuss with reviewer on each comment which he/she is not agreeing with.

What kind of review I can suggest?

• Crash or fatal issues - hang, infinite loop, invalid pointer access

• Major - which can impact the functionality and does a major damage.

• Minor - small issues like else check, missing log, improper comments, dead code.

• Suggestion - good to have (readability), small code restructure, cleanup, naming convention.

• Query - remember, code review is a way to improve your knowledge on the module.

Do not think too much on which bucket it will fall, it gives you another aspect of thinking.

Areas to look at

• Functional – the most tough part for a reviewer.

1. How can we achieve good review, when reviewer is unaware of the functionality?
2. Author should explain the functionality, entry points, high level code flow. If more amount of code, set a meeting with reviewer.
3. Is the code inline with requirement or design?

• Technical aspect – (online free blogs/posts/books/language FAQ)

1. General guideline (missing else condition, loop ending, naming convention, comments, performance improvement)
2. Language specific (pointer initialisation and cleanup, memory cleanup, casting issue, exception handling)

• Non-Functional Requirements – does the code can impact performance? Suggest testing, point out code changes for performance improvement, look at securability, interoperatability, integration aspects, affect on upgrade, test cases covered.

• Beautification of code – Its working code, why to beautify? 

What to look, when code changes are small?

• Big picture – not just if condition, see the impact on the function and its caller.

• Look at the code flow which can hit the code changes.

• Map the code flow into use cases or unit test cases.

• Review the static code analyser report related to code changes.

• Most important – don’t take it easily, a major bug can be introduced with a simple code change.

• A small change can also impact performance, securability or interoperatability issue. Consider non-functional requirements for small changes also.

• Look at the loops around the code changes.

• Remember, every small piece of code can have improvement area.

What to look, when code changes are big?

• Start or entry point

• A quick meeting with author to understand the flow and requirement

• Good code editor(function list, attribute list, package structure in code editor UI)

• Be expert in RegEx and aware of search features in the editor

• Refer design to check if code is inline with requirement

• Do not forget to review major issues in the static code analyser report

•Why not review test cases before the code review.

• Enough logging to debug an issue in future? Of course appropriate log messages and comments.

• Still low confidence on functional understanding – no problem, suggest team review or another developer.

• Look at the limitations of external entity(third party library/ operating system/ etc)

• No assumptions please.

CO - LO - CO View

Note: This is my own theory, but believe me it is very effective. Compare the below three pictures.

Code View: Complete view with comment and logs. Make sure code is inline with comment and logs.

Logging View: Make sure logging is adequate to understand the flow and can map with code.

Comment View : Make sure when you read the comments, you should be able to understand the complete behavior of the function without looking at the code.

I have few more topics to cover, will be updating the document soon.

1. Eagle Eyes - This is an idea to look the impact of a small code changes in higher level(callers and callers of caller).
2. Common Mistakes in coding guideline - many coding guidelines are available on internet, but there are common mistakes we do. So we need to look at the common guidelines across different languages.

I hope you enjoyed the reading..... please comment....

Disclaimer : Images are copied from internet.