hell I have been wanting to write this series of articles for a long time. Over the years I have come across very bad code examples and I have always wanted to share them with you guys, kind of in the way of “Watch and DON’T learn” :) .

 

Everything you are about to see in this series is based on my personal experience and completely real, so before we begin I have only one request:

 

Don’t try this at home! :)

 

Ok, lets get started.

 

First, lets take a look at the code. The code is of a real function I found, that determines the color of an object (All variables were changed to keep confidentiality), brace your selves:

 

   1: Public Sub GetSomethingColor(ByVal IntVar1 As Integer, ByVal BoolVar2 As Boolean
   2:                             , ByVal BoolVar3 As Boolean, ByVal BoolVar4 As Boolean
   3:                             , ByVal IntVar5 As Integer, ByVal IntVar6 As Integer
   4:                             , ByRef ClassVar7 As ClassType1, ByVal IntVar8 As Integer
   5:                             , ByVal IntVar9 As Integer
   6:                             , Optional ByVal BoolVar10 As Boolean = False)
   7: Try
   8:     '' some comment 
   9:     If BoolVar10 = True Then
  10:         ClassVar7.PortStatus = SomeColor.Mixed
  11:         If ClassVar7.ConnectedIntVar1_2 = -1 Then
  12:             ClassVar7.ConnectedIntVar1_2 = IntVar8
  13:         End If
  14:         If IntVar6 = 1 Then
  15:             ClassVar7.Port.BackgroundImage = My.Resources.MixedLedOn()
  16:         Else
  17:             ClassVar7.Port.BackgroundImage = My.Resources.MixedLedOff()
  18:         End If
  19:     Else
  20:         If BoolVar2 AndAlso IntVar9 > 0 AndAlso (IntVar9 <> IntVar8) Then
  21:             ClassVar7.PortStatus = SomeColor.Mixed
  22:             ClassVar7.ConnectedIntVar1_2 = IntVar9
  23:             If IntVar6 = 1 Then
  24:                 ClassVar7.Port.BackgroundImage = My.Resources.MixedLedOn()
  25:             Else
  26:                 ClassVar7.Port.BackgroundImage = My.Resources.MixedLedOff()
  27:             End If
  28:         Else
  29:             ClassVar7.ConnectedIntVar1_2 = -1
  30:             If BoolVar2 = False Then
  31:                 If BoolVar3 Then
  32:                     ClassVar7.PortStatus = SomeColor.PatchCordNo_DesignYes_DisconnectWONo
  33:                     If IntVar6 = 1 Then
  34:                         ClassVar7.Port.BackgroundImage = My.Resources._6LedOn()
  35:                     Else
  36:                         ClassVar7.Port.BackgroundImage = My.Resources._6LedOff()
  37:                     End If
  38:                     ClassVar7.ConnectedIntVar1 = IntVar8
  39:                 Else
  40:                     Select Case IntVar5
  41:                         Case -1 ' some comment 
  42:                             ClassVar7.PortStatus = SomeColor.SomethingColor
  43:                             If IntVar6 = 1 Then
  44:                                 ClassVar7.Port.BackgroundImage = My.Resources._1LedOn()
  45:                             Else
  46:                                 ClassVar7.Port.BackgroundImage = My.Resources._1LedOff()
  47:                             End If
  48:                             ClassVar7.ConnectedIntVar1 = 0
  49:                         Case IntVar5.Invalid
  50:                             ClassVar7.PortStatus = SomeColor.SomethingColor2
  51:                             If IntVar6 = 1 Then
  52:                                 ClassVar7.Port.BackgroundImage = My.Resources._2LedOn()
  53:                             Else
  54:                                 ClassVar7.Port.BackgroundImage = My.Resources._2LedOff()
  55:                             End If
  56:                             ClassVar7.ConnectedIntVar1 = IntVar9
  57:                         Case IntVar5.Valid
  58:                             ClassVar7.PortStatus = SomeColor.SomethingColor3
  59:                             If IntVar6 = 1 Then
  60:                                 ClassVar7.Port.BackgroundImage = My.Resources._3LedOn()
  61:                             Else
  62:                                 ClassVar7.Port.BackgroundImage = My.Resources._3LedOff()
  63:                             End If
  64:                             ClassVar7.ConnectedIntVar1 = IntVar9
  65:                         Case IntVar5.Expired
  66:                             ClassVar7.PortStatus = SomeColor.SomethingColor4
  67:                             If IntVar6 = 1 Then
  68:                                 ClassVar7.Port.BackgroundImage = My.Resources._4LedOn()
  69:                             Else
  70:                                 ClassVar7.Port.BackgroundImage = My.Resources._4LedOff()
  71:                             End If
  72:                             ClassVar7.ConnectedIntVar1 = IntVar9
  73:                     End Select
  74:                 End If
  75:             Else
  76:                 If BoolVar3 = False Then ' some comment 
  77:                     Select Case IntVar5
  78:                         Case -1 ' some comment 
  79:                             ClassVar7.PortStatus = SomeColor.SomethingColor5
  80:                             If IntVar6 = 1 Then
  81:                                 ClassVar7.Port.BackgroundImage = My.Resources._5LedOn()
  82:                             Else
  83:                                 ClassVar7.Port.BackgroundImage = My.Resources._5LedOff()
  84:                             End If
  85:                             ClassVar7.ConnectedIntVar1 = IntVar8
  86:                         Case IntVar5.Invalid
  87:                             If BoolVar4 = True Then
  88:                                 'ClassVar7.PortStatus = SomeColor.SomethingColor13 
  89:                                 'If IntVar6 = True Then 
  90:                                 ' ClassVar7.Port.BackgroundImage = My.Resources._6LedOn 
  91:                                 'Else 
  92:                                 ' ClassVar7.Port.BackgroundImage = My.Resources._6LedOff 
  93:                                 'End If 
  94:                                 'ClassVar7.ConnectedIntVar1 = IntVar9 
  95:                             Else ' Disconnect Work-Order in phase 1 
  96:                                 ClassVar7.PortStatus = SomeColor.SomethingColor6
  97:                                 If IntVar6 = 1 Then
  98:                                     ClassVar7.Port.BackgroundImage = My.Resources._7LedOn()
  99:                                 Else
 100:                                     ClassVar7.Port.BackgroundImage = My.Resources._7LedOff()
 101:                                 End If
 102:                                 ClassVar7.ConnectedIntVar1 = IntVar9
 103:                             End If
 104:                         Case IntVar5.Valid
 105:                             ' some comment
 106:                             ' some comment 
 107:                             ClassVar7.PortStatus = SomeColor.SomethingColor7
 108:                             If IntVar6 = 1 Then
 109:                                 ClassVar7.Port.BackgroundImage = My.Resources._8LedOn()
 110:                             Else
 111:                                 ClassVar7.Port.BackgroundImage = My.Resources._8LedOff()
 112:                             End If
 113:                             ClassVar7.ConnectedIntVar1 = IntVar9
 114:                         Case IntVar5.Expired
 115:                             ' some comment 
 116:                             ' some comment 
 117:                             ClassVar7.PortStatus = SomeColor.SomethingColor8
 118:                             If IntVar6 = 1 Then
 119:                                 ClassVar7.Port.BackgroundImage = My.Resources._13LedOn()
 120:                             Else
 121:                                 ClassVar7.Port.BackgroundImage = My.Resources._13LedOff()
 122:                             End If
 123:                             ClassVar7.ConnectedIntVar1 = IntVar9
 124:                     End Select
 125:                 Else ' some comment 
 126:                     Select Case IntVar5
 127:                         Case -1 ' some comment 
 128:                             ClassVar7.PortStatus = SomeColor.SomethingColor9
 129:                             If IntVar6 = 1 Then
 130:                                 ClassVar7.Port.BackgroundImage = My.Resources._9LedOn()
 131:                             Else
 132:                                 ClassVar7.Port.BackgroundImage = My.Resources._9LedOff()
 133:                             End If
 134:                             ClassVar7.ConnectedIntVar1 = IntVar8
 135:                         Case IntVar5.Invalid
 136:                             ClassVar7.PortStatus = SomeColor.SomethingColor10
 137:                             If IntVar6 = 1 Then
 138:                                 ClassVar7.Port.BackgroundImage = My.Resources._10LedOn()
 139:                             Else
 140:                                 ClassVar7.Port.BackgroundImage = My.Resources._10LedOff()
 141:                             End If
 142:                             ClassVar7.ConnectedIntVar1 = IntVar9
 143:                         Case IntVar5.Valid
 144:                             ClassVar7.PortStatus = SomeColor.SomethingColor11
 145:                             If IntVar6 = 1 Then
 146:                                 ClassVar7.Port.BackgroundImage = My.Resources._11LedOn()
 147:                             Else
 148:                                 ClassVar7.Port.BackgroundImage = My.Resources._11LedOff()
 149:                             End If
 150:                             ClassVar7.ConnectedIntVar1 = IntVar9
 151:                         Case IntVar5.Expired
 152:                             ClassVar7.PortStatus = SomeColor.SomethingColor12
 153:                             If IntVar6 = 1 Then
 154:                                 ClassVar7.Port.BackgroundImage = My.Resources._12LedOn()
 155:                             Else
 156:                                 ClassVar7.Port.BackgroundImage = My.Resources._12LedOff()
 157:                             End If
 158:                             ClassVar7.ConnectedIntVar1 = IntVar9
 159:                     End Select
 160:                 End If
 161:             End If
 162:         End If
 163:     End If
 164:     ClassVar7.IntVar1 = IntVar1
 165: Catch
 166:     MsgBox(Err.Description, MsgBoxStyle.Critical, "Error in Coloring")
 167: End Try
 168: End Sub

 

