Refactoring Dumb, Dumber, Dumbest away

In my previous crazy post I had shown some code that was butt ugly. It was a series of if statements to determine some value (an enum) then use that value to switch on a case statement that would assign some values and audit the steps as it went along. It was ugly and the challenge before me was to refactor it away. I chose the Strategy Pattern as it seemed to make sense in this case, even if it did introduce a few more classes. So here we go.

First, lets look at the full code we're refactoring. Here's the silly enumerated value:

   15 enum DumbLevel

   16 {

   17     Dumb,

   18     Dumber,

   19     Dumbest,

   20     Not

   21 }

And here's how it's used:

  695 for (int i = 0; i < _segments.Count; i++ )

  696 {

  697     ISegment segment = _segments[i];

  698 

  699     #region determine cable count & heat segment voltage

  700 

  701     int cableCount;

  702     decimal heatSegmentVoltage;

  703     DumbLevel dumbLevel;

  704 

  705     //determine the specific segment configuration

  706     if (_segments.Count > 1)

  707     {

  708         cableCount = segment.GetCableCount(); //use # of tracers to determine cable count

  709 

  710         if (cableCount == 1)

  711         {

  712             if (PassesCount > 1)

  713             {

  714                 if (i <= (_segments.Count - 2)) //for all but last

  715                     dumbLevel = DumbLevel.Dumbest;

  716                 else

  717                     dumbLevel = DumbLevel.Dumb; //last segment

  718             }

  719             else

  720                 dumbLevel = DumbLevel.Dumb;

  721         }

  722         else

  723             dumbLevel = DumbLevel.Dumber;

  724     }

  725     else

  726         dumbLevel = DumbLevel.Not;

  727 

  728 

  729     //calculate cable count and heat segment voltage based on the segment configuration

  730     switch (dumbLevel)

  731     {

  732         case DumbLevel.Dumb:

  733             cableCount = segment.GetCableCount(); //use # of tracers to determine cable count

  734             AuditStep("Cable Count: {0} (based on count of Tracers identified)", cableCount);

  735             AuditStep("");

  736 

  737             heatSegmentVoltage = SupplyVoltage * Project.VoltageDrop * segmentPercentage;

  738             AuditStep("Supply Voltage ({0}) * Voltage Drop ({1}) * Segment Percentage ({2})", SupplyVoltage, Project.VoltageDrop, segmentPercentage);

  739             break;

  740 

  741         case DumbLevel.Dumber:

  742             cableCount = segment.GetCableCount(); //use # of tracers to determine cable count

  743             AuditStep("Cable Count: {0} (based on count of Tracers identified)", cableCount);

  744             AuditStep("");

  745 

  746             heatSegmentVoltage = SupplyVoltage * Project.VoltageDrop * segmentPercentage / PassesCount;

  747             AuditStep("Supply Voltage ({0}) * Voltage Drop ({1}) * Segment Percentage ({2}) / Passes # ({3})", SupplyVoltage, Project.VoltageDrop, segmentPercentage, PassesCount);

  748             break;

  749 

  750         case DumbLevel.Dumbest:

  751             cableCount = _passesCount;

  752             AuditStep("Cable Count: {0} (based on Passes #)", _passesCount);

  753             AuditStep("");

  754 

  755             heatSegmentVoltage = SupplyVoltage * Project.VoltageDrop * segmentPercentage / PassesCount;

  756             AuditStep("Supply Voltage ({0}) * Voltage Drop ({1}) * Segment Percentage ({2}) / Passes # ({3})", SupplyVoltage, Project.VoltageDrop, segmentPercentage, PassesCount);

  757             break;

  758 

  759         case DumbLevel.Not:

  760             cableCount = 1;

  761             AuditStep("Cable Count: 1");

  762             AuditStep("");

  763 

  764             heatSegmentVoltage = SupplyVoltage * Project.VoltageDrop;

  765             AuditStep("Supply Voltage ({0}) * Voltage Drop ({1})", SupplyVoltage, Project.VoltageDrop);

  766             break;

  767 

  768         default:

  769             throw new ApplicationException("Could not determine a known segment configuration.");

  770     }

  771 }

