Images in this post missing? We recently lost them in a site migration. We're working to restore these as you read this. Should you need an image in an emergency, please contact us at imagehelp@codebetter.com
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:

Public Class BadClass
    Private ord As Order
    Public Sub New(ByVal inOrder as Order)
        ord = inOrder
    End Sub 

    Public Function CalculateOrderTotal() As Decimal
       ' this first part adds up the cost of all the products
        Dim total As Decimal = 0D
        For Each prod In ord.Products
            If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then
                If prod.ID = 745 Or prod.ID = 265 Then
                    total += 11.5D
                Else
                    total +=  prod.Price * 0.8
                End If
            ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then
                If prod.ID = 745 Or prod.ID = 265 Then
                    total += prod.Price * 0.7                Else
                    total += prod.Price * 0.9
                End If
            Else
                If prod.ID = 745 Or prod.ID = 265 Then
                    total += prod.Price * 0.9
                Else
                    If DateTime.Now > New DateTime(2009, 1, 18) AndAlso DateTime.Now < New DateTime(2009, 2, 18) Then
                        total += prod.Price - 3
                    Else
                        total += prod.Price
                    End If
                End If
            End If
        Next
        ' this next section adds in the shipping
        If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then
            Return total
        ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then
            Return total + 3.5D
        Else
            If total < 20 Then
                Return total + 1.4D
            ElseIf total < 40 Then
                Return total + 2.5D
            ElseIf total < 70 Then
                Return total + 5D
            Else
                Return total + 8D
            End If
        End If
    End Function
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:

Public Class BetterClass
  Private ord As Order
    Public Sub New(ByVal inOrder as Order)
        ord = inOrder
    End Sub

    Public Function CalculateOrderTotal() As Decimal
        ' this first part adds up the cost of all the products
        Dim total As Decimal = 0D
        For Each prod In ord.Products
            If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then
                If ProductIsMegaDeal(prod) Then
                    total += 11.5D
                Else
                    total += prod.Price * 0.8
                End If
            ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then
                If ProductIsMegaDeal(prod) Then
                    total += prod.Price * 0.7
                Else
                    total += prod.Price * 0.9
                End If
            Else
                If ProductIsMegaDeal(prod) Then
                    total += prod.Price * 0.9
                Else
                    If IsDealDays() Then
                        total += prod.Price - 3
                    Else
                        total += prod.Price
                    End If
                End If
            End If
        Next
        Return total + CalculateShipping(ord, total)
    End Function

    Public Function CalculateShipping(ByVal total As Decimal) As Decimal
        ' this next section adds in the shipping
        If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then
            Return 0
        ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then
            Return 3.5D
        Else
            If total < 20 Then
                Return 1.4D
            ElseIf total < 40 Then
                Return 2.5D
            ElseIf total < 70 Then
                Return 5D
            Else
                Return 8D
            End If
        End If
    End Function

    Public Function ProductIsMegaDeal(ByVal prod As Product) As Boolean
        If prod.ID = 745 Or prod.ID = 265 Then
            Return True
        End If
        Return False
    End Function

    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

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:

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
            If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then
                If ProductIsMegaDeal(prod) Then
                    total += 11.5D
                Else
                    total += prod.Price * 0.8
                End If
            ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then
                If ProductIsMegaDeal(prod) Then
                    total += prod.Price * 0.7
                Else
                    total += prod.Price * 0.9
                End If
            Else
                If ProductIsMegaDeal(prod) Then
                    total += prod.Price * 0.9
                Else
                    Dim ddService = New DealDayService()
                    If ddService.IsDealDays() Then
                        total += prod.Price - 3
                    Else
                        total += prod.Price
                    End If
                End If
            End If
        Next
        Dim shipService As New ShippingService
        Return total + shipService.CalculateShipping(ord, total)
    End Function

    Public Function ProductIsMegaDeal(ByVal prod As Product) As Boolean
        If prod.ID = 745 Or prod.ID = 265 Then
            Return True
        End If
        Return False
    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 ord As Order, ByVal total As Decimal) As Decimal
        ' this next section adds in the shipping
        If ord.OrderedBy.CustomerLevel = CustomerLevel.Gold Then
            Return 0
        ElseIf ord.OrderedBy.CustomerLevel = CustomerLevel.Silver Then
            Return 3.5D
        Else
            If total < 20 Then
                Return 1.4D
            ElseIf total < 40 Then
                Return 2.5D
            ElseIf total < 70 Then
                Return 5D
            Else
                Return 8D
            End If
        End If
    End Function
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.


