At some point in your software engineering career you have to do some sort of code review. You should review your own code but most often you do a review of someone else’s code. I thought I’d share a few of the things that I consider while reviewing code. I have reviewed code mostly on C#, VB, html, JavaScript and SQL. I can share the process and some of the things that I have in mind during a code review. Some of these considerations have grown out of what I’ve seen that causes errors either immediately or down the road or makes code hard to read and understand to other developers.
There is a saying that sooner or later one has to pay the piper. Writing code that is not only efficient but also readable can save you a lot of time in the long run and I would say that the benefits can be exponential when you’re part of a team.
Some of the things to consider:
- Does the implementation meet the requirements of the business? It may seem basic but this can sometimes be missed if the initial definition was not clear. It’s been said that a well-defined problem is half the solution.
- Use intelligent variable names; same applies to database objects and columns. “Id” means nothing unless it’s only used locally. The subject of variable names can in itself turn in to a heated argument among developers. I tend to prefer Hungarian notation, it lets you know the datatype right up front.At least be consistent with your conventions.
In General, and especially in SQL, avoid using data types as variable or column names. Consider this example: Paste this line into an SQL query analyzer.
SELECT
DATEADD(m, 1, CAST(CAST(Month AS VARCHAR) + '/' + CAST(5 AS VARCHAR) + '/' + CAST(Year AS VARCHAR) AS datetime)) AS Date
FROM
MyTable
You might be a bit confused at first. What is odd here?
Answer: Month and Year are columns in a given table. Column names are not colorized but in this case they are also colorized because they are MS-SQL functions.
SELECT YEAR('2012-05-08T01:01:01.1234567-07:00');
Returns the integer 2012. Note also that MS-SQL is not case-sensitive and, unlike MySQL, the terminating semicolon is optional.
It’s not a good idea to use a function name as a column name. If you encounter this you can at least make it more readable by enclosing the object name, in this case columns, with square brackets as such (note here they are still colorized but they should not be in a query analyser):
DATEADD(m, 1, CAST(CAST([Month] AS VARCHAR) + '/' + CAST(5 AS VARCHAR) + '/' + CAST([Year] AS VARCHAR) AS DATETIME)) AS Date
- Include good comments in the code.
- Have good indentation. This can be achieved while coding or, if need be, reformat text. In Visual Studio is easy with Crtl+K, Crtl+D. Other text-based editors such as vi, Sublime Text, etc; may use plug-ins.
- Avoid repetition of logic or code anywhere. For example, use a common table for search results vs. having different table definitions if the search is from different sources.
- Create functions or classes for repetitive operations.
- Use consistent naming of variables, classes and namespaces.
- I prefer code that is readable over fancy contractions that makes it hard to debug if necessary.
- Avoid hard coding as much as possible. Use configuration files or other editable settings mechanisms as much as possible.
- Delete unused code. This will save you and others time when you do a global search plus will make your code neater. If you want to see obsolete code again for some reason, use source control or save it in a snippets file.
- This may seem very obvious but just in case, use variables for a single purpose only.
- Catch errors and log user-friendly messages. This will save you time if you need to debug errors. You can use Try / Catch structures in most languages. In .NET and similarly managed platforms there are error listeners, if you will, such as HttpApplication.Error that can be used to capture unhandled exceptions.
- Delete unused files of any type. Same applies to databases, deleted any obsolete objects. Less is more when it comes to code or objects.
- Look for any enhancements that will make the code faster or better in any way.
For example, use the StringBuilder class instead of String for repetitive string manipulation.
Use a Listinstead of array whenever possible. Many times an array is maxed out with 20 items even when not expected, for example variations of a given product can max out a hard coded array.
I ran into this back in February on code that required selecting distinct values from an array and found a more optimal way to do it based on language enhancements.
.NET Framework 1.0/1.1
public int[] GetDistinctValues(int[] array)
{
ArrayList list = new ArrayList();
for (int i = 0; i < array.Length; i++)
{
if (list.Contains(array[i]))
continue;
list.Add(array[i]);
}
return (int[])list.ToArray(typeof(int));
}
New way of doing it in .NET Framework 3.5
One line of code
nrs = nrs.Distinct().ToArray();
btw: This is illustrated in detail in this post.
http://weblogs.asp.net/gunnarpeipman/archive/2008/05/15/getting-distinct-values-from-arrays.aspx
Obviously some of these principles are different in on other languages such as C/C++ or Ruby but at least functionality, readability and consistency should be the same across languages.
I usually do some unit testing and performance testing as part of code review. If it’s web front end UI code it may mean using multiple browsers or Fiddler to check raw responses or performance as well as variant behavior on different browsers.
There are many other things that can be done to look for efficiency in processing and avoid memory leaks. I would say that if you follow some of these basic tips you’re off to a good start. I enjoy building code that do powerful and useful things. Software engineering should be enjoyable not frustrating.