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
Write for Dev102!
We pay for user submitted tutorials and articles that we publish. Anyone can send in a contribution
Learn More
andhapp Said on Oct 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.
Amit Said on Oct 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…
Xerxes Said on Oct 21, 2008 :
Am i expecting too much by asking if this code was unit tested?
Richard Said on Oct 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.
Erik Said on Oct 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.
Richard Said on Oct 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
CurlyFro Said on Oct 22, 2008 :
Refactor, refactor, refactor!
amazon ‘Refactoring’ by Martin Fowler.
911fun Said on Oct 22, 2008 :
This is a fake, isnt it?
Come on there is no place called HELL on EARTH
Eirik Said on Oct 22, 2008 :
Actually there is:
en.wikipedia.org/wiki/Hell,_Norway
Schalk Versteeg Said on Oct 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.
Amit Said on Oct 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!
Rodrigo Said on Oct 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.
Devy Said on Oct 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.
beefarino Said on Oct 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 *
Chris Marisic Said on Oct 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.
Amit Said on Oct 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!
Mike Said on Oct 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!
stratosg Said on Oct 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