Posted 01-20-2009 7:52 PM by Anne Epstein
Filed under:

[Advertisement]

Comments

Dew Drop - January 26, 2008 | Alvin Ashcraft's Morning Dew wrote Dew Drop - January 26, 2008 | Alvin Ashcraft's Morning Dew
on 01-26-2009 3:26 PM

Pingback from  Dew Drop - January 26, 2008 | Alvin Ashcraft's Morning Dew

OJ wrote re: VB.Net and the Spaghetti Code Of Doom
on 01-27-2009 1:24 AM

Don't you mean:

VB.Net == the Spaghetti Code Of Doom

?

;-)

Serge Baranovsky wrote re: VB.Net and the Spaghetti Code Of Doom
on 01-27-2009 11:38 AM

And what does this have to do with VB.NET? One can't do crappy code in other languages? It's not the language but the person who writes the code...

Serge Baranovsky wrote re: VB.Net and the Spaghetti Code Of Doom
on 01-27-2009 11:40 AM

Good article, btw, teaching great principles. It's the title that's controversial. Or is that on purpose? :)

Anne Epstein wrote re: VB.Net and the Spaghetti Code Of Doom
on 01-27-2009 4:20 PM

OJ: All joking aside, take a glance at my intro to the series. Despite the reputation VB.Net has gotten, particularly among C# developers, it's definitely possible to write well-structured maintainable code in VB. Net.

Serge: Agreed: strictly speaking, my article has nothing to do with VB.Net.  However, it's mostly aimed at VB.Net developers who may be new to these concepts, and who may find guiding examples in their language of choice to be helpful.  Just doing my part to lower the barrier of entry...

Simon Brangwin wrote re: VB.Net and the Spaghetti Code Of Doom
on 01-29-2009 11:02 PM

Personally, I hate to see any 'If' statements at all in code. Most of the time they signify code smell. Conditional code like that flies in the face of SRP and the "Open/Closed" principle - Open for extensibility, closed for modification.

Would another solution be to use the Visitor pattern and double dispatch based on product type using subclasses to differentiate MegaDealProduct from regular Product?

This way, when you add UberDealProduct you won't need add another 'If' branch to modify CalculateOrderTotal() and break the Open/Closed principle. Or would that just be adding unnecessary complexity?

joeTheProgrammer wrote re: VB.Net and the Spaghetti Code Of Doom
on 02-04-2009 2:12 PM