Basically it's going through a series of segments (parts of a cable on a line) and figuring out what the segment configuration should be. Once it figures that out (with our fabulous enum) it then goes through a case statement to update the number of cables in the segement and the voltage required to heat it. This is from an application where the domain is all about electrical heat on cables.

Lets get started on the refactoring. Inside that tight loop of segments, we'll call out to get the context (our strategy container) with a method we extracted called DetermineCableCountAndHeatSegment. Here's the replacement of the Dumb/Dumber if statement:

  710 SegmentConfigurationContext context = DetermineCableCountAndHeatSegmentVoltage(i, segmentCount, segment, segmentPercentage);

  711 int cableCount = context.Configuration.CalculateCableCount();

  712 decimal heatSegmentVoltage = context.Configuration.CalculateVoltage();

And here's the actual extracted method:

  765 private SegmentConfigurationContext DetermineCableCountAndHeatSegmentVoltage(int i, int segmentCount, ISegment segment, decimal segmentPercentage)

  766 {

  767     SegmentConfigurationContext context = new SegmentConfigurationContext();

  768 

  769     int cableSegmentCount = segment.GetCableCount();

  770 

  771     if(segmentCount > 1)

  772     {

  773         if (cableSegmentCount == 1)

  774         {

  775             if (PassesCount > 1 && (i <= (segmentCount - 2)))

  776             {

  777                 context.Configuration = new MultiPassConfiguration(PassesCount, SupplyVoltage, Project.VoltageDrop, segmentPercentage);

  778             }

  779             else

  780             {

  781                 context.Configuration = new SinglePassConfiguration(cableSegmentCount, SupplyVoltage, Project.VoltageDrop, segmentPercentage);

  782             }

  783         }

  784         else

  785         {

  786             context.Configuration = new MultipleCableCountConfiguration(cableSegmentCount, SupplyVoltage, Project.VoltageDrop, segmentPercentage, PassesCount);   

  787         }

  788     }

  789     else

  790     {

  791         context.Configuration = new DefaultSegmentConfiguration(1, SupplyVoltage, Project.VoltageDrop);                       

  792     }

  793 

  794     return context;

  795 }

The if statement is still there, but now it's a little easier to read in a method and makes more sense overall. We can tell now, for example, if there's more than 1 cableSegmentCount we use the MultipleCableCountConfiguration strategy object. Okay, not as clean as I would like it but it's a step forward over a case statement.

The configuration classes are the strategy implementation. First here's the interface, ISegmentConfiguration:

    3 public interface ISegmentConfiguration

    4 {

    5     int CalculateCableCount();

    6     decimal CalculateVoltage();

    7 }

This just gives us two methods to return the count of the cables and the voltage for a given segment.

Then we create concrete implementations for each strategy. Here's the Simplest one, the DefaultSegmentConfiguration:

    3 class DefaultSegmentConfiguration : SegmentConfiguration

    4 {

    5     public DefaultSegmentConfiguration(int cableCount, int supplyVoltage, decimal voltageDrop)

    6     {

    7         CableCount = cableCount;

    8         SupplyVoltage = supplyVoltage;

    9         VoltageDrop = voltageDrop;

   10     }

   11 

   12     public override int CalculateCableCount()

   13     {

   14         AuditStep("Cable Count: {0}", CableCount);

   15         AuditStep("");

   16         return CableCount;

   17     }

   18 

   19     public override decimal CalculateVoltage()

   20     {

   21         decimal heatSegmentVoltage = SupplyVoltage * VoltageDrop;

   22         AuditStep("Supply Voltage ({0}) * Voltage Drop ({1})", SupplyVoltage, VoltageDrop);

   23         return heatSegmentVoltage;

   24     }

   25 }

