Correctly disposing of objects - implement IDisposable

To continue with our FxCop backlog, we are going to look at a couple rules dealing with IDisposable. Consider the following class:

  public class TimedProcessor

  {

      Timer startTimer = new Timer();

      public TimedProcessor(double interval)

      {

          startTimer = new Timer(interval);

          startTimer.Elapsed += timer_Elapsed;

      }

      void timer_Elapsed(object sender, ElapsedEventArgs e)

      {

          Console.WriteLine("Do work here");

      }

  }

Running FxCop on this will report a violation of the rule Types that own disposable fields should be disposable. The problem is that the startTimer field is of type Timer, which implements IDisposable. To properly use the timer, we need to call its Dispose method as soon as we are done with it. The solution is to implement IDisposable on our class and make sure we call startTimer.Dispose().

The class after fixing the violation:

  public class TimedProcessor : IDisposable

  {

      Timer startTimer = new Timer();

      public TimedProcessor(double interval)

      {

          startTimer = new Timer(interval);

          startTimer.Elapsed += timer_Elapsed;

      }

      void timer_Elapsed(object sender, ElapsedEventArgs e)

      {

          Console.WriteLine("Do work here");

      }

      public void Dispose()

      {

          if (startTimer != null)

              startTimer.Dispose();

      }

  }

Note that I check for null before calling Dispose. If an error elsewhere left the object in an invalid state, we don't want our Dispose method to throw a NullReferenceException.

Now, say we add a second field called endTimer. Running FxCop now will report a violation of the rule Disposable fields should be disposed. In this case we have already implemented IDisposable, but not all of our disposable fields have been addressed. To fix this, we need to modify our Dispose method slightly:

  public void Dispose()

  {

      if (startTimer != null)

          startTimer.Dispose();

      if (endTimer != null)

          endTimer.Dispose();

  }

A custom FxCop rule - calling Debug methods

While working on a recent production issue, we ran into an interesting problem. We suspected an exception was being thrown when calling an external site, but we couldn't prove it. Our usual method of exception handling is to save the details to a rolling log file. This particular service, however, was failing without leaving behind any info as to why. Digging through the code revealed a catch block similar to this:

  catch (Exception ex)

  {

      Debug.WriteLine(ex.ToString());

  }

For now, ignore the fact that you will only see output using something like DebugView. The real problem is that when the code is compiled in Release mode, calls to Debug.WriteLine are removed completely. So in the example above, the catch block will be empty.

To reduce the odds of this sort of thing happening in the future, I decided to write a custom FxCop rule to locate any calls to methods that had been tagged with a "Debug" conditional. It wasn't as easy as I had expected, but there is an excellent tutorial on the subject. The only difficulty I had was in locating the ConditionalSymbol property in the class tree.

For anyone interested, here is the xml rule file:

    <?xml version="1.0" encoding="utf-8" ?>

    <Rules FriendlyName="Custom Rules">

        <Rule TypeName="DoNotCallDebugConditionalMethods" Category="CustomRules" CheckId="PG1001">

            <Name>Do not call debug conditional methods</Name>

            <Description>Calls to methods marked with the DEBUG conditional will be removed

            from Release builds.</Description>

            <Url></Url>

            <Resolution>Replace the call to '{0}' with a more appropriate call</Resolution>

            <MessageLevel Certainty="100">Warning</MessageLevel>

            <Email></Email>

            <FixCategories>DependsOnFix</FixCategories>

            <Owner></Owner>

        </Rule>

    </Rules>

