VB.Net and the Spaghetti Code Of Doom

(NOTE: This post examines a only few principles, and addresses them in isolation. Learning is a stepwise process, and I want to cater to that.)

Developers everywhere want code that is easy to understand, and easy to change to take new requirements into account. Usually, code starts out simple, but over time, as developers add new rules and special exceptions, code can become confusing. Over time the code may reach a point where maintainers are afraid to go into sections of the code because they’re only guessing as to what things to, and unsure what a change could break in other parts of the system. Take a look at this class, which exhibits some of the typical problems:

view plaincopy to clipboardprint?

1. Public Class BadClass  
2.     Private ord As Order  
3.     Public Sub New(ByVal inOrder as Order)  
4.         ord = inOrder  
5.     End Sub   
6.   
7.     Public Function CalculateOrderTotal() As Decimal  
8.        ‘ this first part adds up the cost of all the products  
9.         Dim total As Decimal = 0D  
10.         For Each prod In ord.Products  
11.             If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then  
12.                 If prod.ID = 745 Or prod.ID = 265 Then  
13.                     total += 11.5D  
14.                 Else  
15.                     total +=  prod.Price * 0.8  
16.                 End If  
17.             ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then  
18.                 If prod.ID = 745 Or prod.ID = 265 Then  
19.                     total += prod.Price * 0.7                Else  
20.                     total += prod.Price * 0.9  
21.                 End If  
22.             Else  
23.                 If prod.ID = 745 Or prod.ID = 265 Then  
24.                     total += prod.Price * 0.9  
25.                 Else  
26.                     If DateTime.Now > New DateTime(2009, 1, 18) AndAlso DateTime.Now < New DateTime(2009, 2, 18) Then  
27.                         total += prod.Price – 3  
28.                     Else  
29.                         total += prod.Price  
30.                     End If  
31.                 End If  
32.             End If  
33.         Next  
34.         ‘ this next section adds in the shipping  
35.         If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then  
36.             Return total  
37.         ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then  
38.             Return total + 3.5D  
39.         Else  
40.             If total < 20 Then  
41.                 Return total + 1.4D  
42.             ElseIf total < 40 Then  
43.                 Return total + 2.5D  
44.             ElseIf total < 70 Then  
45.                 Return total + 5D  
46.             Else  
47.                 Return total + 8D  
48.             End If  
49.         End If  
50.     End Function  
51. End Class  

So, why these 745 and 265 numbers ? They look like product Ids. What if a new product needs to be added? Also, the lower section appears to calculate shipping, but what if we want to calculate shipping elsewhere? What if we have a new kind of CustomerLevel? This class looks complicated, and I’m a little worried that if I make changes, I could break something. If we continue adding features to this class as it is, we’ll only be making things scarier for next time.  When we mess with one section of this to add a new feature, we’re risking code that’s already live, so that we can’t have confidence that it still works.

Let’s take a first hack at fixing this:

view plaincopy to clipboardprint?

1. Public Class BetterClass  
2.   Private ord As Order  
3.     Public Sub New(ByVal inOrder as Order)  
4.         ord = inOrder  
5.     End Sub  
6.   
7.     Public Function CalculateOrderTotal() As Decimal  
8.         ‘ this first part adds up the cost of all the products  
9.         Dim total As Decimal = 0D  
10.         For Each prod In ord.Products  
11.             If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then  
12.                 If ProductIsMegaDeal(prod) Then  
13.                     total += 11.5D  
14.                 Else  
15.                     total += prod.Price * 0.8  
16.                 End If  
17.             ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then  
18.                 If ProductIsMegaDeal(prod) Then  
19.                     total += prod.Price * 0.7  
20.                 Else  
21.                     total += prod.Price * 0.9  
22.                 End If  
23.             Else  
24.                 If ProductIsMegaDeal(prod) Then  
25.                     total += prod.Price * 0.9  
26.                 Else  
27.                     If IsDealDays() Then  
28.                         total += prod.Price – 3  
29.                     Else  
30.                         total += prod.Price  
31.                     End If  
32.                 End If  
33.             End If  
34.         Next  
35.         Return total + CalculateShipping(ord, total)  
36.     End Function  
37.   
38.     Public Function CalculateShipping(ByVal total As Decimal) As Decimal  
39.         ‘ this next section adds in the shipping  
40.         If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then  
41.             Return 0  
42.         ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then  
43.             Return 3.5D  
44.         Else  
45.             If total < 20 Then  
46.                 Return 1.4D  
47.             ElseIf total < 40 Then  
48.                 Return 2.5D  
49.             ElseIf total < 70 Then  
50.                 Return 5D  
51.             Else  
52.                 Return 8D  
53.             End If  
54.         End If  
55.     End Function  
56.   
57.     Public Function ProductIsMegaDeal(ByVal prod As Product) As Boolean  
58.         If prod.ID = 745 Or prod.ID = 265 Then  
59.             Return True  
60.         End If  
61.         Return False  
62.     End Function  
63.   
64.     Public Function IsDealDays() As Boolean  
65.         Return DateTime.Now > New DateTime(2009, 1, 18) AndAlso DateTime.Now < New DateTime(2009, 2, 18)  
66.     End Function  
67. End Class  

