Thursday, March 4, 2010

Rethrowing exceptions to preserve stack details

In my last post, I described our plan at work to introduce FxCop into our development process. The first rule we will be enforcing is RethrowToPreserveStackDetails.

The following example violates the rule:

   12   public void MyTest()
   13   {
   14       Process();
   15   }
   16 
   17   public void Process()
   18   {
   19       try
   20       {
   21           int result = Calculator.Divide(12, 0);
   22       }
   23       catch (DivideByZeroException ex)
   24       {
   25           // React, likely by logging
   26           throw ex; // <- This is wrong
   27       }
   28   }

Executing this code results in the following logged callstack
  ...FxCopTests.Process() in C:\test\FxCopTests.cs:line 26
  ...FxCopTests.MyTest() in C:\test\FxCopTests.cs:line 14
Note that the callstack ends with line 26, which is in the catch block. In this example the real exception location isn't hard to find. Unfortunately, production code is rarely this simple.

The problem here is that calling "throw ex" causes the callstack info to be created at that point. If you were creating a new exception and throwing it this would be desired behavior. With an existing exception you don't want the original callstack to be overwritten.

The fix for this code is easy enough - replace:

  throw ex;

with

  throw;

After this change, the callstack correctly points to the line throwing the exception
  ...Calculator.Divide(Int32 x, Int32 y) in C:\test\Calculator.cs:line 11
  ...FxCopTests.Process() in C:\test\FxCopTests.cs:line 26
  ...FxCopTests.MyTest() in C:\test\FxCopTests.cs:line 14

Tuesday, February 16, 2010

FxCop: A starting point

In an effort to improve overall code quality at work, we are initiating manual and automated code reviews. One of the tools we will be using for automated reviews is FxCop. There are just two minor issues with this:
  1. Running every rule on an existing code base usually results in a massive backlog
  2. Most developers, myself included, are unfamiliar with at least some of the rules
Granted, several of the rules categories can be turned off on most projects (Portability and Interoperability come to mind.) There are also a few rules that are unimportant or largely obsolete. This still leaves a sizeable list of rules to deal with.

Our planned approach is to enable several of the rules as warnings on the build server. After the developers have addressed any issues, the build will be modified to fail on future violations of those rules. Slowly adding rules, first as warnings and then as errors, will allow us to clean up our existing code base and prevent new violations from being introduced. As we go I will be putting together examples of violations and fixes. I'll post the examples for those who want to follow along at home.

Friday, November 13, 2009

More than one way to sort a List<>

One operation you occasionally need to perform is sorting a generic list of objects. Often developers code handle with an inline delegate. If I wanted to sort a collection of boardgames by rating, my code might look like this:

  games.Sort(
      delegate(BoardGame a, BoardGame b)
      {
          if (a.Rating < b.Rating)
              return 1;
          if (a.Rating > b.Rating)
              return -1;
          return 0;
      });

Although this works, it tends to look a bit cluttered. It also increases a method's complexity and doesn't adhere to the idea of separation of concerns. If we only need to sort Boardgames in this one location, a better solution is to move the comparison code into a separate method in the same class. The new method looks like this:

  public int CompareBoardgamesByRank(BoardGame a, BoardGame b)
  {
      if (a.Rating < b.Rating)
          return 1;
      if (a.Rating > b.Rating)
          return -1;
      return 0;
  }

Our call to Sort looks like this:

  games.Sort(CompareBoardgamesByRank);

This makes the code much cleaner for a single sort. If we want to sort Boardgames by rating from multiple locations, we need a better place to store the comparison method. If we implement IComparable on our Boardgame class, we can create a CompareTo method on the class like so:

  public int CompareTo(object obj)
  {
      BoardGame b = (BoardGame)obj;
 
      if (Rating < b.Rating)
          return 1;
      if (Rating > b.Rating)
          return -1;
      return 0;
  }

Since the class now has a default comparison, we no longer need to specify a delegate when calling Sort - the CompareTo method will automatically be executed with an empty call:

  games.Sort();

