Twitter GitHub Facebook Instagram dirv.me

Daniel Irvine on building software

Use short, well-named methods in place of comments

24 August 2013

Traditional computer science teachings tell us to use comments liberally. The big problem with comments is that they become out-of-date quickly: by the time you read them, the surrounding has changed so much that the comment text no longer applies. A second problem is that writing good comments is hard. As with all writing, you have to think carefully about your audience and about what they’re likely to know already, and that is a difficult task. It’s all too easy to confuse your readers even more than they were already.

The modern, minimalist coder teaches you this: don’t write comments. It’s very simple to do. Here’s how:

  • Keep all methods very short--use 7 statements as a guideline for maximum size.
  • If you’re faced with a long method, break it up by following the single responsibility principle: each method should just have one responsibility. Control structures like if statements and loops are natural points at which to break a method up (see the example below).
  • Once you have your method, give it a descriptive name--don’t worry if it ends up being long.
You’ll find that you suddenly need far fewer comments than before. Your method names start doing the documenting for you.

As an added bonus, stack traces will have more useful information in them. Sometimes you’ll know what went wrong just from reading the stack trace alone.

Example Here’s a long method (in C#):

public void Run() {
  _notifier.Log(TraceLevel.Info, Beginning HttpDataSender operation to
    http://<host>/{0}/{1}., _serviceDefinition.ServiceEndpoint, _relativeUri);
  Uri relativeUri;
  try{
    relativeUri = new Uri(String.Format({0}/{1}, _serviceDefinition.ServiceEndpoint, _relativeUri),
      UriKind.Relative);
  }catch(UriFormatException){
    _notifier.Log(TraceLevel.Error, Either the endpoint or the relative URI is not valid. Please
      check your service definition and try again.);
    return;
  }

  List<Task> runningTasks = new List<Task>();
  foreach (var server in _serviceDefinition.Servers) {
    Uri fullUri;
    try {
      var serverString = new Uri(http:// + server);
      fullUri = new Uri(serverString, relativeUri);
    } catch (UriFormatException) {
      _notifier.Log(TraceLevel.Error, The host {0} is not a valid hostname/port combination.  It
        will be skipped.);
      continue;
    }
    var thisServer = server; // copy string so it can be used in the lambda expression below
    runningTasks.Add(Task.Factory.StartNew((o)=>SendDataTo(fullUri, thisServer), null));
  }

  if (!Task.WaitAll(runningTasks.ToArray(), TimeSpan.FromSeconds(20))) {
    _notifier.Log(TraceLevel.Error, Not all tasks completed in time.);
  }
}

We can break out these methods:

private bool TryBuildRelativeUri(out Uri relativeUri)
{
  _notifier.Log(TraceLevel.Info, Beginning HttpDataSender operation to
    http://<host>/{0}/{1}., _serviceDefinition.ServiceEndpoint, _relativeUri);

  try {
   relativeUri = new Uri(String.Format({0}/{1}, _serviceDefinition.ServiceEndpoint, _relativeUri),
      UriKind.Relative);
  }catch(UriFormatException){
    relativeUri = null;
    _notifier.Log(TraceLevel.Error, Either the endpoint or the relative URI is not valid. Please
      check your service definition and try again.);
  }

  return relativeUri != null;
}

private void StartNewDataTaskIfServerIsValid(string server, Uri relativeUri, List<Task> runningTasks)
{
  try {
    var serverString = new Uri(http:// + server);
var fullUri = new Uri(serverString, relativeUri);

    runningTasks.Add(Task.Factory.StartNew((o)=>SendDateTo(fullUri, thisServer), null));
} catch (UriFormatException) {
    _notifier.Log(TraceLevel.Error, The host {0} is not a valid hostname/port combination.  It
      will be skipped.);
  }
}

private void WaitOnAllTasksAndReport(List<Task> runningTasks)
{
  if (!Task.WaitAll(runningTasks.ToArray(), TimeSpan.FromSeconds(20))) {
    _notifier.Log(TraceLevel.Error, Not all tasks completed in time.);
  }
}

At first glance the source looks longer and more complicated. However, have a look at the new Run method:

public void Run() {
  Uri relativeUri;

  if(TrybuildRelativeUri(out relativeUri)) {
    List<Task> runningTasks = new List<Task>();
foreach (var server in _serviceDefinition.Servers) {
      StartNewDataTaskIfServerIsValid(server, relativeUri, runningTasks);
    }

    WaitOnAllTasksAndReport(runningTasks);
  }
}

Although this is a fairly contrived example, it highlights the main point. The factorization has create more methods (with slightly more syntax), but the primary method is more readable as a result. You can grok the Run method in a matter of seconds, and then home in on the individual method you’re looking for without having to understand the other methods.

About the author

Daniel Irvine is a software craftsman at 8th Light, based in London. These days he prefers to code in Clojure and Ruby, despite having been a C++ and C# developer for the majority of his career.

For a longer bio please see danielirvine.com. To contact Daniel, send a tweet to @d_ir or use the comments section below.

Twitter GitHub Facebook Instagram dirv.me