How long was that! Lets start butchering it from top to bottom :)

Method Variables

 

I don’t know about you, but I think that 10 method variables are just too much!

 

Once you are sending that many variables to a method, you are either writing a method that does too much, or you are overlooking the possibility that some of these variables have similar uses and responsibilities, and should be grouped into a structure or a class. That object will be passed to methods instead of passing so many variables.

 

This is a very good example of coding with no prior design, every time there is a need to add some functionality the result is:

 

“Sure, lets add to method x another variable, some “if elses”, a “switch case” and that’s it! It works!”

 

Method Length

 

168 lines!!!!!!!!!!!!!!!!!!!!!!

 

How can you write a 168 lines long method?

 

After 20 lines you have no recollection of: what the method name is, what it is supposed to do, Why are you reading this method, and what got you in this method in the first place.

 

Debugging and understanding this code is just great this way…

 

No doubt, This code had to be broken to at least 10 methods and its a no brainer. Switch Case and If Else expressions makes breaking pieces of code into smaller chunks so much easier.

 

Try Catch Phrases

 

Try Catch phrases have a very important role in coding. Using them correctly is an art known to not many people. On the other hand, abusing them is easy as hell. Sure, Why not wrap the whole code in a big try catch and then if there’s an error, we will have no way of knowing where it happened. What to do? Just throw out a message box that says

 

