VB.Net and the Case of the Iffy Ifs

Previous Items in Series:
Intro to series on quality VB.Net
VB.Net and the Spaghetti Code Of Doom

In the previous item in this series, we looked at a pretty typical class, discussed some of its problems, and started in on fixing those problems.  We finished with smaller, easier-to-understand pieces of code, but it still exhibits some notable problems.  Most visible for me are all those branching if/else statements, some of them nested.  This is a potential problem for several reasons:

1. Each time we add/remove/change the customer levels, we’re going to need to change both the OrderTotalService and the ShippingService.   That’s one core change we have to make, and now we’re going to have to update two different pieces of code.  Hopefully, we remember to do that!
2. Each time we go in and mess with any customer level, we have to change these classes.  Hopefully, we change the classes safely and don’t accidentally destabilize other customer levels when we’re in there!  This is starting to feel a little risky…
3. Are all these tangled ifs really readable?  they take some mental walking through to see what’s going on.
4. It looks like constructing test data that matches the pattern required to drill through all these conditions could be tricky.  Testing is important, but it’s a lot harder to find the motivation when it seems like it’s going to be an overwhelming task.

So, one way to handle the mess is to try some polymorphism.  The basic idea we’re going to use is that instead of switching on a property of a class, create a set of classes based on that property that share an interface, and move the switched behavior over to the classes. Let’s give that a try. 

The Customer class has been subclassed into three different classes to reflect the Gold, Silver, and Basic customer levels. Then, the customer level-specific product price and shipping calculations have been moved into the subclasses:

view plaincopy to clipboardprint?

1. Public Interface ICustomer  
2.     Property CompanyName() As String  
3.     Property ContactName() As String  
4.     Property Orders() As IList(Of Order)  
5.     Sub AddOrder(ByVal order As Order)  
6.     Sub RemoveOrder(ByVal order As Order)  
7.     Function CalculateProductPrice(ByVal prod As Product) As Decimal  
8.     Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal  
9. End Interface  
10.   
11. Public Class GoldCustomer  
12.     Inherits Customer  
13.     Implements ICustomer  
14.   
15.     Public Overrides Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice  
16.         If prod.IsMegaDeal Then  
17.             Return 11.5D  
18.         Else  
19.             Return prod.Price * 0.8  
20.         End If  
21.     End Function  
22.     Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping  
23.         Return 0D  
24.     End Function  
25. End Class  
26.   
27. Public Class SilverCustomer  
28.     Inherits Customer  
29.     Implements ICustomer  
30.   
31.     Public Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice  
32.         If prod.IsMegaDeal Then  
33.             Return prod.Price * 0.7  
34.         Else  
35.             Return prod.Price * 0.9  
36.         End If  
37.     End Function  
38.   
39.     Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping  
40.         Return 3.5D  
41.     End Function  
42. End Class  
43.   
44. Public Class BasicCustomer  
45.     Inherits Customer  
46.     Implements ICustomer  
47.   
48.     Public Overrides Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice  
49.         If prod.IsMegaDeal Then  
50.             Return prod.Price * 0.9  
51.         Else  
52.             Dim ddService = New DealDayService()  
53.             If ddService.IsDealDays() Then  
54.                 Return (prod.Price – 3)  
55.             End If  
56.         End If  
57.     End Function  
58.     Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping  
59.         If orderTotal < 20 Then  
60.             Return 1.4D  
61.         ElseIf orderTotal < 40 Then  
62.             Return 2.5D  
63.         ElseIf orderTotal < 70 Then  
64.             Return 5D  
65.         Else  
66.             Return 8D  
67.         End If  
68.     End Function  
69. End Class  
70.   
71. Public MustInherit Class Customer  
72.     Implements ICustomer  
73.   
74.     … (original methods) …  
75.   
76.   
77.     Public MustOverride Function CalculateProductPrice(ByVal prod As Product) As Decimal _  
78.         Implements ICustomer.CalculateProductPrice  
79.     Public MustOverride Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal _  
80.         Implements ICustomer.CalculateShipping  
81. End Class  

(A note: we’re seeing the effects of a VB.Net “feature” here: in VB.Net-methods implementing an interface *must* state what it’s implementing from that interface. Customer shouldn’t really need to implement ICustomer-and thus wouldn’t need the two MustOverride methods. The problem is that if Customer didn’t implement ICustomer, then when GoldCustomer, etc used Customer’s methods to implement ICustomer, we’d get a compile error because those method/properties in Customer wouldn’t have the “implements” mapping. We can avoid this issue by making GoldCustomer contain a Customer instead of inheriting from it, but that’s a discussion for a different post)

This shrinks the OrderTotalService to just a few lines, and shrinks the ShippingService to nothing-it would be reasonable to completely remove the ShippingService class in the next refactoring pass.

view plaincopy to clipboardprint?

