Yesterday I blogged about my opinions on heavily commented code. It was controversial, and I received some good (differing) opinions.
Something that intrigued me was the voting pattern on reddit for my post, +11, -10. Obviously people agreed with me, it was just the people who disagreed who really thought they should vocalize their opinion...Anyways, enough with that.
Today, I want to talk about the famous TODO comments.
These are comments that are usually placed in by a developer during an initial development cycle, and then are never touched again, something like this painfully contrived example:
public float GetAverage(List<float> input)
{
// TODO: Check to see if casting to an int causes precision errors.
int totals = 0;
foreach (float f in input)
totals += (int)f;
return totals / input.Count;
}
While the developers intentions are clear here, they are usually far too busy to ever take another look at their TODO comments unless there is a defect filed.
However, .NET provides a feature called attributes, and Java provides a similar feature called annotations, and we can use these to automate analysis on TODO comments.
[Todo("This method loses precision by casting to int. This needs to be fixed")]
public float GetAverage(List<float> input)
{
int totals = 0;
foreach (float f in input)
totals += (int)f;
return totals / input.Count;
}
I created a custom attribute named TodoAttribute that accepts a string in its constructor.
class TodoAttribute : System.Attribute
{
public TodoAttribute(string message)
{
Message = message;
}
public readonly string Message;
}
Using this attribute, the developer can mark methods, properties, and classes with TODO comments, but they will no longer get lost in the noise, because it is trivially easy to create an analysis tool that can automatically generate a report during each build cycle.
To demonstrate this, here is some example code that dumps out the TODO attributes on methods on the current assembly:
foreach (Type t in Assembly.GetExecutingAssembly().GetTypes())
{
foreach (MethodInfo m in t.GetMethods())
{
object[] attributes = m.GetCustomAttributes(typeof(TodoAttribute), false);
if (attributes != null)
{
foreach (object todo in attributes)
{
Console.WriteLine(t.Name + "::" + m.Name + " TODO: " + ((TodoAttribute)todo).Message );
Console.WriteLine();
}
}
}
}
Running this example program gives me the following output:
Program::RandomIP TODO:
This method needs to be checked over to ensure that it creates
valid random IP's
Program::GetAverage TODO:
This method loses precision by casting to int. This needs to be fixed
Now imagine extending this to produce a nicely formatted analysis report, and integrating it with your continuous integration build system. Imagine how useful that would be for the dev lead in estimating the true project status.
Why stop at TODO Attributes? Why not FIXME, or FIXEDIN attributes to replace the typical comments, a trivial tool could be generated to check the FIXEDIN attributes and ensure that the defect ID's matched the ID's committed into source control. This prevents all kinds of last minute nightmares when the deadline is tight.
All of these practices help ensure a quality product when working with large teams, and are in no way gospel, just some suggestions from my experiences.
Posted by Jonathan Holland on 2/5/2009.