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:

Public Interface ICustomer
    Property CompanyName() As String
    Property ContactName() As String
    Property Orders() As IList(Of Order)
    Sub AddOrder(ByVal order As Order)
    Sub RemoveOrder(ByVal order As Order)
    Function CalculateProductPrice(ByVal prod As Product) As Decimal
    Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal
End Interface

Public Class GoldCustomer
    Inherits Customer
    Implements ICustomer

    Public Overrides Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice
        If prod.IsMegaDeal Then
            Return 11.5D
        Else
            Return prod.Price * 0.8
        End If
    End Function
    Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping
        Return 0D
    End Function
End Class

Public Class SilverCustomer
    Inherits Customer
    Implements ICustomer

    Public Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice
        If prod.IsMegaDeal Then
            Return prod.Price * 0.7
        Else
            Return prod.Price * 0.9
        End If
    End Function

    Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping
        Return 3.5D
    End Function
End Class

Public Class BasicCustomer
    Inherits Customer
    Implements ICustomer

    Public Overrides Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice
        If prod.IsMegaDeal Then
            Return prod.Price * 0.9
        Else
            Dim ddService = New DealDayService()
            If ddService.IsDealDays() Then
                Return (prod.Price - 3)
            End If
        End If
    End Function
    Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping
        If orderTotal < 20 Then
            Return 1.4D
        ElseIf orderTotal < 40 Then
            Return 2.5D
        ElseIf orderTotal < 70 Then
            Return 5D
        Else
            Return 8D
        End If
    End Function
End Class

Public MustInherit Class Customer
    Implements ICustomer

    ... (original methods) ...


    Public MustOverride Function CalculateProductPrice(ByVal prod As Product) As Decimal _
        Implements ICustomer.CalculateProductPrice
    Public MustOverride Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal _
        Implements ICustomer.CalculateShipping
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.

Public Class OrderTotalService
    Public Function CalculateOrderTotal(ByVal ord As Order) As Decimal
        ' this first part adds up the cost of all the products
        Dim total As Decimal = 0D
        For Each prod In ord.Products
            total += ord.OrderedBy.CalculateProductPrice(prod)
        Next

        Dim shipService As New ShippingService
        Return shipService.CalculateShipping(ord.OrderedBy, total) + total
    End Function
End Class

Public Class DealDayService
    Public Function IsDealDays() As Boolean
        Return DateTime.Now > New DateTime(2009, 1, 18) AndAlso DateTime.Now < New DateTime(2009, 2, 18)
    End Function
End Class

Public Class ShippingService
    Public Function CalculateShipping(ByVal cust As ICustomer, ByVal total As Decimal) As Decimal
        ' this next section adds in the shipping
        Return cust.CalculateShipping(total)
    End Function
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)

Private _isMegaDeal As Boolean
Public Property IsMegaDeal() As Boolean
     Get
        Return _isMegaDeal
     End Get
     Set(ByVal value As Boolean)
         _isMegaDeal = value
     End Set
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:

Public Class GoldCustomer
    Inherits Customer
    Implements ICustomer 

    Public Overrides Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice
        If prod.IsMegaDeal Then Return 11.5D
        Return prod.Price * 0.8
    End Function 

    Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping
        Return 0
    End Function
End Class

 
Public Class SilverCustomer
    Inherits Customer
    Implements ICustomer

    Public Overrides Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice
        If prod.IsMegaDeal Then Return prod.Price * 0.7
        Return prod.Price * 0.9
    End Function

    Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping
        Return 3.5D
    End Function
End Class

 
Public Class BasicCustomer
    Inherits Customer
    Implements ICustomer

    Public Overrides Function CalculateProductPrice(ByVal prod As Product) As Decimal Implements ICustomer.CalculateProductPrice
        If prod.IsMegaDeal Then Return prod.Price * 0.9
        Dim ddService = New DealDayService()
        If ddService.IsDealDays() Then Return (prod.Price - 3)
    End Function

    Public Overrides Function CalculateShipping(ByVal orderTotal As Decimal) As Decimal Implements ICustomer.CalculateShipping
        If orderTotal < 20 Then Return 1.4D
        If orderTotal < 40 Then Return 2.5D
        If orderTotal < 70 Then Return 5D
        Return 8D
    End Function
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.