April 26, 2009

Unnecessary comments must go

There are about as many commenting styles out there as there are programmers. Probably more. Some programmers believe that comments are pure goodness, and the more comments a codebase has, the better. Documentation is good, right? More comments mean more documentation.

Some programmers feel that comments just get in the way, and sometimes lie. They’re to be avoided, mistrusted, shunned even. Get that peanut butter out of my chocolate.
In his book Clean Code, Bob Martin says:
Indeed, comments are, at best, a necessary evil. If our programming languages were expressive enough, or if we had the talent to subtly wield those languages to express our intent, we would not need comments very much – perhaps not at all.
I tend to agree.

He goes on to categorize comments as Good and Bad. Good comments being those that express legal terms, those that are truly informative, explain intent, clarify code that is otherwise tricky to understand, warnings, TODOs, or comments that amplify the importance of a piece of code that may otherwise seem inconsequential. In my experience, those types of comments (with the exception of Legal comments) are actually pretty rare.

Bad comments are those that are redundant (the comment says practically what the code says), misleading, mandated (like documentation comments for every method), journals (who edited what, and when),  noisy (like decorating the default constructor with code that says “this is the default constructor”), position markers (“Properties here”, “Methods here”), attributions (“Eric did this”), commented out code, and such.

Here’s what it boils down to for me: I want to read code, not comments. It’s the code that gets executed, not the comments. So when I’m looking at some code, the comments just get in the way.

If I ever get the itch to add a clarifying comment, I try to step back and see if there’s a way I can provide clarification in the code itself. Is the method really long? Then break it up into smaller chunks. Can the variables be named better? How about the method name itself?

The vast majority of the time, it turns out that by writing the code a bit differently, a comment is not actually needed.

So, in my ongoing experiment with refactoring BlogEngine.NET, one of my first acts when I open a class file is to cull the unnecessary comments. Cut them out. With the comments out of the way, I can actually see the code, and figure out what’s going on under the hood.

An example, from the Post.cs class:

   29         #region Constructor
   31         /// <summary>
   32         /// The default contstructor assign default values.
   33         /// </summary>
   34         public Post()
   35         {
   36             base.Id = Guid.NewGuid();
   37             _Comments = new List<Comment>();
   38             _Categories = new StateList<Category>();
   39             _Tags = new StateList<string>();
   40             DateCreated = DateTime.Now;
   41             _IsPublished = true;
   42             _IsCommentsEnabled = true;
   43         }
   45         #endregion

The “default constructor” comment is just clutter. It’s got to go. Nobody who could possibly understand what this code is doing would fail to understand that it is the default constructor. The comment just gets in the way. And, as an aside, did you catch the misspelling?

I remove the evil code-hiding region markers also.

Another example, from Utils.cs:

  102         /// <summary>
  103         /// Removes the HTML whitespace.
  104         /// </summary>
  105         /// <param name="html">The HTML.</param>
  106         public static string RemoveHtmlWhitespace(string html)
  107         {
  108             if (string.IsNullOrEmpty(html))
  109                 return string.Empty;
  111             html = REGEX_BETWEEN_TAGS.Replace(html, "> ");
  112             html = REGEX_LINE_BREAKS.Replace(html, string.Empty);
  114             return html.Trim();
  115         }

Do I need a comment to tell me that the RemoveHtmlWhitespace() method removes the HTML whitespace?

And there’s a whole slew of these, too:

   21         private Guid _Id;
   22         /// <summary>
   23         /// The Id of the comment.
   24         /// </summary>
   25         public Guid Id
   26         {
   27             get { return _Id; }
   28             set { _Id = value; }
   29         }

When the (completely unnecessary) comments are removed and Auto-Implemented Properties are employed, the above code turns into:

   10  public Guid Id { get; set; }

So much better, isn’t it?

April 25, 2009

Scottsdale Awarded 5 PTI Technology Solutions Awards

