Terrible Code Example - Methods From Hell
.
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:
Public Sub GetSomethingColor(ByVal IntVar1 As Integer, ByVal BoolVar2 As Boolean, ByVal BoolVar3 As Boolean, ByVal BoolVar4 As Boolean, ByVal IntVar5 As Integer, ByVal IntVar6 As Integer, ByRef ClassVar7 As ClassType1, ByVal IntVar8 As Integer , ByVal IntVar9 As Integer, Optional ByVal BoolVar10 As Boolean = False)
Try
'' some comment
If BoolVar10 = True Then
ClassVar7.PortStatus = SomeColor.Mixed
If ClassVar7.ConnectedIntVar1_2 = -1 Then
ClassVar7.ConnectedIntVar1_2 = IntVar8
End If
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources.MixedLedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources.MixedLedOff()
End If
Else
If BoolVar2 AndAlso IntVar9 > 0 AndAlso (IntVar9 <> IntVar8) Then
ClassVar7.PortStatus = SomeColor.Mixed
ClassVar7.ConnectedIntVar1_2 = IntVar9
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources.MixedLedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources.MixedLedOff()
End If
Else
ClassVar7.ConnectedIntVar1_2 = -1
If BoolVar2 = False Then
If BoolVar3 Then
ClassVar7.PortStatus = SomeColor.PatchCordNo_DesignYes_DisconnectWONo
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._6LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._6LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar8
Else
Select Case IntVar5
Case -1 ' some comment
ClassVar7.PortStatus = SomeColor.SomethingColor
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._1LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._1LedOff()
End If
ClassVar7.ConnectedIntVar1 = 0
Case IntVar5.Invalid
ClassVar7.PortStatus = SomeColor.SomethingColor2
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._2LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._2LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar9
Case IntVar5.Valid
ClassVar7.PortStatus = SomeColor.SomethingColor3
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._3LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._3LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar9
Case IntVar5.Expired
ClassVar7.PortStatus = SomeColor.SomethingColor4
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._4LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._4LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar9
End Select
End If
Else
If BoolVar3 = False Then ' some comment
Select Case IntVar5
Case -1 ' some comment
ClassVar7.PortStatus = SomeColor.SomethingColor5
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._5LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._5LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar8
Case IntVar5.Invalid
If BoolVar4 = True Then
'ClassVar7.PortStatus = SomeColor.SomethingColor13
'If IntVar6 = True Then
' ClassVar7.Port.BackgroundImage = My.Resources._6LedOn
'Else
' ClassVar7.Port.BackgroundImage = My.Resources._6LedOff
'End If
'ClassVar7.ConnectedIntVar1 = IntVar9
Else ' Disconnect Work-Order in phase 1
ClassVar7.PortStatus = SomeColor.SomethingColor6
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._7LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._7LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar9
End If
Case IntVar5.Valid
' some comment
' some comment
ClassVar7.PortStatus = SomeColor.SomethingColor7
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._8LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._8LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar9
Case IntVar5.Expired
' some comment
' some comment
ClassVar7.PortStatus = SomeColor.SomethingColor8
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._13LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._13LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar9
End Select
Else ' some comment
Select Case IntVar5
Case -1 ' some comment
ClassVar7.PortStatus = SomeColor.SomethingColor9
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._9LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._9LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar8
Case IntVar5.Invalid
ClassVar7.PortStatus = SomeColor.SomethingColor10
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._10LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._10LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar9
Case IntVar5.Valid
ClassVar7.PortStatus = SomeColor.SomethingColor11
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._11LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._11LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar9
Case IntVar5.Expired
ClassVar7.PortStatus = SomeColor.SomethingColor12
If IntVar6 = 1 Then
ClassVar7.Port.BackgroundImage = My.Resources._12LedOn()
Else
ClassVar7.Port.BackgroundImage = My.Resources._12LedOff()
End If
ClassVar7.ConnectedIntVar1 = IntVar9
End Select
End If
End If
End If
End If
ClassVar7.IntVar1 = IntVar1
Catch
MsgBox(Err.Description, MsgBoxStyle.Critical, "Error in Coloring")
End Try
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.
- Login or register to post comments
- 1065 reads
- Printer-friendly version
(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)