So, phase one, we’ve grouped this up into a lot of different methods in our class. This is already a lot better-at least I can see what’s going on. However, as all these methods are in the same class, interacting together all over the place, we still have a potential problem. These methods, which might be used in different contexts for different purposes throughout the system are all in the same class, where they might share private variables.(In fact, they share “ord”) If one method is called, it could unexpectedly affect the way other methods act in different circumstances. The methods may even act differently depending on the order they are called, which could make behavior more unexpected. It’s definitely easier to read than before, but because of this entanglement, there’s still a concern that changes could break things you never expected, and never intended.

So, how can we lower the scare factor here? The Single Responsibility Principle (http://www.objectmentor.com/resources/articles/srp.pdf) offers us a way to help clean things up. This principle states “THERE SHOULD NEVER BE MORE THAN ONE REASON FOR A CLASS TO CHANGE.” We can see that in the code above, there are lots of reasons for this class to change, so when we work in one method, its interactions with private methods and variables could break other unrelated methods in the same class… and so other, should-be-unaffected methods/features are at risk of breaking. As these methods are all together and all bound up together, NONE of them may ever stay in a state where we leave them alone for long periods of time, tested and stable… instead, we’ll keep changing the class, and as we do so, we’re introducing risk and possible confusion into things that have nothing to do with the tweak of the day. SRP can help lower this risk, but what would putting SRP into this code mean? Let’s have a go at chopping this up, keeping SRP in mind:

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.             If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then  
7.                 If ProductIsMegaDeal(prod) Then  
8.                     total += 11.5D  
9.                 Else  
10.                     total += prod.Price * 0.8  
11.                 End If  
12.             ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then  
13.                 If ProductIsMegaDeal(prod) Then  
14.                     total += prod.Price * 0.7  
15.                 Else  
16.                     total += prod.Price * 0.9  
17.                 End If  
18.             Else  
19.                 If ProductIsMegaDeal(prod) Then  
20.                     total += prod.Price * 0.9  
21.                 Else  
22.                     Dim ddService = New DealDayService()  
23.                     If ddService.IsDealDays() Then  
24.                         total += prod.Price – 3  
25.                     Else  
26.                         total += prod.Price  
27.                     End If  
28.                 End If  
29.             End If  
30.         Next  
31.         Dim shipService As New ShippingService  
32.         Return total + shipService.CalculateShipping(ord, total)  
33.     End Function  
34.   
35.     Public Function ProductIsMegaDeal(ByVal prod As Product) As Boolean  
36.         If prod.ID = 745 Or prod.ID = 265 Then  
37.             Return True  
38.         End If  
39.         Return False  
40.     End Function  
41. End Class  
42.   
43. Public Class DealDayService  
44.     Public Function IsDealDays() As Boolean  
45.         Return DateTime.Now > New DateTime(2009, 1, 18) AndAlso DateTime.Now < New DateTime(2009, 2, 18)  
46.     End Function  
47. End Class  
48.   
49. Public Class ShippingService  
50.     Public Function CalculateShipping(ByVal ord As Order, ByVal total As Decimal) As Decimal  
51.         ‘ this next section adds in the shipping  
52.         If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then  
53.             Return 0  
54.         ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then  
55.             Return 3.5D  
56.         Else  
57.             If total < 20 Then  
58.                 Return 1.4D  
59.             ElseIf total < 40 Then  
60.                 Return 2.5D  
61.             ElseIf total < 70 Then  
62.                 Return 5D  
63.             Else  
64.                 Return 8D  
65.             End If  
66.         End If  
67.     End Function  
68. End Class  

That’s getting better- the shipping code is now completely independent of the OrderTotal Code, and the DealDay calculation, which could also be used in all kinds of situations having nothing to do with totaling orders is also pulled out. These classes now are definitely unaffected by any changes in the CalculateOrderTotal function. As the classes are physically broken apart and separated, they can’t affect each other. Noteworthy is the MegaDeal code, which didn’t seem to be an independent service-level calculation, but it doesn’t seem to belong where it still is. Also, every time we need to change the actions of a CustomerLevel, or add a new CustomerLevel, we’re going to have to make changes to this file… and each time we touch this file, we could break things for existing customer types. If the company decides they want a lot of customer levels, this file could get very big and hard to debug. Also of note is that the OrderTotalService only seems to be able to run against one shipping calculation tool-if, let’s say we end up with multiple shipping vendors in the future that require us to have different shipping calculators, we’re going to go back and change this code. Testing looks like it could be complicated as well. I’ll examine those issues and how to address them in future parts of this series.