Welcome to Part 2) of the BSP Software ‘Code Review’ series
If you missed Part 1) Author and Reviewer Preparation, catch up here
Code Review Aspects
Peter Gabris, MD of BSP Software Inc
The following list of aspects the reviewer should be looking for is not complete; it could not be. Each platform, programming language, environment and application type has its own traps. This list is meant to inspire the reviewer only.
Readability
The code that is hard to read can easily hide dangerous bugs. Clarity of the code reflects the clarity of the author’s mind and his understanding of the problem. Coding standards can provide a good guidance. Coding standards violations should not get into the review – there is nothing to discuss. Send violations via email to the author, wait for the author to re-submit the code.
Frequently the project includes modifications to an existing (production) code. If that code is hard to read it is hard and risky to modify too. While it is not practical to rewrite the whole code, it is useful and efficient to clear and improve the program segment (procedure, function) we intend to modify. Instead of increasing the complexity of the segment it is usually better to split it into smaller segments, each serving a single well defined purpose. The time spent on such code clarification pays back on the time saved on the coding and testing changes and extensions.
Brevity is an important part of readability. Using the programming language features and separating repeated code into functions are tools to avoid verbosity. For example, the following code (a note about the code origin: nobody can see the log is his own code so I had to use somebody else’s speck. Luckily, I have no clue whose code it is. Moreover, I show it here because the copy/paste practice is nothing special):
if ((11 – reader[0].ToString().Length) > 0)
{
finderNumber = reader[0].ToString().PadRight(11, ‘ ‘);
}
else if (reader[0].ToString().Length == 11)
{
finderNumber = reader[0].ToString();
}
else if (reader[0].ToString().Length > 11)
{
finderNumber = reader[0].ToString().Remove(11);
}
if ((15 – reader[1].ToString().Length) > 0)
{
acsid = reader[1].ToString().PadRight(15, ‘ ‘);
}
else if (reader[1].ToString().Length == 15)
{
acsid = reader[1].ToString();
}
else if (reader[1].ToString().Length > 15)
{
acsid = reader[1].ToString().Remove(15);
}
if ((10 – reader[2].ToString().Length) > 0)
{
locationCode = reader[2].ToString().PadRight(10, ‘ ‘);
}
else if (reader[2].ToString().Length == 10)
{
locationCode = reader[2].ToString();
}
else if (reader[2].ToString().Length > 10)
{
locationCode = reader[2].ToString().Remove(10);
}
// and so on..
Is much more readable as:
finderNumber = StringToFixedLength( reader[ 0 ].ToString(), 11 );
acsid = StringToFixedLength( reader[ 1 ].ToString(), 15 );
locationCode = StringToFixedLength( reader[ 2 ].ToString(), 10 );
// and so on..
Just for the completeness, here is the function we extracted:
string StringToFixedLength( string str, int length )
{
return str.PadRight( length ).Substring( 0, length );
}
Code Review continues in Part 3) Structure
Feel free to leave any questions or comments below