A true master craftsman of any profession takes pride in doing a great work. A masterpiece is recognized by the combination of a great total impression and worked out details. Most programmers tend to think that their latest code is a masterpiece. I even went as far as starting this blog, to show off all the masterpieces of code I write ;-).
The sad truth is that looking thoroughly at some code it’s often far from the carefully worked out details of a true master piece. Let’s look at an example of how code can look and try to get it right (according to my definition of right).
public class Car { [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1709:IdentifiersShouldBeCasedCorrectly", MessageId = "reg")] public string regNo { get; set; } public string Brand { get; set; } /// <summary> /// Drive the car /// </summary> /// <param name="speed">Speed to drive at</param> public void Drive() { Debug.WriteLine("Driving car!"); } public void DragRace(DragStrip strip) { while (!strip.AtEnd) { try { Drive(); } catch (Exception) { } } } } |
This is not real code from a project (if it was, I would need stress therapy), but I’ve seen all of these examples in live code. If I found this during code review, there would be a number of things I would point out.
My Code Review Comments
- The indentation is messed up, making the code harder to read.
- There are extra line breaks, making less code fit on screen at once.
- There is a suppressed code analysis warning (someone at least did run code analysis) that should have been fixed instead.
- There are other code analysis warnings that are not handled.
- Documentation comments are missing and where existing they are wrong.
- There should probably be some other comments explaining behaviour.
A junior developer who got that list would have to get back to the code and rework it. Here’s an idea of what the updated suggestion might look like.
/// <summary> /// Car class. /// </summary> public class Car { /// <summary> /// RegNo /// </summary> public string RegNo { get; set; } /// <summary> /// Brand /// </summary> public string Brand { get; set; } /// <summary> /// Drive the car /// </summary> public void Drive() { // Write driving car message. Debug.WriteLine("Driving car!"); } /// <summary> /// Drive the car /// </summary> /// <param name="strip">Dragstrip</param> [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes")] public void DragRace(DragStrip strip) { // Continue to drive until at end. while (!strip.AtEnd) { try { Drive(); } catch (Exception) { } } } } |
It definitely looks better and now compiles without warnings. Still there are some issues with the code.
- The documentation comments doesn’t tell anything that is not already evident from the code. The summary for
DragRacelooks like a direct copy fromDrive. - The other comments added doesn’t add any information not easily read from the code. There are also other cases where an explanation would make sense that does not have any comments.
My Idea of Correct
The final listing of this post is my idea of what the correct code should look like.
/// <summary> /// A car. /// </summary> public class Car { /// <summary> /// The registration number of the car. Is unique to each car. This /// field should never be null. /// </summary> public string RegNo { get; set; } /// <summary> /// The brand of the car, if available. Set to null if unknown. /// </summary> public string Brand { get; set; } /// <summary> /// Drive the car normally. /// </summary> public void Drive() { Debug.WriteLine("Driving car!"); } /// <summary> /// Drag race with the car, ignoring any problems during the race - focus /// on winning. /// </summary> /// <param name="strip">The dragstrip that the race is run on.</param> [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes")] public void DragRace(DragStrip strip) { while (!strip.AtEnd) { try { Drive(); } catch (Exception) { // Silently ignore any exceptions during racing. Just focus on // winning. The car will need a major service after the // race anyways. } } } } |
The updated code now has documentations that add information not already present in the code. For the string properties there is some clear information on null handling that is useful for someone using this code. Getting a popup from intellisense each time the Brand property is used with a reminder about null increases the possibility that null will be handled gracefully.
There is also now a comment explaining why exceptions are caught and silently ignored. Whenever anything is done that is against normal best practices, there should be a comment clearly stating why.
The last code sample is right according to my definition of right. Is it right according to your definition?
How about changing the properties to
{ get; private set; }and inject the values in the constructor? This way you can verify that the notnullrequirement of the RegNo property is fulfilled in the constructor. You can also guard your car from evil users trying to change your RegNo or Brand.