Joel Fights Back

Joel responds to Ned's criticism with his own words:

“Ned, for the sake of argument, could you do me a huge favor, let's use a real example. Change the name of DoSomething() to InstallSoftware(), rename DoThing1() to CopyFiles() and DoThing2() to MakeRegistryEntries().”

Ok, here we go:

void InstallSoftware(string[] files, string[] registryEntries)
{
   int ret = CopyFiles(files);
   if(ret != NO_ERROR)
   {
      RollBackFileCopy(files, ret);
      MessageBox.Show(“Installation failed for the following reason:“+GetErrorDescription(ret));
   }
   else
   {
       ret = MakeRegistryEntries(registryEntries);   
       if(ret != NO_ERROR)
       {
           RollBackFileCopy(files, files.Length - 1);
           RollBackRegistry(registryEntries, ret);
           MessageBox.Show(“Installation failed for the following reason:“+GetErrorDescription(ret));
       }
   }
}
void InstallSoftware(string[] files, string [] registryEntries)
{
   try
   {
      CopyFiles(files);
      MakeRegistryEntries(b);    
   }
   catch(EarlyAbortException ex)
   {
      if(ex is RegistryException)  RollBackRegistry(registryEntries, ex.LastIndex);
            
      int fileIndex = files.Length - 1;
      if(ex is FileCopyException)    fileIndex = ex.LastIndex;
      
      RollBackFileCopy(files, fileIndex);
      
      MessageBox.Show(“Installation failed for the following reason:“+ex.Message);
   }
}

For the sake of simplicity, I haven't included error handling on the RollBack methods, but it would work pretty much the the same way (however, they should probably handle errors internally to ensure that they rollback as much as possible without just aborting because of some error).

In any case, some observations:

• Installations are a bad example, because they contain a lot of manual rollback work. In the majority of cases you run into, about the only “rollback“ required is closing a connection or two or aborting a transaction. In those simple cases, which comprise the majority of an application's error handling, a few lines of code in a simple catch block or finally block can often solve the issue.
  
• The exception version requires that CopyFiles and MakeRegistryEntries only return exceptions of the type “EarlyAbortException.“ This actually is pretty easy to enforce, and doesn't limit you too much, because you can always wrap the cause of the abort in an inner exception, or create sub exception classes for each type of exception thrown by the method.
  
• The error code version must include a generic GetErrorDescription method. Exceptions are much nicer, because the error message can be tailored to the context (for example, “the file x could not be copied because...“). It is techically possible to do something like this with the error code version, but you are going to write a bit of plumbing to make it happen.
  
• If the return value on these methods had been some type of handle, a simple != NO_ERROR would not have been adequate. You can solve this by using “out“ parameters to return your handle, but that does clutter up your code a bit.
  
• The exception handling version allows me to put all my rollback code in the same place, instead of scattering it throughout.
  
• The exception handling version handles errors in the exact same way as all my other code. One of the major problems with status codes isn't visible until you look at a bunch of different method calls from different libraries or portions of your code. The way you handle status codes will probably be completely different depending on what method you are calling. As Ned pointed out, some times the error might be indicated by a 0, some times it is a -1, some times it is a value less than a value you passed in (like file read operations), some times it is NULL, etc.
  
• This example is contrived, because we would really want to have a class called “InstallStep“ or something that had a execute and a rollback method, because then we could handle rollback a lot easier. With either of the two versions above, you would have to be pretty smart about things to avoid your rollback balooning out of control. Here is a better example of how the process should work:

  int InstallSoftware(InstallSteps steps)
  {
   int ret = NO_ERROR;
   foreach(InstallStep s in steps)
   {
    if( (ret = s.Execute()) != NO_ERROR)
    {
     for(int i = 0; i < steps.IndexOf(s); i++)
     {
      steps[i].RollBack();      
     }
     break;
    }
   }
   return (ret == NO_ERROR) ? NO_ERROR : INSTALL_FAILED;
  }

OR

  void InstallSoftware(InstallSteps steps)
  {
   foreach(InstallStep s in steps)
   {
    try
    {
     s.Execute();
    }
    catch(Exception ex)
    {
     for(int i = 0; i < steps.IndexOf(s); i++)
     {
      steps[i].RollBack();
     }
     throw new InstallStepFailedException(s, ex);     
    }
   }
  }

Although the complexity of the two is about the same, I definately prefer the exception version because of the extra information I get (such as inner exception for the detailed abort information and stack traces, etc.). But, feel free to make your own observations.