And the code:

  namespace CustomFxCopRules

  {

      /// <summary>

      /// Warns of any methods being called that are removed from non-debug builds,

      /// such as Debug.WriteLine()

      /// </summary>

      public class DoNotCallDebugConditionalMethods : BaseIntrospectionRule

      {

          public DoNotCallDebugConditionalMethods()

              : base("DoNotCallDebugConditionalMethods", "CustomFxCopRules.rules.xml",

                  typeof(DoNotCallDebugConditionalMethods).Assembly)

          { }

          public override ProblemCollection Check(Member member)

          {

              Method method = member as Method;

              if (method != null)

              {

                  VisitStatements(method.Body.Statements);

              }

              return Problems;

          }

          public override void VisitMethodCall(MethodCall call)

          {

              var member = ((MemberBinding)call.Callee).BoundMember;

              var method = (Method)member;

              var symbol = method.ConditionalSymbol;

              if (!string.IsNullOrEmpty(symbol) && symbol.Contains("DEBUG"))

              {

                  Problems.Add(new Problem(GetResolution(method.FullName), call.SourceContext));

              }

          }

      }

  }

In space, no one can hear you scream

Lately, most of my time is being spent refactoring legacy code. Everywhere I look I find try/catch blocks wrapped around a few lines of code. It appears the developer was using this as a way to "fix" bugs - by catching and eating the exception instead of tracking down the root cause. Enabling exception breakpoints and attempting to run the code is enough to make a developer scream. Unfortunately, if the exception isn't rethrown...

   catch (Exception)

   {

       // No one can hear you scream!

   }

I may have to put that on a t-shirt...

Proper exception usage

Continuing our FxCop code cleanup, I decided to focus the next set of rules on working with exceptions.

Do not raise reserved exception types

These exceptions (such as SystemException and OutOfMemoryException) were designed as base classes or for CLR use only. Instead of using one of these, either find a more specific one in the .Net Framework or create your own.

  public static void ThrowsBaseException()

  {

      // This is too vague to be useful

      throw new Exception("Bummer");

  }

Instantiate argument exceptions correctly

The following is an example violation. Note that the thrown exception doesn't inform the caller which argument was at fault or how it was supposed to be called.

  public static int Divide(int dividend, int divisor)

  {

      if (divisor == 0)

          throw new ArgumentException();

      return dividend / divisor;

  }

To fix this, use a constructor that takes the name of the parameter and/or a string message stating the problem.

  throw new ArgumentException("Divisor cannot be 0", "divisor");

Do not raise exceptions in unexpected locations

Certain methods are generally assumed to never throw an exception when called (equality operators) or only throw certain exceptions (such as property getters.) To be consistent, your code should follow a similar pattern.

For example, the debugger uses the ToString method to display information. The following will cause issues while debugging:

  public override string ToString()

  {

      throw new Exception("Don't do this!");

  }

Do not raise exceptions in exception clauses

The following code attempts to call the Divide method with a divisor of zero:

  public void ThrowsExceptionFromFinally()

  {

      try

      {

          Divide(12, 0);

      }

      finally

      {

          throw new Exception("Ouch");

      }

  }

Based on the method defined previously, the code should throw an ArgumentException. In this example, however, the ArgumentException is lost due to a new exception being thrown from the finally block. If you want to thrown a new exception, do so from a catch block and include the caught exception as the inner exception.

  catch (ArgumentException ex)

  {

      throw new Exception("Ouch", ex);

  }

Exceptions should be public

The previous rules dealt with exceptions built into the .Net Framework. The last two deal with custom exceptions. Take the following custom exception class:

  internal class MyCustomException : Exception

  {    

      public MyCustomException()

      {            

      }

      public MyCustomException(string message)

          : base(message)

      {            

      }

  }

Note that the class is marked internal. The problem with this is that outside the assembly, the only way to handle this exception is to catch the base Exception (which is a bad thing.) The simple fix is to make the class public.

Implement standard exception constructors

Using the above exception class, note there are currently two constructors. The base Exception, however, defines two others - one to allow inner exceptions and one for serialization. Correcting the violation means making sure all four standard constructors have been defined. To fix the above class, the following methods need to be added:

  public MyCustomException(string message, Exception innerException)

      : base(message, innerException)

  {            

  }

  protected MyCustomException(SerializationInfo info, StreamingContext context)

      : base(info, context)

  {            

  }