“Error in Coloring”

 

That will help the user know what is wrong… NOT!

 

And of course, Lets make that message box Critical, so the user will get nervous… (line 166)

 

Throwing a new exception? Or letting it bubble up? Nope, why bother….

 

Comments

 

I must say, and I think I speak for 99.9% of the programmers:

 

I hate commenting my code!

 

But, I do it anyway cause it is so important, not only for the next guy who is going to handle my code, but also for me one month later, when I have to fix some bugs there, and won’t have a clue as to why I wrote this line or another.

 

Here, I have counted 10 comment lines on 168 code lines, and I can tell you that the comments written weren’t all that informative…

 

IF Else Expressions

 

I know this is more of a flavor question, but using 5 If Else expressions one inside the other? And some Switch Cases for desert? I don’t like it.

 

that is a recipe for a major Bug. Everyone knows that sometimes you have no choice, but I can tell you that here, there was a choice…

 

There is no doubt that writing this code will someday come back at you and bite you in the ass, your only hope is that you might not work there anymore… :)

 

Well that is all I had to say about this code. If you see anything else that I did not address, or disagree with something, please do comment.

 

Don’t forget to Grab our Feed so you get the next post in the Terrible coding examples series.

 

And as I said earlier:

 

Don’t try this at home…

 

Bye

Tags :