You code is not structured (i.e. it's Spaghetti).  A function should only have ONE return statement, etc.  Look up "Structured Programming". Like ...

#     Public Function ProductIsMegaDeal(ByVal prod As Product) As Boolean

#         Return prod.ID = 745 Or prod.ID = 265 'Why are these magic numbers. Explain.

#     End Function

# End Class '

#

# Public Class DealDayService

#     Public Function IsDealDays() As Boolean

#       Dim dttmNow As DateTime = DateTime.Now()

#       Dim dttmBeginDeal As DateTime = New DateTime(2009, 1, 18)

#       Dim dttmEndDeal As DateTime = New DateTime(2009, 2, 18)

#       Return dttmNow > dttmBeginDeal AndAlso dttmNow < dttmEndDeal

#     End Function '

# End Class 'DealDayService

#

# Public Class ShippingService

#     Public Function CalculateShipping(ByVal ord As Order, ByVal total As Decimal) As Decimal

#         ' this next section adds in the shipping

#       Dim decShippingCost AS Decimal = 0

#       Select Case ord.OrderedBy.CustomerLevel

#          Case CustomerLevel.Gold

#            decShippingCost = 0

#          Case CustomerLevel.Silver

#            decShippingCost = 3.5

#          Case Else 'Neither Gold nor Silver

#            Select Case total

#              Case Is < 20

#                decShippingCost = 1.4

#              Case Is < 40

#                decShippingCost  = 2.5

#              Case Is < 70

#                decShippingCost  = 5.0

#              Case Else

#                decShippingCost = 8.0

#            End Select 'total

#       End Select 'CustomerLevel

#       Return decShippingCost

#     End Function 'CalculateShipping

# End Class 'ShippingService

ajepst wrote re: VB.Net and the Spaghetti Code Of Doom
on 02-06-2009 11:23 AM

As I said in the beginning of the post, this is covering a single step of refactoring, illustrating a specific idea, SRP in this case.  The next post in the series will show one technique for addressing all of the If statement spaghetti.  

Fred The\\ wrote re: VB.Net and the Spaghetti Code Of Doom
on 04-02-2009 9:20 PM

"..........So, how can we lower the scare factor here? The Single Responsibility Principle (www.objectmentor.com/.../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."..............."

Quote from the first page of the above referenced .pdf

"None but Buddha himself must take the responsibility of giving out occult secrets...

— E. Cobham Brewer 1810–1897.

Dictionary of Phrase and Fable. 1898."

WTF? Is that a joke?

Here's the Wikipedia to-the-point "occult secret": en.wikipedia.org/.../Single_responsibility_principle

Setting the humor issue aside; as far as the core of Anne's argument, I think this snippet from a conversation between Joel Spolsky and Robert Martin  sums up the debate:

<Transcript Excerpt>

   Spolsky: Bob, can you explain again the Single Responsibility principle? Because I don’t think I understand it right.

   Martin: The Single Responsibility principle is actually a very old principle, I think it was coined by Bertrand Meyer a long time ago. The basic idea is simple, if you have a module or a function or anything it should have one reason to change. And by that I mean that if there is some other source of change… if there are sources of change out there one source of change should impact it. So a simple example, we have an employee, this is the one I use all the time…

   Spolsky: Wait, wait, hold on, let me stop you for a second. Change, you mean like at run-time?

   Martin: No. At development time.

   Spolsky: You mean changes of code. There should be one source of entropy in the world which causes you to have to change the source code for that thing.

   Martin: Yeah. That’s the idea. Do you ever achieve that? No.

   Martin: You try to get your modules so that if a feature changes a module might change but no other feature change will affect that module. You try and get your modules so partitioned that when a change occurs in the requirements the minimum possible number of modules are affected. So the example I always use is the employee class. Should an employee know how to write itself to the database, how to calculate it’s pay and how to write an employee report. And, if you had all of that functionality inside an employee class then when the accountants change the business rules for calculating pay you’d have to change the employee, you’d have to modify the code in the employee. If the bean counters change the format of the report you’d have to go in and change this class, or if the DBAs changed the schema you’d have to go in and change this class. I call classes like this dependency magnets, they change for too many reasons.

   Spolsky: Is that… how is that bad?

</Transcript Excerpt>

Transcript source. www.equivalence.co.uk/.../769

Notice in the additional commentary in the above link, the author points out that even Mr. Martin does not adhere to such a rigid definition of the S in SOLID. Here he represents a plugin as such:

"The discussion in SO #41(blog.stackoverflow.com/.../podcast-41) then moves on to the Wordpress plugin system where Bob comments on how having independent plugins are a great example of the single point of responsibility (I must stress it was not Bob’s example, he just went with it). Eh NO. I think that it sometimes becomes easy to forget what the S means. Let me just state it again: A CLASS should have one, and only one, reason to change. The plugin itself, which is represented by a class, may have lots of reasons to change. Again in the subsequent Hansel Minutes podcast Bob uses the term module, in the sense of say a jar file in Java or a dll in .NET, to describe building a set of components that adhere to the S in the SOLID principles. This is once again deviating away from the actual meaning of S.

OK, am I being pedantic about the definition? I don’t think so. If it doesn’t mean what it says, then change it; there are people out there that will follow these rules blindly, and having such a specific definition only encourages this."

It must be pointed out that Spolsky & Martin really do look at software development from very different perspectives and sometimes the conversations get rather heated: blog.objectmentor.com/.../on-open-letter-to-joel-spolsky-and-jeff-atwood

I can't help but agree with Joel on this point: ”...it seems to me like a lot of the Object Oriented Design principles you’re hearing lately from people like Robert Martin and Kent Beck and so forth have gone off the deep end into architecture for architecture’s sake.”

Jonadab wrote re: VB.Net and the Spaghetti Code Of Doom
on 05-04-2009 8:47 AM

I don't think the function that totals up the order should be calculating the product discounts like that.  The minute you want to write an invoice-printing routine that lists each item's price, you're going to duplicate that code, and then six months later some poor fool of a maintenance programmer who doesn't know the code very well gets a change requests, finds the place in the code with the discounts, makes the change, and all of a sudden the line items on the invoices don't match the totals.   Or maybe you write a function that tells the customer the prices of the individual items, and they look up the prices of six items, and then when they place the order the total doesn't match, because, again, the guy who didn't know the code very well only changed it in one place.

The discounts need to be calculated in a separate function.  The order-totaling code should then just call this function to find out the cost of each item (for this customer, with these other things on the order, on this date).

Anne Epstein wrote VB.Net and the Case of the Iffy Ifs
on 05-12-2009 2:12 AM

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

Sean wrote re: VB.Net and the Spaghetti Code Of Doom
on 05-12-2009 10:30 AM

@joeTheProgrammer --  dttmNow? decShippingCost? dttmBeginDeal? Reminds me of VBA/VB6.

Please, please lose the Hungarian notation already. It is useless, unsightly and I cringe every time I see it.  

Mmpiprta wrote re: VB.Net and the Spaghetti Code Of Doom
on 06-23-2009 5:24 AM

E9NkXi comment5 ,

Joe Holt wrote re: VB.Net and the Spaghetti Code Of Doom
on 06-27-2010 10:40 PM

I added the UberDealProduct and the 'If' branch to modify CalculateOrderTotal()is not needed when the break the Open/Closed principle is used.

xgmp porn video k0p2 wrote re: VB.Net and the Spaghetti Code Of Doom
on 07-02-2011 11:55 PM

Vb net and the spaghetti ball of doom.. Slap-up :)