1. Public Class OrderTotalService  
2.     Public Function CalculateOrderTotal(ByVal ord As Order) As Decimal  
3.         ‘ this first part adds up the cost of all the products  
4.         Dim total As Decimal = 0D  
5.         For Each prod In ord.Products  
6.             total += ord.OrderedBy.CalculateProductPrice(prod)  
7.         Next  
8.   
9.         Dim shipService As New ShippingService  
10.         Return shipService.CalculateShipping(ord.OrderedBy, total) + total  
11.     End Function  
12. End Class  
13.   
14. Public Class DealDayService  
15.     Public Function IsDealDays() As Boolean  
16.         Return DateTime.Now > New DateTime(2009, 1, 18) AndAlso DateTime.Now < New DateTime(2009, 2, 18)  
17.     End Function  
18. End Class  
19.   
20. Public Class ShippingService  
21.     Public Function CalculateShipping(ByVal cust As ICustomer, ByVal total As Decimal) As Decimal  
22.         ‘ this next section adds in the shipping  
23.         Return cust.CalculateShipping(total)  
24.     End Function  
25. End Class  

The last change here is in the Product class-I made a decision that though there were only two hardcoded MegaDeal products, this felt like a group that could change, and I wanted to get those hardcoded values out of there. Instead of being hardcoded, those values are now going to be stored as a flag in Product. (It very well may be that MegaDeals would be better implemented as some way other than as a property the first priority was getting out the hardcoded values-that can be revisited)

view plaincopy to clipboardprint?

1. Private _isMegaDeal As Boolean  
2. Public Property IsMegaDeal() As Boolean  
3.      Get  
4.         Return _isMegaDeal  
5.      End Get  
6.      Set(ByVal value As Boolean)  
7.          _isMegaDeal = value  
8.      End Set  
9. End Property  

Next, I’m going to do a simple change on those inner ifs just to make them a bit easier to visually scan:

view plaincopy to clipboardprint?

1. Public Class GoldCustomer  
2.     Inherits Customer  
3.     Implements ICustomer   
4.   
5.     Public Overrides Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice  
6.         If prod.IsMegaDeal Then Return 11.5D  
7.         Return prod.Price * 0.8  
8.     End Function   
9.   
10.     Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping  
11.         Return 0  
12.     End Function  
13. End Class  
14.   
15.    
16. Public Class SilverCustomer  
17.     Inherits Customer  
18.     Implements ICustomer  
19.   
20.     Public Overrides Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice  
21.         If prod.IsMegaDeal Then Return prod.Price * 0.7  
22.         Return prod.Price * 0.9  
23.     End Function  
24.   
25.     Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping  
26.         Return 3.5D  
27.     End Function  
28. End Class  
29.   
30.    
31. Public Class BasicCustomer  
32.     Inherits Customer  
33.     Implements ICustomer  
34.   
35.     Public Overrides Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice  
36.         If prod.IsMegaDeal Then Return prod.Price * 0.9  
37.         Dim ddService = New DealDayService()  
38.         If ddService.IsDealDays() Then Return (prod.Price – 3)  
39.     End Function  
40.   
41.     Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping  
42.         If orderTotal < 20 Then Return 1.4D  
43.         If orderTotal < 40 Then Return 2.5D  
44.         If orderTotal < 70 Then Return 5D  
45.         Return 8D  
46.     End Function  
47. End Class  

Conclusion:

Getting rid of those ifs made a big difference.

1. Now, if we need to modify a customer level, we can do it in one place, in that level’s class.  No more hoping we got all the places we branch over customer level types!  In addition, if we need to add a new level, *none* of this code needs to change-we just add a new class for that new level that implements ICustomer, and everything that’s here works as-is.
2.  Related to the item #1, when we change a customer level we don’t have to change the places that level is used.  We can test the heck out of what actually *had* to change, the modified or new level, and since nothing else was touched, we won’t have to consider any other exiting code destabilized.
3. Unfortunately, there are still ifs, but we’ve split them out some, so they’re not so nested.  OrderTotalService and ShippingService are a lot cleaner and easier to look at.
4. Testing these classes is a little easier now.  we don’t have to feed as many combinations CalculateOrderTotal now-it treats all customer types the same.  We still have to test that logic in the inheritors of ICustomer, but without that extra layer of ifs, it should be a little clearer what conditions need to be tested.

This code can definitely still be improved!  For one thing, we’ll have to keep an eye on the logic inside the customer level subtypes.  Though the differing computation logic is now split off and close to what it differs on, I’ve got a feeling that code could get complex and overwhelm those classes over time.  The DealDays logic and dates seem to be fixed to a single datespan, but that’s the sort of thing clients want changed regularly, and we also don’t have a way to calculate the order total without the shipping right now. We also still haven’t addressed the upcoming desire to be able to handle pricing for different shipping vendors either.