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!