I have managed to write some pretty terrible code in VBA and I know there has to be a better way. I have made all of these variables to store some data which I eventually reference in the application. I’m fairly certain I need to loop over the recordset or fields and store the variables that way but I can’t figure out how to do it in a way that actually simplifies this code. It’s torturing me because it seems like a painfully simple concept.
The columns of the table that it is gathering data from looks similar to this:
[LANGUAGE_ID]-[NEXT]-[PREVIOUS]-[RESUME] — and so on
sqlstmt = "SELECT *" & _ " FROM cf_Language" & _ " WHERE LANGUAGE_ID = " & parm_language & ";" Set rst_t = dbs.OpenRecordset(sqlstmt, dbOpenSnapshot) If (parm_language <> 1) Then parm_surveyname = UnicodeToAscii(parm_surveyname, True) aNext = UnicodeToAscii(rst_t![Next], True) aPrevious = UnicodeToAscii(rst_t![previous], True) aResume = UnicodeToAscii(rst_t![resume], True) aFinish = UnicodeToAscii(rst_t![finish], True) aSave = UnicodeToAscii(rst_t![Save], True) aLogIn = UnicodeToAscii(rst_t![log_in], True) aComplete = UnicodeToAscii(rst_t![complete], True) aHeader = UnicodeToAscii(rst_t![header], True) aThanks = UnicodeToAscii(rst_t![thanks], True) aSavedMsg = UnicodeToAscii(rst_t![saved_msg], True) aBlankCode = UnicodeToAscii(rst_t![blank_code], True) aInvalidCode = UnicodeToAscii(rst_t![invalid_code], True) aTimeOut = UnicodeToAscii(rst_t![timeout], True) aClosed = UnicodeToAscii(rst_t![closed], True) aAccessCode = UnicodeToAscii(rst_t![access_code], True) aConFirstTime = UnicodeToAscii(rst_t![con_first_time], True) aAnonFirstTime = UnicodeToAscii(rst_t![anon_first_time], True) aAnonResume = UnicodeToAscii(rst_t![anon_resume], True) Else aNext = rst_t![Next] aPrevious = rst_t![previous] aResume = rst_t![resume] aFinish = rst_t![finish] aSave = rst_t![Save] aLogIn = rst_t![log_in] aComplete = rst_t![complete] aHeader = rst_t![header] aThanks = rst_t![thanks] aSavedMsg = rst_t![saved_msg] aBlankCode = rst_t![blank_code] aInvalidCode = rst_t![invalid_code] aTimeOut = rst_t![timeout] aClosed = rst_t![closed] aAccessCode = rst_t![access_code] aConFirstTime = rst_t![con_first_time] aAnonFirstTime = rst_t![anon_first_time] aAnonResume = rst_t![anon_resume] End If
- I would not use local variables to hold recordset values, because you’d need one for every field of every returned row… which is just wrong.
- As already pointed out, good variable names are readable variable names (which can be pronounced!).
- You’re not showing it, but make sure you close that connection once you’re done.
Here’s an [perhaps less appealing] alternative to
Dictionary. Since I learned about C# and started using Entity Framework, I like wrapping each record in an object – given a class called
MyEntity (which does nothing more than exposing getters and setters for all properties):
Private Type ThisEntity SomeProperty As String AnotherProperty As String End Type Private this As ThisEntity Option Explicit Public Property Get SomeProperty() As String SomeProperty = this.SomeProperty End Property Public Property Let SomeProperty(value As String) this.SomeProperty = value End Property Public Property Get AnotherProperty() As String AnotherProperty = this.AnotherProperty End Property Public Property Let AnotherProperty(value As String) this.AnotherProperty = value End Property
You define this object only once, and then you instantiate it as many times as you have records in your recordset. With a dictionary you get easier loading (each “entity” is a dictionary where you identify each field by their names to retrieve the corresponding value), but with this object you completely avoid referring to your properties using strings – the only hard-coded strings involved are the field names.
Reading the recordset
All you need to do is exactly as you said, loop through the recordset:
Dim entity As MyEntity Dim entities As New Collection While Not rs.EOF And Not rs.BOF 'BOF check if result has 0 rows Set entity = New MyEntity With entity .SomeProperty = rs.Fields("SomeField").Value 'or rs!SomeField .AnotherProperty = rs.Fields("AnotherField").Value 'or rs!AnotherField End With entities.Add entity ', entity.Id /PK property if you want to use a key here rs.MoveNext 'don't forget this, or your loop will be endless! Wend rs.Close Set rs = Nothing 'at this point you have your entire recordset's content in the "entities" collection
About rs!FieldName notation (little “Gotcha!”)
For what it’s worth, I’ve recently fixed a bug in our codebase that was caused by the
rs!FieldName notation in a specific circumstance: rs!FieldName was being added to a collection, and the recordset was left open. When I found that out, I added
rs.Close and all of a sudden the collection, although non-empty, could no longer be read. This is because adding
rs!FieldName to the collection (or, if you take the dictionary road) was actually adding a reference to the
Field object, and the original programmer was left with the impression it worked fine because he could retrieve any entry in the collection and see the field’s value – when he was actually retrieving a
Field object and through the default property was fetching the value from the recordset: that stopped working when I added code to close the recordset after all records were read.
That bug was fixed by explicitly adding rs.Fields(“FieldName”).Value to the collection instead.
From the help : “The default collection of a Recordset object is the Fields collection, and the default property of a Field object is the Value property. Use these defaults to simplify your code.”
I would either :
- Loop over the fields and create a dictionary with name/value pairs
- Make sure that rst_t is of type Snapshot and just keep using rst_t
Other than that:
- Hungarian notation, just don’t do it
- Disemvoweling ( sqlstmt ? ), just don’t do it either
- Seems like you could simply always call UnicodeToAscii and cut half of the code
On dictionaries in VBa:
On Hungarian notation ( and why it is bad ):
I could not find a good article on disemvoweling ( remove all vowels from the name and then some like ‘statement’ -> ‘sttmnt’ -> ‘stmt’ ), I was sure Coding Horror had one. Basically it just makes your code harder to read for other people, it is one more barrier to (mis)understand code.