Thought it would be good to include this article from my other blog (Brian Welch's TechBlog). Njoy!!!
A while back, my organization introduced code reviews...
I wanted to take the opportunity today to provide you with a snippet of the particular points of note while reviewing code (and why this is useful). The snippet is part of an e-mail I sent out to my fellow team members when we were initially establishing our code review process (in 2005). These principles still apply when reviewing code... so here goes:
Code Reviews
Code reviews are put in place to as a last quality check of the code before it goes into the released system.
Quality of code means:
Is the code in line with the detailed design and the existing architecture?
Generally, developed software applications are existing production systems that are in line with an existing architecture and technical design. These systems are supported by a production support team, that needs to be able to quickly respond to defects in systems, as well as develop enhancements requested by the business stakeholders. Changes to the architecture or technical design, slow the production support developers down in providing these changes timely, as there is a learning curve involved and the production support team is generally on a very tight (forthnightly) release schedule. Any change to the architecture or to the technical design of these systems must be approved by the Application Architect (in collaboration with the senior developers in your team).
Does the code adhere to the coding standards provided?
Coding standards may at time seem limiting in coding freedom, or may (when first adopting) slow a developer down a bit. Mostly, the reason not to have coding standards is: "It's too hard to think about trivialities such as formatting, naming and API documentation... "The logic is more important". Sadly, this is not really true... having consistently formatted code makes reading the code easier for everyone (not just the person who did the coding, but also for persons having to maintain the code afterwards). Most of the formatting can be done by IDE templates or special programs (e.g. XML-tidy or Checkstyle). However, the discipline comes into the picture when checking the code back into the source repository. Consistently styled code also makes differencing CVS and local code easier (much easier).
Is the code self explanitory (naming and structuring are key here)?
Code styling is part of a coding standard, but there are less automatable constructs that should also be part of a coding standard. Naming is a very important one... Code reviews should ensure that proper care is taken that classes, objects, methods, parameters etc... are named clearly (no exceptions). Reading code should be like reading the instructions in a cookbook.
1 e
1/2 on
1 clv glk
1 tsp btr
chp the glk and on, then bt together with the e
fry the btr in a pan, then add mxt,.Fry for 10s on both sds.
is a lot less clear than
1 egg
1/2 onion
1 clove of garlic
1 teaspoon of butter
Chop the garlic and onion, the beat them together with the egg
Fry the butter in a pan, then add the mixture. Fry for 10 seconds on both sides
The same goes for code... Care should be taken to keep the abbreviations to the minimum (except when these are business acronyms). And most of all, this should be enforced by the conding standard and policed through a review.
Strucururing of the code is almost as important. The key to this is that generally code becomes alot less readable when the logical collection of "actions" becomes greater. I.e. the more lines of code to read, the harder to focus on what the code actually does. As a rule of thumb, I use the "15 lines per method" rule. If a piece of code is more than 15 lines long, there is a 95% chance that the method that contains the code is doing more than it hints (through its name) it is doing, which means that there probably is a good chance that the code should split into more cohesive units (methods, sometimes classes etc... depending on the granularity). When this practise is kept (and only in combination with proper naming), the code becomes alot more clear. Of course it is hard to set a clear rule about this in a coding standard... and this becomes a suggestion to be enforced where appropriate... The code review is there to ensure that the code is broken up into "cohesive units" where appropriate
Patterns and Anti-Patterns
When reviewing code, care should also be taken that the code does not attempt to solve problems that were solved elsewhere. What this means is that if a common problem is solved with different code, where a "simpler/clearer" existing solution could be applicable. Existing could mean elsewhere in the system, or maybe a "well known solution" published in literature (e.g. Design Patterns, by Gamma et al, or Patterns of Enterprise Architecture, by Fowler). Of course this adds the responsibility of maintaining a "catalog" of well known, properly documented solutions for problems (patterns), from which developers could obtain these patterns. The reviewer of code should also make sure that if problems are solved in code that are solved elsewhere (and known to work there), the code is augmented/refactored to apply the pattern. Please note that this is not the same as reviewing the design as a whole... it is only a review of a self-contained piece of code that solves a problem that is solved (and documented) elsewhere. I.e. the reviewer is expected to point out where patterens could be applied. Recurring patterns make it easy for code maintainers to read the code.
It is just as important for a reviewer to spot code that solves a problem in a manner "known to have caused other issues in the past", and recommend that these anti-patterns be refactored to a safer solution (usually a pattern). There is alot of literature documenting anti-patterns (including Manning Publication's Bitter..., series). However, there are undoubtedly cases where anti-patterns are in-house application or framework specific. These anti-patterns should also be documented in a "catalog".
As a side note... code reviews could be a very good start to documenting in-house patterns and anti-patterns, because in the beginning, these will be identified by team members that have more knowledge of the code base (i.e. problems that were solved in the past, both patterns and anti-patterns). For the "younger" developers, they provide an opportunity to "highlight" areas in existing code that were refactored (or could be refactored) to "well known" patterns that are applied in the industry.
Providing useful API documentation and code comments
Another area of code review should be the providion of API documentation. API documentation is important in 3 ways:
- To provide the developer a "high level" overview of which fine grained components (classes and interfaces) are there and how they should be applied
- To provide the developer with corner conditions in which these fine grained components are guaranteed to work/not to work, where naming alone is not sufficient.
- To provide the developer with a brief overview of what each fine grained component's function is from a business point of view.
The reviewer of the code should make sure that where appropriate this documentation is provided.
The reviewer should also police that documentation and code comments are not used "in vain". In-vain documentation/comments are used for instance rewording variable names or function names.
EG
/**
* this method displays the premium
*/
public void displayPremium()
The comment used here is clearly useless, since the name of the method already explains what the code does
/**
* Only displays the premium if the associated
* risk is not declined, otherwise 0 is displayed
*/
public void displayPremium()
In the case above, comments are used to augment non-obvious functionality, namely that the premium is only displayed if the risk is not declined... this cannot be ascertained from the method name.
The reviewer should also make sure that proper methods are used instead of comments over statements
E.G.
...
// if disabled just show the value, no dropdown <-- comment over statement
if (getDisabled() &&amp; parent != null) {
String displayValue = (String) RequestUtils.lookup(
pageContext, name, property, null);
if(!GenericValidator.isBlankOrNull(this.codeTranslator)) {
CodeTranslator translator = (CodeTranslator) RequestUtils.lookup(
pageContext, this.codeTranslator, null);
displayValue = translator.translate(displayValue);
}
if(displayValue == null) {
displayValue = "";
}
ResponseUtils.write(pageContext, ResponseUtils.filter(displayValue));
return SKIP_BODY;
}
...
could be refactored into a new method
private void showValueOnly() {
String displayValue = (String) RequestUtils.lookup(
pageContext, name, property, null);
if(!GenericValidator.isBlankOrNull(this.codeTranslator)) {
CodeTranslator translator = (CodeTranslator) RequestUtils.lookup(pageContext,
this.codeTranslator, null);
displayValue = translator.translate(displayValue);
}
if(displayValue == null) {
displayValue = "";
}
ResponseUtils.write(pageContext, ResponseUtils.filter(displayValue));
}
this method could then be called without the need for the comment like so:
if (getDisabled() &&amp; parent != null) {
showValueOnly();
return SKIP_BODY;
}
Note that the simplicity of this construct already tells the story, there is no comment needed
Are the corner cases in the code well guarded (Exception handling)
What the revierw should also look for, are corner cases. Corner cases are cases in the code where method calls or variable assignments are not guaranteed to change the affected component(s) into a stable (well known) state. There are 2 ways to guard for this in the code:
With assertions, the code itsself checks that the parameters passed are guaranteed to bring the code into a valid state and once the affected component's state is changed assure that the state is valid. When the state is not the expected state an exception is thrown (application if at the application level or system when at the system level).
Pre- and postconditions do exactly the same, but at documentation (rather than implementation level). This can usually be done in the API documentation
When reviewing the code, the reviewer must see to it (to a reasonable degree) that the corner cases are covered in either of these was. The reviewer must also see to it that either method is used where most appropriate (within reasonable bounds). I agree that this is a grey area, but at least this should be discussed in the code review. The reviewer should also make sure that when assertions are used, the appropriate level exceptions are thrown. And in cases where code is called that throws exceptions they are properly handled (which includes forwarding). The reviewer must also insure that pre- and postconditions are observed (either handled or documented when ignored) by code that uses pre-/postconditioned functionality.
Do the unit tests test the main functionality provided by the developed unit?
Automated unit tests should test the functionality of a fine grained component. The reviewer should at least make sure that there are unit tests written for the changed or introduced components. At a more detailed level, the reviewer could also review the "isolation" of tests to make sure that tests do not rely on infrastructure, but are contained to test the unit (this speeds up the unit testing and also makes tests more reliable, since they do not rely on other code, that may or may not be working within specifications).
The reviewer will also need to verify (using the unit test logs), that the tests all pass AND have the appropriate coverage of the component tested.
Well, that was it for today.
Oh yes, and... we've finally managed to complete the "homegrown framework" change today... As was to be expected, we ran into pop-up after pop-up... but it's working... Would that we could have done it with Spring. Thank goodness the next few deliverables don't require "framework changes"
TTFN
(that is: Ta... Ta... For Now...)
A while back, my organization introduced code reviews...
I wanted to take the opportunity today to provide you with a snippet of the particular points of note while reviewing code (and why this is useful). The snippet is part of an e-mail I sent out to my fellow team members when we were initially establishing our code review process (in 2005). These principles still apply when reviewing code... so here goes:
Code Reviews
Code reviews are put in place to as a last quality check of the code before it goes into the released system.
Quality of code means:
- Is the code in line with the detailed design and the existing architecture?
- Does the code adhere to the coding standards provided?
- Is the code self explanitory (naming and structuring are key here)?
- Does the code attempt to solve problems that were solved elsewhere (i.e. were design patterns applied to solve common problems)?
- Does the code attempt to solve problems in a way that is known to cause other issues (i.e. were anti-patterns avoided)?
- Where the naming is not sufficient to describe the code, is proper API documentation provided?
- Are useless comments cluttering the code? (e.g. rewording of a something that is already explained by a name)
- Are the corner cases in the code well guarded (Exception handling)
- Do the unit tests test the main functionality provided by the developed unit?
Is the code in line with the detailed design and the existing architecture?
Generally, developed software applications are existing production systems that are in line with an existing architecture and technical design. These systems are supported by a production support team, that needs to be able to quickly respond to defects in systems, as well as develop enhancements requested by the business stakeholders. Changes to the architecture or technical design, slow the production support developers down in providing these changes timely, as there is a learning curve involved and the production support team is generally on a very tight (forthnightly) release schedule. Any change to the architecture or to the technical design of these systems must be approved by the Application Architect (in collaboration with the senior developers in your team).
Does the code adhere to the coding standards provided?
Coding standards may at time seem limiting in coding freedom, or may (when first adopting) slow a developer down a bit. Mostly, the reason not to have coding standards is: "It's too hard to think about trivialities such as formatting, naming and API documentation... "The logic is more important". Sadly, this is not really true... having consistently formatted code makes reading the code easier for everyone (not just the person who did the coding, but also for persons having to maintain the code afterwards). Most of the formatting can be done by IDE templates or special programs (e.g. XML-tidy or Checkstyle). However, the discipline comes into the picture when checking the code back into the source repository. Consistently styled code also makes differencing CVS and local code easier (much easier).
Is the code self explanitory (naming and structuring are key here)?
Code styling is part of a coding standard, but there are less automatable constructs that should also be part of a coding standard. Naming is a very important one... Code reviews should ensure that proper care is taken that classes, objects, methods, parameters etc... are named clearly (no exceptions). Reading code should be like reading the instructions in a cookbook.
1 e
1/2 on
1 clv glk
1 tsp btr
chp the glk and on, then bt together with the e
fry the btr in a pan, then add mxt,.Fry for 10s on both sds.
is a lot less clear than
1 egg
1/2 onion
1 clove of garlic
1 teaspoon of butter
Chop the garlic and onion, the beat them together with the egg
Fry the butter in a pan, then add the mixture. Fry for 10 seconds on both sides
The same goes for code... Care should be taken to keep the abbreviations to the minimum (except when these are business acronyms). And most of all, this should be enforced by the conding standard and policed through a review.
Strucururing of the code is almost as important. The key to this is that generally code becomes alot less readable when the logical collection of "actions" becomes greater. I.e. the more lines of code to read, the harder to focus on what the code actually does. As a rule of thumb, I use the "15 lines per method" rule. If a piece of code is more than 15 lines long, there is a 95% chance that the method that contains the code is doing more than it hints (through its name) it is doing, which means that there probably is a good chance that the code should split into more cohesive units (methods, sometimes classes etc... depending on the granularity). When this practise is kept (and only in combination with proper naming), the code becomes alot more clear. Of course it is hard to set a clear rule about this in a coding standard... and this becomes a suggestion to be enforced where appropriate... The code review is there to ensure that the code is broken up into "cohesive units" where appropriate
Patterns and Anti-Patterns
When reviewing code, care should also be taken that the code does not attempt to solve problems that were solved elsewhere. What this means is that if a common problem is solved with different code, where a "simpler/clearer" existing solution could be applicable. Existing could mean elsewhere in the system, or maybe a "well known solution" published in literature (e.g. Design Patterns, by Gamma et al, or Patterns of Enterprise Architecture, by Fowler). Of course this adds the responsibility of maintaining a "catalog" of well known, properly documented solutions for problems (patterns), from which developers could obtain these patterns. The reviewer of code should also make sure that if problems are solved in code that are solved elsewhere (and known to work there), the code is augmented/refactored to apply the pattern. Please note that this is not the same as reviewing the design as a whole... it is only a review of a self-contained piece of code that solves a problem that is solved (and documented) elsewhere. I.e. the reviewer is expected to point out where patterens could be applied. Recurring patterns make it easy for code maintainers to read the code.
It is just as important for a reviewer to spot code that solves a problem in a manner "known to have caused other issues in the past", and recommend that these anti-patterns be refactored to a safer solution (usually a pattern). There is alot of literature documenting anti-patterns (including Manning Publication's Bitter..., series). However, there are undoubtedly cases where anti-patterns are in-house application or framework specific. These anti-patterns should also be documented in a "catalog".
As a side note... code reviews could be a very good start to documenting in-house patterns and anti-patterns, because in the beginning, these will be identified by team members that have more knowledge of the code base (i.e. problems that were solved in the past, both patterns and anti-patterns). For the "younger" developers, they provide an opportunity to "highlight" areas in existing code that were refactored (or could be refactored) to "well known" patterns that are applied in the industry.
Providing useful API documentation and code comments
Another area of code review should be the providion of API documentation. API documentation is important in 3 ways:
- To provide the developer a "high level" overview of which fine grained components (classes and interfaces) are there and how they should be applied
- To provide the developer with corner conditions in which these fine grained components are guaranteed to work/not to work, where naming alone is not sufficient.
- To provide the developer with a brief overview of what each fine grained component's function is from a business point of view.
The reviewer of the code should make sure that where appropriate this documentation is provided.
The reviewer should also police that documentation and code comments are not used "in vain". In-vain documentation/comments are used for instance rewording variable names or function names.
EG
/**
* this method displays the premium
*/
public void displayPremium()
The comment used here is clearly useless, since the name of the method already explains what the code does
/**
* Only displays the premium if the associated
* risk is not declined, otherwise 0 is displayed
*/
public void displayPremium()
In the case above, comments are used to augment non-obvious functionality, namely that the premium is only displayed if the risk is not declined... this cannot be ascertained from the method name.
The reviewer should also make sure that proper methods are used instead of comments over statements
E.G.
...
// if disabled just show the value, no dropdown <-- comment over statement
if (getDisabled() &&amp; parent != null) {
String displayValue = (String) RequestUtils.lookup(
pageContext, name, property, null);
if(!GenericValidator.isBlankOrNull(this.codeTranslator)) {
CodeTranslator translator = (CodeTranslator) RequestUtils.lookup(
pageContext, this.codeTranslator, null);
displayValue = translator.translate(displayValue);
}
if(displayValue == null) {
displayValue = "";
}
ResponseUtils.write(pageContext, ResponseUtils.filter(displayValue));
return SKIP_BODY;
}
...
could be refactored into a new method
private void showValueOnly() {
String displayValue = (String) RequestUtils.lookup(
pageContext, name, property, null);
if(!GenericValidator.isBlankOrNull(this.codeTranslator)) {
CodeTranslator translator = (CodeTranslator) RequestUtils.lookup(pageContext,
this.codeTranslator, null);
displayValue = translator.translate(displayValue);
}
if(displayValue == null) {
displayValue = "";
}
ResponseUtils.write(pageContext, ResponseUtils.filter(displayValue));
}
this method could then be called without the need for the comment like so:
if (getDisabled() &&amp; parent != null) {
showValueOnly();
return SKIP_BODY;
}
Note that the simplicity of this construct already tells the story, there is no comment needed
Are the corner cases in the code well guarded (Exception handling)
What the revierw should also look for, are corner cases. Corner cases are cases in the code where method calls or variable assignments are not guaranteed to change the affected component(s) into a stable (well known) state. There are 2 ways to guard for this in the code:
- Assertions (i.e. exceptions)
- Pre- and postconditions
With assertions, the code itsself checks that the parameters passed are guaranteed to bring the code into a valid state and once the affected component's state is changed assure that the state is valid. When the state is not the expected state an exception is thrown (application if at the application level or system when at the system level).
Pre- and postconditions do exactly the same, but at documentation (rather than implementation level). This can usually be done in the API documentation
When reviewing the code, the reviewer must see to it (to a reasonable degree) that the corner cases are covered in either of these was. The reviewer must also see to it that either method is used where most appropriate (within reasonable bounds). I agree that this is a grey area, but at least this should be discussed in the code review. The reviewer should also make sure that when assertions are used, the appropriate level exceptions are thrown. And in cases where code is called that throws exceptions they are properly handled (which includes forwarding). The reviewer must also insure that pre- and postconditions are observed (either handled or documented when ignored) by code that uses pre-/postconditioned functionality.
Do the unit tests test the main functionality provided by the developed unit?
Automated unit tests should test the functionality of a fine grained component. The reviewer should at least make sure that there are unit tests written for the changed or introduced components. At a more detailed level, the reviewer could also review the "isolation" of tests to make sure that tests do not rely on infrastructure, but are contained to test the unit (this speeds up the unit testing and also makes tests more reliable, since they do not rely on other code, that may or may not be working within specifications).
The reviewer will also need to verify (using the unit test logs), that the tests all pass AND have the appropriate coverage of the component tested.
Well, that was it for today.
Oh yes, and... we've finally managed to complete the "homegrown framework" change today... As was to be expected, we ran into pop-up after pop-up... but it's working... Would that we could have done it with Spring. Thank goodness the next few deliverables don't require "framework changes"
TTFN
(that is: Ta... Ta... For Now...)
0 comments:
Post a Comment