Easy pickings: Class-level FxCop warnings

The last batch of FxCop warnings proved to require a lot more effort than I had anticipated. It's amazing how much dead code accumulates over the years. For the next set of rules I decided to pick ones that were easy to fix but, more importantly, had few actual violations in our projects.


Abstract types should not have constructors

The following class violates the rule:

  public abstract class MyAbstractClass

  {

      public MyAbstractClass()

      {

          // Do setup code for derived classes

      }

  }

A class defined as abstract can't be instantiated directly. Thus, the only purpose for a constructor is to allow for default setup when a derived class is created. To fix the issue, change the constructor accessibility to protected.


Do not declare protected members in sealed types
Do not declare virtual members in sealed types

A sealed class is one that cannot be used as a base class. Protected and virtual members are useful for derived classes, which is a contradiction. Fix the protected member by making it private instead. As for the virtual members, this is a C++ issue only, as C# and VB.Net will fail to compile. To fix this for C++, unseal the class or remove the virtual modifier.

  public sealed class MySealedClass

  {

      protected void Process()

      {

          // Might as well be private

      }

      public virtual void WillNotCompile()

      {

          // A C++ feature only, so that was easy :)

      }

  }

Static holder types should be sealed
Static holder types should not have constructors

Take the following:

  public class MyStaticClass

  {

      public static void DoWork()

      {

          // Do stuff here...

      }

  }

This class only contains static members, so there is no reason to create instances of the class. When this is compiled, however, the class will be given a default public constructor. Prior to .Net 2.0, the fix was to implement an empty constructor and set the access level to private. Beginning with .Net 2.0, an easier fix is to set the class itself to static.

Unit test all enum values with NUnit

In an older post I demonstrated NUnit's built-in parameterized tests feature. This allows a developer to call a single method to run multiple tests.

Let's say I want to run the test for each value in an enumeration. Using the TestCase attribute, I can write the test like this:

  [TestCase(Powertools.Chainsaw)]

  [TestCase(Powertools.CircularSaw)]

  [TestCase(Powertools.PowerDrill)]

  public void PowerToolsTestExplicit(Powertools p)

  {

      // Do test

  }

Which is fine, but what if I add a new value to the enum? Instead of having to add another attribute to the test, it would be easier to loop over all enum values at runtime. With the TestCaseSource attribute I can do just that.

Within my unit test class I first create a method that returns an IEnumerable (in this case Array) containing the enum values:

  public Array GetPowerTools()

  {

      return Enum.GetValues(typeof(Powertools));

  }

Then I create my unit test and decorate it with the TestCaseSource attribute. The attribute constructor takes one parameter, sourceName, which is the name of the method to call:

  [TestCaseSource("GetPowerTools")]

  public void PowerToolsTestWithIEnumerable(Powertools p)

  {

      // Do test

  }

In either case, this expands my unit tests as expected. The second method is easier to maintain and less likely to allow untested code into the system.

Remove unused code

When considering the next set of FxCop rules to enable on the build server, my first thought was to look at those dealing with properly disposing of objects. Scanning the active projects at work has revealed a rule-set slightly more important - unused code. In some of our older code bases there are a variety of methods, variables, etc. that are no longer being used. Deleting the dead code will make it easier to understand what the code is actually doing. In many cases, this will also fix a variety of other FxCop issues, as the flagged items are no longer present. Why fix what you will eventually delete?

And before I go over the rules I should emphasize deletion of dead code. I've seen numerous instances where a developer has commented out, #ifdef'd, or otherwise excluded a section of code. In some cases this was to finish or re-implement the code later. In other cases it was to remove code not currently needed. Regardless, this should be avoided. Tracking previous revisions of code is what source control is for.

The first rule to address is Avoid uninstantiated internal classes. This is a class not visible outside the assembly and is never actually used within that assembly. If you're lucky, deleting this class will improve a variety of code metrics (the number of FxCop warnings being just one of them.)

