|
Yet Another VB Horror!
The following is an error-handling block from some old code I'm having to maintain (for my sins)..
errorPAT:
If retryCount < 1000 Then
ElseIf Err.Number = errOdbcCallFailed Then
If InStr(odbcErrorMess(Err), "insert duplicate") > 0 Then
retryCount = retryCount + 1
Resume tryAgain
ElseIf InStr(odbcErrorMess(Err), "unique constraint") > 0 And InStr(odbcErrorMess(Err), "violated") > 0 Then
retryCount = retryCount + 1
Resume tryAgain
ElseIf InStr(odbcErrorMess(Err), "Duplicate key value specified") > 0 Then
retryCount = retryCount + 1
Resume tryAgain
Else
message "Error " & Format(Err, "######") & " " & odbcErrorMess(Err), "P_ArchiveTag"
Exit Sub
End If
ElseIf Err.Number = errDuplicateValues Then
retryCount = retryCount + 1
Resume tryAgain
End If
message "Error " & Format(Err, "######") & " " & Error, "P_ArchiveTag"
End Sub
I think there's ample horrors for a short chunk of code...
|
|
|
|
|
It's 8am here. Remind me to never again visit this forum before breakfast. I can see this ruining my whole day (and no, I'm not going to sea, but that's another story.)
:vomit:
Peter
Software rusts. Simon Stephenson, ca 1994.
|
|
|
|
|
Yes, like the unique constraint in there, I feel "violated"
|
|
|
|
|
There a very aptly named method/function called 'odbcErrorMess', which neatly sums the whole code block you pasted.
Take-away: never shorten your method/function names 
'As programmers go, I'm fairly social. Which still means I'm a borderline sociopath by normal standards.' Jeff Atwood
|
|
|
|
|
Julien Villers wrote: Take-away: never shorten your method/function names
I don't know, in this case I'd say the variable name is spot on  .
Must be the only example in the code of a well named variable. I have such treasures as Function names:
Function H_WriteRecord1()
Function H_WriteRecord2()
Function H_WriteRecord3()
Note - those are the declarations as they appear. No return type given, all state passed in via global or module-level variables, and nothing to indicate what gets written, what the difference between the three variants is. I suspect "that nearly does what I want, I'll copy and paste it".
I also like the "batchHeader" boolean that sometimes outputs a header when true, otherwise when false. I still can't work out the logic governing when the two forms are used.
I suspect the originator may be a closet worshipper of The FSM[^]. I just wished he hadn't felt the need to spread noodly appendages through the code I'm now tasked with maintaining.
|
|
|
|
|
I was assigned to do some changes on built page and I found this.....
protected void btnEditClick(object sender, EventArgs e)
{
for (int i = 0; i < gvContactInfo.Rows.Count; i++)
{
var row = gvContactInfo.Rows[i].Cells[8];
ImageButton button = row.FindControl("modifyLink") as ImageButton;
if (button.GetHashCode() == SenderButton.GetHashCode())
{
ContactInfoView contactInfoView = row.FindControl("ctlContactInfoView") as ContactInfoView;
ctlNewContactInfo.ContactInfo = contactInfoView.ContactInfo;
}
}
}
|
|
|
|
|
Why? Why?
Why check an already boolean result?
Noticed this redundancy in many parts of an ill written app.
If myControl.Visible = True Then
Else
End If
I don't know why, it's just plain turn off to see this redundancy!
-
Just that something can be done, doesn't mean it should be done. Respect developers and their efforts!
Jk
|
|
|
|
|
Unfortunately this is the biggest issue I have about vb. It's too easy to be deep in thought and literally code what you are thinking and make the code more annoying to read. i.e. If control x visible property is true then do this and this and this. I usually pick it up after I have written it and clean it up afterwards but sometimes I've come across code a few months later and go whoops.
Hopefully the compiler is smart enough to fix the extra redundancy I've added so it doesn't effect performance.
|
|
|
|
|
I don
Anyway, to be really sure the control is visible you should obviously write
<pre>If (myControl.Visible = True) = True Then...
> so it doesn't effect performance
You mean "affect" (I hope)
|
|
|
|
|
And I ran this test once with "if mybool" and once with "if mybool = True".
No difference in speed. 1st one showed 33 seconds, 2nd one showed 32 seconds.
Module Module1
Sub Main()
Dim mytime As Date
Dim mybool As Boolean
Dim mydiff As TimeSpan
Dim j As Double
mytime = TimeOfDay()
While mytime = TimeOfDay()
End While
mybool = True
j = 0
For i = 1 To 100000
For j = 1 To 100000
If mybool = True Then
j = j + 1
End If
Next
Next
mydiff = TimeOfDay() - mytime
Console.WriteLine(mydiff)
Console.ReadKey()
End Sub
End Module
|
|
|
|
|
Had you taken a look at the disassembly, you would have discovered that the compiler generates exactly the same code in both cases. There is no real need for a test program.
"Dark the dark side is. Very dark..." - Yoda
---
"Shut up, Yoda, and just make yourself another toast." - Obi Wan Kenobi
|
|
|
|
|
Ummm...if you dissassemble something isn't there a program to disassemble? Ergo a test program?
I was simply providing an example that proves that this is not redundant code at all. The disassembly does confirm it though so thanks for that observation.
|
|
|
|
|
What I meant is, that when such a question arises, I simply write both lines in the application I'm working on and then look what I find in the disassembly. Why try to measure something that you can examine directly? As to the topic: I see that just as you do. It's not redundant and has no impact (in all languages I commonly use). A difference which makes no difference is no difference.
"Dark the dark side is. Very dark..." - Yoda
---
"Shut up, Yoda, and just make yourself another toast." - Obi Wan Kenobi
|
|
|
|
|
mdblack98 wrote:
Dim j As Double
...
j = 0
For j = 1 To 100000
...
j = j + 1
...
Next
Ouch!
* Declaring a control variable as a Double
* Redundant initialisation of a variable immediately before using it as a control variable
* Doing integer arithmetic on a Double (in the For and in the assignment statements)
* Changing the control variable inside the loop
* Writing a Hall Of Shame contender in response to a Hall Of Shame entry. Priceless!
|
|
|
|
|
|
I actually sometimes check is some condition is not fulfilled even when there is else branch.
This I do in cases when one case of if/else block is expected behavior and in other one i do simple logging/recovery or similar thing.
Specifically, I do this when one block is short (less than 5 lines), and put that block in front. In that case, code is more clean to my eye because else block is very near to if block. This is very helpful to me when i have several nested if/else structures.
Would that make sense to anyone but me?
|
|
|
|
|
makes sense to me to have the preferred condition as the first block of code
if not WingsFallenOff then
FlyNormally
else
Panic
End if
|
|
|
|
|
How about:
try{
if (wingsFallenOff) {
throw new OutOfLiftError("Ahhhhhhhhhhh!!!!!");
}
flyNormally();
} catch (OutOfLiftError ex) {
panic(ex);
}
To be honest, rather then checking the boolean value, a method call, say checkAirworthy(), could be used that, if the plane is in the air, will throw an exception rather than returning false. Thta means that you can later upgrade it to check for engines, air-con and lemon-scented hand sanitisers. The same method would then be used before pushing back.
Panic, Chaos, Destruction. My work here is done.
Drink. Get drunk. Fall over - P O'H
OK, I will win to day or my name isn't Ethel Crudacre! - DD Ethel Crudacre
I cannot live by bread alone. Bacon and ketchup are needed as well. - Trollslayer
Have a bit more patience with newbies. Of course some of them act dumb - they're often *students*, for heaven's sake - Terry Pratchett
|
|
|
|
|
It may seem structurally sound, but it's a mistake to use exception handling for control flow.
First, exceptions are exactly that. Cases which you do not expect should happen in normal workflow, exceptional situations which are caused by a failure in the system. They should not be used to control business logic. However, I am also tempted to use them in this way when I have more than 2 layers of abstraction between business logic decision, and handling of that decision.
Second, it's big hit to performance. If this is not client side code, or a single occurrence during request processing, this can be real PITA when you come to a stage you need to optimize performance.
|
|
|
|
|
Nikola Radosavljevic wrote: exceptions are [...] Cases which you do not expect
So, you expect the wings to fall of the plane? Unless they're upgrading the TU-154[^] to fly-by-wire, I don't think it is /normal/ behaviour. 
Panic, Chaos, Destruction. My work here is done.
Drink. Get drunk. Fall over - P O'H
OK, I will win to day or my name isn't Ethel Crudacre! - DD Ethel Crudacre
I cannot live by bread alone. Bacon and ketchup are needed as well. - Trollslayer
Have a bit more patience with newbies. Of course some of them act dumb - they're often *students*, for heaven's sake - Terry Pratchett
|
|
|
|
|
True. I didn't expect myself properly, but still. If user enters invalid PIN code, would you use exception to handle this situation? I'd argue you shouldn't.
|
|
|
|
|
The PIN case is different.
When a PIN is entered, it has to be authorised or declined. There is no exception in this.
If the software has used an invalid key for the encryption then THAT could be an exception, in general in is treated as decline.
Panic, Chaos, Destruction. My work here is done.
Drink. Get drunk. Fall over - P O'H
OK, I will win to day or my name isn't Ethel Crudacre! - DD Ethel Crudacre
I cannot live by bread alone. Bacon and ketchup are needed as well. - Trollslayer
Have a bit more patience with newbies. Of course some of them act dumb - they're often *students*, for heaven's sake - Terry Pratchett
|
|
|
|
|
PIN is more similar to usual business cases than wings falling off a plane 
Anyhow, maybe I could make up a rule that would say:
Do not use exception handling if result of failed operation is displayed to end user, and end user is expected to understand it (there can be exceptions to this )
|
|
|
|
|
Completely agree with you there!
If the wings fall of an airplane in flight then that would certainly raise an exception.
However, the function Nagy made is a function that checks if the wings have fallen off. If someone would check something like that they are probably expecting that it could somehow have happened. The person checking it would probably want a true or false
"Co-pilot to first pilot, the wings have fallen off!"
"First pilot here, no sweat, we'll just use another plane for the coming flight" 
It's an OO world.
|
|
|
|
|
Actually that does make sense to me.
I just don't usually have a lot of code in If blocks. If there is a lot of code I try to put it in different Methods so the If blocks always fit on my screen completely
Nested If's are a pain... If there are to many I once again make seperate Methods. If there are three, maybe four (absolute max) I try to keep the If's seperated by some comments that explain why there are so many if's and an empty line. Keeps code quite readable to my eyes
It's an OO world.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
)...