Over a million developers have joined DZone.
{{announcement.body}}
{{announcement.title}}

Terrible Code Example - Methods From Hell

DZone's Guide to

Terrible Code Example - Methods From Hell

·
Free Resource
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:

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.

Topics:

Published at DZone with permission of Amit Raz. See the original article here.

Opinions expressed by DZone contributors are their own.

THE DZONE NEWSLETTER

Dev Resources & Solutions Straight to Your Inbox

Thanks for subscribing!

Awesome! Check your inbox to verify your email so you can start receiving the latest in tech news and resources.

X

{{ parent.title || parent.header.title}}

{{ parent.tldr }}

{{ parent.urlSource.name }}