19 Responses to “Terrible Code Example – Methods From Hell”


  1. andhapp

    Said on October 21, 2008 :

    I have been very critical of some of your posts… but this one points out a very good flaw in the code now days. One should head to amazon and order Clean code + The Prgamatic Programmer…both these books stress on the craftmanship and how to become a better programmer.

  2. Amit

    Said on October 21, 2008 :

    I wish that the guy who wrote this code read that book… cause now I have the headache of dealing with this code… :)

  3. Xerxes

    Said on October 21, 2008 :

    Am i expecting too much by asking if this code was unit tested?

  4. Richard

    Said on October 21, 2008 :

    Wow.

    I started scrolling the page and the first words out of my mouth were, “Oh! Jimmy code!”

    I had a contract about 5 years ago where the entire program (about 15,000 lines of PHP) was written like this in one file, not counting the includes thrown in at random. Even with Vim set to set tabs at 1 space width each, it still took a while to decipher what was going on. The refactoring took longer. definitely earned my fee for that one.

    On the other hand, it’s given me that wonderful little phrase, “Jimmy code!”, so I guess that’s good.

  5. Erik

    Said on October 21, 2008 :

    While I agree with your post 100%, I think a more useful post would have included a demonstration on refactoring this into something more maintainable.

  6. Richard

    Said on October 21, 2008 :

    168 lines in a single method is nothing compared to the project (C#) I’m working on right now. I just found a 1046-line method – cylomatic complexity of 246.

    They wonder why no one wants to work on this project :)

  7. CurlyFro

    Said on October 22, 2008 :

    Refactor, refactor, refactor!

    amazon ‘Refactoring’ by Martin Fowler.

  8. 911fun

    Said on October 22, 2008 :

    This is a fake, isnt it?

    Come on there is no place called HELL on EARTH ;-)

  9. Eirik

    Said on October 22, 2008 :

    Actually there is:
    en.wikipedia.org/wiki/Hell,_Norway

  10. Schalk Versteeg

    Said on October 22, 2008 :

    This reminded me of code at my previous place of employment.
    Just this would’ve been one of the shorter methods.

    I reduced two one thousand+ line methods into one of more or less 168 line method and two or three normal 5-15 line methods. The only way I might’ve been able to reduce the complexity more was to redo this functionality a lot smarter. but the company did not have the two weeks to spare to redo it.

    There where at least one more function (2500 lines) with intermittent “untraceable” bugs. This function in turn called another function of 2000 lines code, twice.

    I marked this section and one other section as the most critical to change. I was the sole developer and had to add promised (by the sales person (the owner) to one of the clients) functionality at a breakneck speed. The functionality was already overdue before I joined the company.

  11. Amit

    Said on October 22, 2008 :

    I see I am not the only one in a mess like this… And it seem that some of you guys got it worse :)

    @Xerxes: I forgot to mention it in the article but when I asked about unit testing, I am not sure the guy new what I was talking about.

    @911fun: for the 0.001 chance you are not kidding :) YES!

  12. Rodrigo

    Said on October 22, 2008 :

    What a horrible code…but I think I can beat you I saw a code once that had if(1==2) all over the place to make some code not execute.
    I guess the developers never heard of commenting out the code .
    The excuse for that behavior was that the business rules changed all the time, so the code that was on that if might be useffull again… come on… comment the code and if is usefull again uncomment.

  13. Devy

    Said on October 22, 2008 :

    Before I read Schalk Versteeg’s comment I thought I had it bad at a previous place where a 1000 line method called another 1000 line method but sometimes would call itself recursively.

    That was painful to fix.

  14. beefarino

    Said on October 22, 2008 :

    Looks like I have y’all beat – one component of our product consists of about 5 code files, each is over 30,000 lines long. One has a *constructor* that is over 8,000 lines on its own.

    Unfortunately we have a culture of code ownership here, and the code “owner” won’t refactor it because it would, and I’m quoting here, “refactoring won’t add new features” and “might break existing functionality.”

    Several people have suggested unit testing the existing base as a way to tackle both refactoring and to prevent breakage, and have pointed out that an organized and concise codebase would allow for easier extension down the line. No go. “Sounds like a lot of effort, it works now, why change it?”

    * sigh *

  15. Chris Marisic

    Said on October 22, 2008 :

    I swear if that method was written in C# and you replaced Color with LoanAggregateAmount I would have been sure you stole that right from the source code of my last job.

  16. Amit

    Said on October 22, 2008 :

    I am so glad to see I am not alone…

    Those are horrific tales your are sharing.

    I just got an Idea, why dont you all send me samples of those code examples through the contact page or by email to admin at dev102.com. I promise to compile some posts with them (and credit you of course)

    The world needs to see!

  17. Mike

    Said on October 22, 2008 :

    Ha, one consultant that worked at my current job that is no long here, made a method signature that tool 135 parameters! Yeah, I am not kidding. In VS when writing a method call to that method, the intellisense filled up half of the IDE window!

  18. stratosg

    Said on October 25, 2008 :

    i once had a coworker that believed that 2000 queries for a simple page render was “just fine since the machine is fast and it’s done less than 10 seconds”! so yes i know what you mean :D

  19. Vance Goeden

    Said on October 4, 2010 :

    It’s such an significant subject and ignored by quite a few people, even experts. I thank you for your help getting people more aware about that subject.

Post a Comment