Here it just implements those methods to return the values we want. DefaultSegmentConfiguration inherits from SegmentConfiguration which looks like this:

    5 internal abstract class SegmentConfiguration : DomainBase, ISegmentConfiguration

    6 {

    7     protected int CableCount;

    8     protected int SupplyVoltage;

    9     protected decimal VoltageDrop;

   10     protected decimal SegmentPercentage;

   11     protected int PassesCount;

   12     public abstract int CalculateCableCount();

   13     public abstract decimal CalculateVoltage();

   14 }

This provides protected values for the sub-classes to use during the calculation and abstract methods to fulfil the ISegmentConfiguration contract. There's also a requirement to audit information in a log along the way so these classes derive from DomainBase where there's an AuditStep method (we're looking at using AOP to replace all the ugly "log the domain" code).

Now we have multiple configuration classes that handle simple stuff. Calculate cable count and voltage. This lets us focus on the algorithm for each and return the value needed by the caller. Other implementations of ISegmentConfiguration will handle the CalculateVoltage method differently based on how many cables there are, voltage, etc.

Like I said, it's a start and puts us in a better place to test each configuration now rather than that ugly case statement (that's practically untestable). Its also clearer for new people coming onto the project as they [should] be able to pick up on what the code is doing. Tests will help strengthen this and make the use of the classes much more succinct. More refactorings that could be done here:

  • Get rid of that original if statement, however this might be a bigger problem as it bubbles up to some of the parent classes. At the very least, it could be simplified and still meet the business problem.
  • Since all ISegmentConfiguration classes return the cable count, maybe this should just be a property as there's no real calculation involved here for the cable count.

Feel free to suggest even more improvements if you see them!

Published Monday, June 11, 2007 2:56 PM by Bil Simser
Filed under:

Comments

# re: Refactoring Dumb, Dumber, Dumbest away

Monday, June 11, 2007 6:20 PM by foobar
It's still pretty ugly. You can factor away the if statements by using a factory.

# re: Refactoring Dumb, Dumber, Dumbest away

Monday, June 11, 2007 9:20 PM by Simone Busoli
To achieve some decoupling in the creation of SegmentConfiguration implementations and to follow the SOC principle you may consider delegating the operation to a factory - i.e. the factory method pattern. This would shield you from eventual changes in the implementations or the rules which drive the creation of the implementations.

# re: Refactoring Dumb, Dumber, Dumbest away

Tuesday, June 12, 2007 9:44 AM by Bil Simser

Thanks for the advice guys. So I'm just looking at the code and in my head all I'm seeing is a factory (say SegmentConfigurationFactory) which encapsulates that if statement? Not sure how the factory helps me any, other than introducing SOC (which is always a good thing). Or am I missing something?

# re: Refactoring Dumb, Dumber, Dumbest away

Tuesday, June 12, 2007 8:37 PM by Bil Simser

@JP: as with anything, code evolves (and finds a way). I've since replaced the if statements with a factory but for sure step 2 will be to ditch the context object and just return the configuration and get the values out of it. Also I renamed the "i" variable to segmentIndex which is what it represents. I'll probably post a Refactoring Part II follow-up. I agree 100% about the small things, they always go a long way to readibility and maintainability.

# re: Refactoring Dumb, Dumber, Dumbest away

Tuesday, June 12, 2007 9:35 PM by Simone Busoli
"Not sure how the factory helps me any, other than introducing SOC (which is always a good thing). Or am I missing something?" Using a factory to create objects lets you forget about how those objects are in the end created and centralize the creation logic. Suppose you need to create an ISegmentConfiguration implementation somewhere else. Without a factory which abstracts its creation you'll have to reimplement the creation logic. Just by extracting the factory method to a factory class you get a reusable creation logic for free, plus a class which explicitly does just that and which you can find easily in case you need to vary its behavior.

# re: Refactoring Dumb, Dumber, Dumbest away

Tuesday, June 12, 2007 10:50 PM by Bil Simser

@Simone: I posted the comment before I did a quick factory to see what it looked like (I'm a visual kinda guy) and it looked much better than how I saw it in my head. Like I said with JPs comment, I'll probably post a Part II to this refactoring and that includes the factory which should be a good post showing various code smells and different patterns to implement a more maintainable system. Thanks.