A few years back I composed an extensive checklist for things to
consider while reviewing code. I started compiling this list after I saw that
many code reviews were not as effective as it could be.
Each of the reviewers focused on things they believed were important (which is, of course, individual) or mainly commented on cosmetics (the easiest things to spot).
Lately, I was asked by the unit director to give my suggestion for "TOP 5 Effective code reviewing checklist". The first thing that came to my mind was "how is that even possible?" Reflecting on my past checklist, I was certain that are so many things worth considering. However, "if there are more than five items in the list", they said, "no one will follow it". And they had a point. No one was using my checklist. Not even me…
Each of the reviewers focused on things they believed were important (which is, of course, individual) or mainly commented on cosmetics (the easiest things to spot).
Lately, I was asked by the unit director to give my suggestion for "TOP 5 Effective code reviewing checklist". The first thing that came to my mind was "how is that even possible?" Reflecting on my past checklist, I was certain that are so many things worth considering. However, "if there are more than five items in the list", they said, "no one will follow it". And they had a point. No one was using my checklist. Not even me…
After contemplating, I realized that I can summarize almost everything required
for an effective code reviewing in this following TOP 3 questions
1.
IS THE CODE WELL
TESTED?
I'm not a fan of Test Driven Development (TDD) methodology, at least not in
its extreme (Writing only the code that passes a single test). Instead, I suggest something else: Why
not start the review by reviewing the tests first? You can call it: Test
Driven Code Review.
This question forces the reviewer not only to understand what the reviewed feature is supposed to do, but to perform the review in light of the requirements. This is important especially in teams in which not every programmer knows every feature in the software. Sadly, It is easy to review lines of code without understanding the bigger picture, and relying on the programmer that he (or she) adresses the requirements. Moreover, as it is easy to write procedures that test the written code (and not the requirements), it is easy to review tests in light of the existing code. This is handled when reviewing the tests before reviewing the code (and can help in preventing fixation and bias on part of the reviewer).
Secondly, I cannot stress enough that tests are the most important part of software development, in my opinion (I will save that for another post…). As such, it only prompts to be checked first, with the benefit of the reviewer being fresh and sharp in thought, rather than reviewing it last when the reviewer might feel tired and that most of the review is behind him/her. I truly believe that having the programmer know that the tests are the main focus of the review will also make a strong effect on the programmer.
Lastly, a common say is that "it is hard to maintain the tests". It can be true, but we should not cave to this… We need to demand the same high quality from our tests as we demand from the source code, mainly - easy to extend or modify. Therefore, it is important that the test code will be treated the same as the source code when we come to review it. Thus, readability and usability are important for the test code as well.
This question (that is, is the code well tested) along with automated build and code coverage, guarantee that
I'm not a fan of Test Driven Development (TDD) methodology, at least not in
its extreme (Writing only the code that passes a single test). Instead, I suggest something else: Why
not start the review by reviewing the tests first? You can call it: Test
Driven Code Review.This question forces the reviewer not only to understand what the reviewed feature is supposed to do, but to perform the review in light of the requirements. This is important especially in teams in which not every programmer knows every feature in the software. Sadly, It is easy to review lines of code without understanding the bigger picture, and relying on the programmer that he (or she) adresses the requirements. Moreover, as it is easy to write procedures that test the written code (and not the requirements), it is easy to review tests in light of the existing code. This is handled when reviewing the tests before reviewing the code (and can help in preventing fixation and bias on part of the reviewer).
Secondly, I cannot stress enough that tests are the most important part of software development, in my opinion (I will save that for another post…). As such, it only prompts to be checked first, with the benefit of the reviewer being fresh and sharp in thought, rather than reviewing it last when the reviewer might feel tired and that most of the review is behind him/her. I truly believe that having the programmer know that the tests are the main focus of the review will also make a strong effect on the programmer.
Lastly, a common say is that "it is hard to maintain the tests". It can be true, but we should not cave to this… We need to demand the same high quality from our tests as we demand from the source code, mainly - easy to extend or modify. Therefore, it is important that the test code will be treated the same as the source code when we come to review it. Thus, readability and usability are important for the test code as well.
This question (that is, is the code well tested) along with automated build and code coverage, guarantee that
a.
The feature does
what it's supposed to do.
b.
The feature doesn't
do what it's not supposed to do.
c.
The code does not
break existing features.
* Note that there are different levels of
tests that need to be considered: unit, component, integration, performance and
load tests and each level should receive the proper attention
2.
IS THE PRODUCT EASY
TO MAINTAIN?
This question addresses issues of support and maintenance after the feature
is deployed, along with the entire product. I think this point doesn't get
the proper attention as it deserves. I suggest the reviewers ask:
This question addresses issues of support and maintenance after the feature
is deployed, along with the entire product. I think this point doesn't get
the proper attention as it deserves. I suggest the reviewers ask:
a.
Is the flow
understandable from the logs? I recommend reviewing actual log files and not
only loggings in the code
b.
How well are errors
handled?
c.
What alerts are
possible?
d.
Is it easy to
monitor and control the feature?
3.
IS THE CODE EASY TO
MAINTAIN?
Since we already know that changes are imminent, we need to verify that new
code is easy to extend, change or fix, even by a different programmer than the one
who coded it. This requires us to check its readability and design via means of:
Since we already know that changes are imminent, we need to verify that new
code is easy to extend, change or fix, even by a different programmer than the one
who coded it. This requires us to check its readability and design via means of:
a.
Coding conventions.
b.
Design matrices, such as: coupling and cohesion.
c.
Proper documentation.
d. Readability, meaningful comments
(that explains why rather than what).
These are my TOP 3 recommendations for an effective code review. But, since
the world is not perfect, and since our tests will never cover everything, I
suggest two additional (secret) bullets:
4.
DOES THE FEATURE
WORK?
In my original checklist, this was my first and most important bullet. The
reason why it is only fourth now (and secret) is that, intuitively, this bullet is
supposed to be answered by the first bullet- if the feature is well tested and
the tests cover the functional requirements, then the feature works. But, in
practice, the tests will be incomplete and another level of quality assurance
is required. Reviewing the code itself, seeking for bugs and making sure that the
code covers all functional requirements, provides that additional assurance.
Moreover, some of the bugs are really easy to spot in the code, yet are very
hard to test (for instance, concurrency problems).
In my original checklist, this was my first and most important bullet. The
reason why it is only fourth now (and secret) is that, intuitively, this bullet is
supposed to be answered by the first bullet- if the feature is well tested and
the tests cover the functional requirements, then the feature works. But, in
practice, the tests will be incomplete and another level of quality assurance
is required. Reviewing the code itself, seeking for bugs and making sure that the
code covers all functional requirements, provides that additional assurance.
Moreover, some of the bugs are really easy to spot in the code, yet are very
hard to test (for instance, concurrency problems).
5.
DOES THE FEATURE
HAVE SIDE EFFECTS?
While conducting a code review it is tempting to review only the lines of
code that changed, rather than all the code parts that relate to the changed
code. This is mostly because we use comparison tools to perform the code review
which results in ignoring the related code that did not change.
If the entire software is tested properly, then this question is supposed to be covered too. In addition, well designed code will reduce unwanted side effects of code altering. Otherwise, we should check how the feature interacts with existing features, for instance:
While conducting a code review it is tempting to review only the lines of
code that changed, rather than all the code parts that relate to the changed
code. This is mostly because we use comparison tools to perform the code review
which results in ignoring the related code that did not change.If the entire software is tested properly, then this question is supposed to be covered too. In addition, well designed code will reduce unwanted side effects of code altering. Otherwise, we should check how the feature interacts with existing features, for instance:
a.
Do other features
that use the altered code still work?
b.
Do assumptions
change?
c.
Does the product's
performance change?
To summarize my checklist for an effective code reviewing process:
1.
IS THE CODE WELL TESTED?
2.
IS THE PRODUCT EASY TO
MAINTAIN?
3.
IS THE SOFTWARE EASY TO
MAINTIN?
4.
(DOES THE FEATURE WORK?)
5.
(DOES THE FEATURE HAVE
SIDE EFFECTS?)
Have a happy and productive code review!