For the next examples I reference the following class:

    9   public class Unused

   10   {

   11       int unusedPrivateField;

   12 

   13       private void UnusedPrivateCode()

   14       {

   15           Debug.WriteLine("This method isn't called");

   16       }

   17 

   18       internal void DoWork()

   19       {

   20           int unusedLocal;

   21           Debug.WriteLine("This is the only method possibly called");

   22       }

   23   }

Working through the code, line 11 returns a warning for the rule Avoid unused private fields. Notice that none of the methods use it and it isn't exposed through a property.

Next, the method UnusedPrivateCode violates, you guessed it, Avoid uncalled private code. This method is uncalled locally and unreachable outside the class.

The final violation is of type Remove unused locals, at line 20. The variable is not used within the method and can be removed. Note that this rule will also fire if the variable is assigned to, but its value is never actually used. An example:

   18   public void DoWork()

   19   {

   20       int unusedLocal = 0;

   21       unusedLocal = 12; // Still not using...

   22   }

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

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.

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);

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.

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;

  }

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

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.

Cleaning up enumerations

When working on existing code I occasionally run into an enumeration class similar to this:

  public class PowertoolsConstants

  {

      public enum Powertools

      {

          PowerDrill = 0, // Standard, corded drill

          Chainsaw,       // Everyone's favorite

          CircularSaw     // If the chainsaw is out of gas, use this

      }

      public static Powertools ConvertFromString(string s)

      {

          switch (s)

          {

              case "PowerDrill":

                  return Powertools.PowerDrill;

              case "0":

                  return Powertools.PowerDrill;

              case "Chainsaw":

                  return Powertools.Chainsaw;

              case "1":

                  return Powertools.Chainsaw;

              case "CircularSaw":

                  return Powertools.CircularSaw;

              case "2":

                  return Powertools.CircularSaw;

              default:

                  throw new Exception("Unknown Powertool");

          }

      }

      public static string ConvertFromPowertool(Powertools p)

      {

          switch (p)

          {

              case Powertools.Chainsaw:

                  return "Chainsaw";

              case Powertools.CircularSaw:

                  return "CircularSaw";

              case Powertools.PowerDrill:

                  return "PowerDrill";

              default:

                  return "Unknown";

          }

      }

  }

Nothing too complex, but it can be cleaned up a bit. For starters, accessing the enum currently requires referencing the class:

  PowertoolsConstants.Powertools tool =

      PowertoolsConstants.Powertools.Chainsaw;

If we move the enum declaration above the class we can remove the class reference:

  Powertools tool = Powertools.Chainsaw;

Next is addressing the two methods in the class: ConvertFromString and ConvertFromPowertool. The purpose of these methods is to switch between our enumeration and a string representation of the enum, perhaps to store values in an xml file or database. As the .Net Framework already contains this functionality, the methods are not necessary and can be deleted.

To convert from an enum value to a string we can use Enum.GetName

  string toolName = Enum.GetName(typeof(Powertools), tool);

To convert from a string to an enum value we can use Enum.Parse. Note that this will throw an ArgumentException if an invalid string is passed in.

  tool = (Powertools)Enum.Parse(typeof(Powertools), toolName);

With the two methods removed, the class PowertoolsConstants is empty and can be deleted.

One final thing to look at are the comments beside the enum values. These appear to be usage notes. If a developer using the enum needs to know this information, he shouldn't have to open this code to get it. The way to correct this is to replace the existing comments with xml-style comments.

  public enum Powertools

  {

      /// <summary>

      /// Standard, corded drill

      /// </summary>

      PowerDrill = 0,

      /// <summary>

      /// Everyone's favorite

      /// </summary>

      Chainsaw,

      /// <summary>

      /// If the chainsaw is out of gas, use this

      /// </summary>

      CircularSaw

  }

Doing this will provide the developer with Intellisense hints as they code: