The Boy Scout Principle

Earlier this week I helped out a junior colleague to solve a problem. I really just told him the quick fix. The fix involved overloading a method and accessing a property through reflection. Why reflection you ask? Well the code was written that way. All we had to do was follow suit and use reflection to access the property that we needed. The use of reflection made the code extremelly ugly and difficult to read. I didn't pay much attention to it so I provided the answer on how to access the property and went on my merry way.

When I got home I was going through my tweets and found the following link in a re-tweet by Uncle Bob (@unclebobmartin):

http://www.se-radio.net/podcast/2009-11/episode-150-software-craftsmanship-bob-martin

I listened to this podcast in it's entirety. In it Bob Martin talks about the different disciplines related to software craftsmanship; TDD, CI, etc. In particular one thing caught my attention. That was the concept which he refers to as "the boyscout principle", which basically states that a boyscout should leave the campsite better than he found it.

That got me thinking about the quick fix I provided to my colleague earlier. I should have took the time to show him how to leave the code better than he found it. After all my job is to mentor. What craftsman or mentor am I if I don't promote craftsmanship.

It also force me to ask the question:

"Why had the code been written in such an ugly manner?".

The only reason I could come up with was that the programmer wanted a way to make a call to method below it in the hierarchy but that method needed a concrete class from the calling assembly. I assume that the programmer figured somewhere in his/her career that you could do this through reflection. The problem that needed to be solved here was to work around a circular reference between two assemblies. That's precisely one of the problems that interfaces are designed to solve; to allow extensibility through an abstractions. This way we can code against the interface instead of the concrete class. Then our details depend on abstractions (Bob Martin describes this concept in his article about the Dependency Inversion Principle (DIP)) .

To solve the problem I decided to implement an interface instead of having all the reflection calls. While the refactor effort is not perfect, it does eliminate much of the reflection code. This enables us to follow the "Boy Scout Principle" of leaving the code better than we found it.

Here is a sample of the code before and after. I am not saying that the code is perfect, it would benefit from additional refactoring, but it is certainly better than before.

In case you are wondering what I did exactly in the code. I extracted an interface out of the class being reflected and then I cast the instance to that interface. This enables me to use the properties directly instead of using reflection to access the properties. The reason that the concrete class was not being used in the first place is because the concrete class is defined at a higher level than the method being called. Reflection was the ugly way to get around it. The correct way should have been through an abstranction to begin with.

As a disclaimer, I would have like to pass the interface instance directly into the method but that would also have meant breaking all the other consumers of this method.

//code before

public static void DirectPrintOldCodeDoNotUse(string assemblyFile, string reportFile, bool directPrint, DataSet refDataSet, string reportName, string reportForm, string tableName, int numCopies)
{

  try
  {

    string binFolder = MAT.Common.Utils.Utility.GetBinFolder();

    Directory.SetCurrentDirectory(binFolder);

    Assembly a = Assembly.LoadFrom(assemblyFile);

    Type rpt = a.GetType(reportFile);

    if(rpt == null)
    {
       throw new Exception("Could not create Type for " + reportFile);
    }
   
    MethodInfo run = rpt.GetMethod("Run");
    MethodInfo print = rpt.GetMethod("Print");
    PropertyInfo setDS = rpt.GetProperty("RefDataSet");
    PropertyInfo setReportName = rpt.GetProperty("ReportName");
    PropertyInfo setReportForm = rpt.GetProperty("ReportForm");
    PropertyInfo setTableName = rpt.GetProperty("TableNameToGetCountFrom");
    PropertyInfo setNumberOfCopies = rpt.GetProperty("NumberOfCopies");
    PropertyInfo dirPrnt = rpt.GetProperty("DirectPrint");
    object obj = Activator.CreateInstance(rpt);
    object[] objRn = new object[0];
    object[] objPrnt = new object[0];
    dirPrnt.SetValue(obj, directPrint, null);
   
    if(reportName != string.Empty && reportName != null)
    {
       setReportName.SetValue(obj, reportName, null);
    }

    setReportForm.SetValue(obj, reportForm, null);

    setTableName.SetValue(obj, tableName, null);

    setNumberOfCopies.SetValue(obj, numCopies, null);

    setDS.SetValue(obj,refDataSet, null);

    run.Invoke(obj, objRn);

    print.Invoke(obj, objPrnt);

  }
  catch(Exception exc)
  {
    Utilities.HandleException(exc);
  }
}


//code after
public static void DirectPrint(string assemblyFile, string reportFile, bool directPrint, DataSet refDataSet, string reportName, string reportForm, string tableName, int numCopies, Hashtable reportParameters)
{
  try
  {
    string binFolder = MAT.Common.Utils.Utility.GetBinFolder();
    Directory.SetCurrentDirectory(binFolder);
    Assembly assembly = Assembly.LoadFrom(assemblyFile);
    Type rpt = assembly.GetType(reportFile);

    if (rpt == null)
    {
      throw new Exception("Could not create Type for " + reportFile);
    }

    var directReportRunner = (IDirectReport)Activator.CreateInstance(rpt);
    if (!string.IsNullOrEmpty(reportName))
    {
      directReportRunner.ReportName = reportName;
    }

    directReportRunner.ReportForm = reportForm;
    directReportRunner.TableNameToGetCountFrom = tableName;
    directReportRunner.NumberOfCopies = numCopies;
    directReportRunner.RefDataSet = refDataSet;
    directReportRunner.Run();

    directReportRunner.Print();
  }
  catch (Exception exc)
  {
    Utilities.HandleException(exc);
  }
}