What if we need additional comparison methods for our Boardgame? An easy way to handle this is to create static methods on the class, which will be available anywhere that Boardgame can be accessed. Here are a couple methods I've added to my Boardgame class:

  public static int CompareByName(BoardGame a, BoardGame b)
  {
      return string.Compare(a.Name, b.Name);
  }
 
  public static int CompareByRankAssending(BoardGame a, BoardGame b)
  {
      if (a.Rating > b.Rating)
          return 1;
      if (a.Rating < b.Rating)
          return -1;
      return 0;
  }

Which I can now pass into the Sort method:

  games.Sort(BoardGame.CompareByRankAssending);

Tuesday, October 20, 2009

Refactoring assemblies without breaking existing apps

Over time, most development shops identify common code that needs to be used across multiple projects. This code is eventually collected into a single assembly, usually something like Common.dll or Utilities.dll. In the beginning this is a decent way to eliminate code duplication. Over time, however, this single assembly becomes difficult to maintain.

At this point the obvious fix is to split the assembly into multiple smaller ones. Unfortunately, by then the single assembly is used on numerous projects. A major refactoring now requires extensive changes across all of these referencing applications. This realization usually stops any refactoring effort, leaving the utilities assembly to continue growing larger and more unwieldy.

To look at potential solutions, I've created a Utilities assembly with the following Logger class:

  namespace Utilities
  {
      public class Logger
      {
          public void LogError(string message)
          {
              Debug.WriteLine("Error: " + message);
          }
      }
  }

The goal is to move this class to a separate assembly called LoggingUtilities.

One solution is to use the TypeForwardedTo attribute. This is an assembly-level attribute that flags a specified class as having moved. To use this, I start by moving the Logger class to my new assembly. Note that I keep the same namespace as before - this is required for the forwarding to work.

Next I add a reference to LoggingUtilities within Utilities.



Finally, I open up AssemblyInfo.cs file in the Utilities project and add the following line:

  [assembly: TypeForwardedTo(typeof(Utilities.Logger))]

If I recompile the dlls and drop them in a folder with my existing application, it will continue to function even though the class has been moved.

This takes care of keeping the current compiled code running, but what about future versions? If I open up the source for one of my applications and attempt to compile, I now receive errors stating "The type or namespace name 'Logger' could not be found." It seems the redirection works at runtime but not at compile time. For someone not familiar with the previous refactoring, this could prove an interesting issue to track down.

In my opinion, there is a far better solution than using the TypeForwardedTo attribute. Going back to the original code, this time I copy the code to the new assembly (as opposed to moving it.) On the copy I change the namespace to match my new assembly.

  namespace LoggingUtilities
  {
      public class Logger
      {
          public void LogError(string message)
          { 
              Debug.WriteLine("Error: " + message);
          }
      }
  }

In my original Logger class, I create an instance of my new Logger. Each method in the original class now forwards requests to the new Logger instance. In this way, I am wrapping the new class in the original. This allows applications to still use the old class, though the functionality has been moved.

  public class Logger
  {
      LoggingUtilities.Logger _logger =
          new LoggingUtilities.Logger();
 
      [Obsolete("Use LoggingUtilities.Logger instead")]
      public void LogError(string message)
      {
          _logger.LogError(message);
      }
  }

As before we need to evaluate referencing projects. Because our original class still exists, these applications will continue to compile.

Note that I've added an "Obsolete" attribute to the LogError method. This means we will receive a compiler warning (or error) that we need to change our application to use the new class. This makes it clear what needs to be modified, saving time on any rework.

Sunday, October 18, 2009

Code can be both clean and efficient

Chapter 26 of Code Complete focuses on code tuning - the art of modifying code to improve performance. One example given is a switched loop:

  for (i = 0; i < count; i++)
  {
      if (sumType == SUMTYPE_NET)
      {
          netSum = netSum + amount[i];
      }
      else
      {
          grossSum = grossSum + amount[i];
      }
  }

