YAVH - Yet another VBA horror
-
Well another from the goldmine of coding horrors that is VBA... Here, two tables have similar structures, and need to be processed similarly (don't ask)... Naturally, I paraphrase to protect the innocent.
Sub SomeSub()
Dim rs As Recordset
Dim tblname As String
Dim i As IntegerOn Error GoTo Err\_Handler tblname = "Table1" For i = 0 To 1 Set rs = db.OpenRecordset("SELECT \* FROM " & tblname & " WHERE ... ") ' Code to process records here ' Including...NextTable:
tblname = "Table2" Next i ' Remainder of code Exit SubErr_Handler:
' Code to log error Resume NextTable ' Note that there's no real error-checking, just logging.End Sub
(I should mention that code to process records here expands to a couple of hundred lines) Why, oh why, not define another procedure and call it...
Sub SomeSub()
' Replacement for above
ProcessRecords "Table1"
ProcessRecords "Table2"
End SubSub ProcessRecords(tableName AS String)
' Code to process records here
' When errors occur, can simply return (ideally after some real error-checking)
End SubMakes you want to hurl.
-
Well another from the goldmine of coding horrors that is VBA... Here, two tables have similar structures, and need to be processed similarly (don't ask)... Naturally, I paraphrase to protect the innocent.
Sub SomeSub()
Dim rs As Recordset
Dim tblname As String
Dim i As IntegerOn Error GoTo Err\_Handler tblname = "Table1" For i = 0 To 1 Set rs = db.OpenRecordset("SELECT \* FROM " & tblname & " WHERE ... ") ' Code to process records here ' Including...NextTable:
tblname = "Table2" Next i ' Remainder of code Exit SubErr_Handler:
' Code to log error Resume NextTable ' Note that there's no real error-checking, just logging.End Sub
(I should mention that code to process records here expands to a couple of hundred lines) Why, oh why, not define another procedure and call it...
Sub SomeSub()
' Replacement for above
ProcessRecords "Table1"
ProcessRecords "Table2"
End SubSub ProcessRecords(tableName AS String)
' Code to process records here
' When errors occur, can simply return (ideally after some real error-checking)
End SubMakes you want to hurl.
Oh please. Your way might be slightly more elegant, but it is neither more robust, nor faster. You haven't fixed anything, nor really changed anything.
The practical reason for freedom is that freedom seems to be the only condition under which any kind of substantial moral fiber can be developed — we have tried law, compulsion and authoritarianism of various kinds, and the result is nothing to be proud of. ~ Albert Jay Nock
-
Oh please. Your way might be slightly more elegant, but it is neither more robust, nor faster. You haven't fixed anything, nor really changed anything.
The practical reason for freedom is that freedom seems to be the only condition under which any kind of substantial moral fiber can be developed — we have tried law, compulsion and authoritarianism of various kinds, and the result is nothing to be proud of. ~ Albert Jay Nock
-
Oh please. Your way might be slightly more elegant, but it is neither more robust, nor faster. You haven't fixed anything, nor really changed anything.
The practical reason for freedom is that freedom seems to be the only condition under which any kind of substantial moral fiber can be developed — we have tried law, compulsion and authoritarianism of various kinds, and the result is nothing to be proud of. ~ Albert Jay Nock
I'd view it as a huge improvement. Take a look at how you'd be hopping around the code on error conditions with what amounts to goto statements (resume NextTable). The original posters change would be the first thing I'd do as a refactor.
-
I'd view it as a huge improvement. Take a look at how you'd be hopping around the code on error conditions with what amounts to goto statements (resume NextTable). The original posters change would be the first thing I'd do as a refactor.
Jeremy Hutchinson wrote:
Take a look at how you'd be hopping around the code on error conditions with what amounts to goto statements (resume NextTable).
VB6 error handling is icky regardless. Depending upon local variable usage, that 'for loop' style may be reasonable. Not great, but given the lack of nice array initializers in VB6 I wouldn't call it horrible. The alternative would be to have a "select case" statement at the start of the loop to set the table name.
-
Well another from the goldmine of coding horrors that is VBA... Here, two tables have similar structures, and need to be processed similarly (don't ask)... Naturally, I paraphrase to protect the innocent.
Sub SomeSub()
Dim rs As Recordset
Dim tblname As String
Dim i As IntegerOn Error GoTo Err\_Handler tblname = "Table1" For i = 0 To 1 Set rs = db.OpenRecordset("SELECT \* FROM " & tblname & " WHERE ... ") ' Code to process records here ' Including...NextTable:
tblname = "Table2" Next i ' Remainder of code Exit SubErr_Handler:
' Code to log error Resume NextTable ' Note that there's no real error-checking, just logging.End Sub
(I should mention that code to process records here expands to a couple of hundred lines) Why, oh why, not define another procedure and call it...
Sub SomeSub()
' Replacement for above
ProcessRecords "Table1"
ProcessRecords "Table2"
End SubSub ProcessRecords(tableName AS String)
' Code to process records here
' When errors occur, can simply return (ideally after some real error-checking)
End SubMakes you want to hurl.
Eeeewww. Yeah I would definitely change that code. But if you do change it, make sure you change it to use query parameters instead of building SQL from scratch. It's not so bad when the tablename is hard-coded into the method itself but if passed in from outside, it's a huge security hole. (Some future slave might decide to call the method with user-provided input, and it won't be properly escaped) It's so hard to find good slaves today...
Before .NET 4.0, object Universe = NULL;
-
Well another from the goldmine of coding horrors that is VBA... Here, two tables have similar structures, and need to be processed similarly (don't ask)... Naturally, I paraphrase to protect the innocent.
Sub SomeSub()
Dim rs As Recordset
Dim tblname As String
Dim i As IntegerOn Error GoTo Err\_Handler tblname = "Table1" For i = 0 To 1 Set rs = db.OpenRecordset("SELECT \* FROM " & tblname & " WHERE ... ") ' Code to process records here ' Including...NextTable:
tblname = "Table2" Next i ' Remainder of code Exit SubErr_Handler:
' Code to log error Resume NextTable ' Note that there's no real error-checking, just logging.End Sub
(I should mention that code to process records here expands to a couple of hundred lines) Why, oh why, not define another procedure and call it...
Sub SomeSub()
' Replacement for above
ProcessRecords "Table1"
ProcessRecords "Table2"
End SubSub ProcessRecords(tableName AS String)
' Code to process records here
' When errors occur, can simply return (ideally after some real error-checking)
End SubMakes you want to hurl.
-
Oh please. Your way might be slightly more elegant, but it is neither more robust, nor faster. You haven't fixed anything, nor really changed anything.
The practical reason for freedom is that freedom seems to be the only condition under which any kind of substantial moral fiber can be developed — we have tried law, compulsion and authoritarianism of various kinds, and the result is nothing to be proud of. ~ Albert Jay Nock
Except that I have to maintain and enhance this code. Refactoring code for elegance is not wasted time - every future maintainer will appreciate your effort.