interior design wrote re: VB.Net and the Spaghetti Code Of Doom
on 12-30-2011 3:05 AM

I love the archaic version. Lancelot is related injuries, hard eyes that Symmetria e. And the flying fish! I also like the ending (very happy that he does not hide completely behind the plate this time), so it's nice to be able to see both, see the choice

bglfsbjlfns@gmail.com wrote re: VB.Net and the Spaghetti Code Of Doom
on 07-10-2014 12:42 AM

The quality is excellent! I loved the bag. Syagkaya skin and pleasant to the touch. Size is just right. Love it. I recommend. Reached in 34 days.The quality of the bag is excellent, quick delivery to Russia 2.5 weeks.

link building wrote re: VB.Net and the Spaghetti Code Of Doom
on 07-18-2014 10:33 PM

fxvini I truly appreciate this blog post.Really looking forward to read more. Really Great.

john wrote re: VB.Net and the Spaghetti Code Of Doom
on 10-04-2014 11:08 PM

Add a Comment

(required)  
(optional)
(required)  
Remember Me?

About The CodeBetter.Com Blog Network
CodeBetter.Com FAQ

Our Mission

Advertisers should contact Brendan

Subscribe
Google Reader or Homepage

del.icio.us CodeBetter.com Latest Items
Add to My Yahoo!
Subscribe with Bloglines
Subscribe in NewsGator Online
Subscribe with myFeedster
Add to My AOL
Furl CodeBetter.com Latest Items
Subscribe in Rojo

Member Projects
DimeCasts.Net - Derik Whittaker

Friends of Devlicio.us
Red-Gate Tools For SQL and .NET

NDepend

SlickEdit
 
SmartInspect .NET Logging
NGEDIT: ViEmu and Codekana
LiteAccounting.Com
DevExpress
Fixx
NHibernate Profiler
Unfuddle
Balsamiq Mockups
Scrumy
JetBrains - ReSharper
Umbraco
NServiceBus
RavenDb
Web Sequence Diagrams
Ducksboard<-- NEW Friend!

 



Site Copyright © 2007 CodeBetter.Com
Content Copyright Individual Bloggers

 

Community Server (Commercial Edition)