Notice the 'if' statement inside the loop. If the array is rather large, this statement will be evaluate numerous times, despite the fact that the result will never change. The recommended solution is to unswitch the loop, so the 'if' statement is only evaluated once:

  if (sumType == SUMTYPE_NET)
  {
      for (i = 0; i < count; i++)
      {
          netSum = netSum + amount[i];
      }
  }
  else
  {
      for (i = 0; i < count; i++)
      {
          grossSum = grossSum + amount[i];
      }
  }

This recommendation was given with one warning: this code is harder to maintain. If the logic for the loops needs to change, you have to make sure to change both loops to match.

As with most coding tasks, there is more than one possible solution. In this case the ideal approach is to have both a single comparison and a single loop. If we throw one additional variable into the code, we can calculate the summation and then add it accordingly:

  for (i = 0; i < count; i++)
  {
      arraySum = arraySum + amount[i];
  }
 
  if (sumType == SUMTYPE_NET)
  {
      netSum = netSum + arraySum;
  }
  else
  {
      grossSum = grossSum + arraySum;
  }

Saturday, October 17, 2009

Export filtered Access data to Excel

In my free time I've been creating an MSAccess database containing a few data-entry forms. One of these forms allows the user to filter records based on several different criteria. This part was relatively straightforward. The difficulty was in trying to export the filtered information to an Excel spreadsheet. Although this functionality exists in Access, the installed help file was less than helpful. Forum posts seemed to contain partial solutions or solve something almost, but not quite what I was trying to do.

The following VBA subroutine is the eventual solution:

Private Sub Export_Click()
    Dim whereClause As String
    
    ' Generate our WHERE clause based on form values
    whereClause = GenerateFilterClause
    
    ' If we have no filter, export nothing
    If IsEmptyString(Nz(whereClause)) Then
        Exit Sub
    End If
    
    Dim query As String
    query = "SELECT DISTINCTROW Contacts.* " & _
            " FROM Contacts " & _
            " INNER JOIN Applications " & _
            " ON Contacts.ContactID = Applications.ContactID " & _
            " WHERE " & whereClause & ";"
    
    Dim filename As String
    filename = "c:\test.xls"

    ' Placeholder query already in the database
    Dim queryName As String
    queryName = "FilterExportQuery"

    ' Update the placeholder with the created query
    CurrentDb.QueryDefs(queryName).SQL = query

    ' Run the export
    DoCmd.TransferSpreadsheet acExport, acSpreadsheetTypeExcel9, queryName, filename
End Sub

Monday, October 12, 2009

Monitoring log files in real time

Debugging Windows services, especially in a test or production environment, can be tricky. In many cases you won't have access to the box. You certainly won't have the ability to step through the code.

Thus, you are usually forced to monitor log files, looking for an indication of where the problem occurred. The typical method is to start by opening the file in Notepad. Then, when you want to see more recent log entries, you close and reopen the file. This is mildly annoying when dealing with a single service. If your business workflow is split among multiple services, this becomes incredibly inefficient.

An easier solution is to use a log monitor app such as BareTail. For this little demo I am using the free version of the tool.

I have three separate services logging to files named "process1.log", "process2.log" and "process3.log." To keep it simple I am only logging the RequestID for each received request. Here I have loaded all three logs into BareTail.


When I send a new request to the system it should update each log file. BareTail monitors the logs and displays any updates. In the screenshot below, note the new Request ID #2918891. Note also that the document tabs for each file show a green arrow - this indicates an update was made.


As you view each tab, the green arrow will be cleared to visually show which files you have already reviewed.


Say you were trying to debug a request that fails to make it through the system. A quick glance at the document tabs will show you how far a request made it through the system. After reviewing each log (to clear the green markers) we submit another request. In the following screenshot, note that we have a green arrow for process1.log and process2.log, but none for process3.log. So either the second process failed to send the message on, or the third process failed to receive it.