Sweet. The City of Scottsdale, where I work, was awarded several of the Public Technology Institute’s annual Technology Solutions Awards. We won first place in two categories, and received “Significant Achievement” awards for three other categories. That’s five winning entries in the ten possible categories. I think it’s quite an achievement.

I was personally involved in two of the winning projects, and peripherally related to the others. You can read the city’s official press release here.

One of the winning projects is titled “Intranet SOA Portal Architecture”, and the write-up describes the ongoing effort of the past year or two to upgrade our custom-written applications infrastructure. Our team has put forth some significant effort to move from many individual classic ASP and .NET 1.1 applications to a more modern .NET 3.5 infrastructure with a service-oriented codebase with a significant amount of code shared among applications. This is what I spend most of my time working on.

One of the Significant Achievements went to our “Application Development Team Incorporates Agile Development and Project Management” entry. Changing our software project management processes and philosophy was a significant contributing factor to our success with our “Intranet SOA Portal Architecture”. And, as everybody knows, change is hard. Especially hard in government. But, our customers (other city departments and the citizenry) have been nearly unanimously happy with our scrum-like process and the quality of the software being developed.

Clearing out the junk drawer

This is the first post in a series that I’m going to write while refactoring the BlogEngine.NET source to better match my own personal coding style. I’m going to be dogfooding my changes, publishing them to this site as I go.

Before I start I want to make it clear, though, that I have a lot of respect for the developers of BlogEngine. The changes I’m going to make and the comments that go along with them are not meant to be negative criticisms of their work. Rather, I consider this an exercise in expressing my coding styles. As I change things around, I mean no disrespect to Mads or Al or any of the other contributors. They’re doing a great job.

Having said that, one of the first things I noticed about the organization of the source code was the existence of a “Utils.cs” file. I’m always very wary whenever I see such a file, because often it functions like a junk drawer, collecting all of the miscellaneous functionality that doesn’t fit in any other obvious place. That’s not necessarily bad, but it can easily balloon out of control, with much of the commonly used core functionality being stuffed in this one file indiscriminately. This can make it hard to see where various pieces of functionality exist, as they all get buried together in the same drawer.

In this case, the Utils.cs file contains functionality ranging from string manipulation (RemoveIllegalCharacters, RemoveExtraHyphen, etc.), to sending email (SendMailMessage, SendMailMessageAsync, etc.), to password hashing and semantic discovery and code assembly discovery.

One of the first things I did was to begin separating the various type of functionalities into individual classes that are more single purpose. This more closely follows the basic concept of the Single Responsibility Principle. For organization, I decided to place some of the extracted classes into a folder called “Agents”, with classes named TextAgent, MailAgent, and so on.

For me, this forms a much better organizational structure for some of the commonly used “utility” functionality.

April 22, 2009

BlogEngine.NET Experiment

So, the other day I decided to switch my still embryonic blog from Graffiti CMS to BlogEngine.NET. There is nothing wrong with Graffiti, but a couple of things have driven me away.

Firstly, Graffiti, while very slick, is not open source. I can't tinker with it, except through their Chalk interface. Extending Chalk seems awkward.

Secondly, our dev shop at work has decided to use the open source BlogEngine.NET to run an internal website for our group. If we need to modify anything, we've got the source.

Thirdly, I've been wanting to find a platform on which I can build some custom specialty sites, with some very specific 'modules'. It's gotta be free and open source, and it's gotta be written in .NET. I've looked around at a bunch of blog and cms software, and though none of them looked like they were perfect for me, BlogEngine.NET seems as good as any other.

So, as the planets align, I'm now spending some time experimenting with it in earnest. The first step was installing it and replacing Graffiti on devadept.com. Next is to customize it. As I've browsed through the source, I've noticed a lot of things I would have done differently, or that don't match my particular coding style. So, I'm going to change it. I'll try to mold it into something that I'm more comfortable working with. I hope that, along the way, I'll find some things that inspire me to write some good posts.