3
\$\begingroup\$

I have a form that consists of about 60 fields that all update when choosing a different customer from a drop down. I feel like my current way is not the best way as it takes about 30 seconds to update when changing customers. Currently I have:

Private Sub ClientSelection_AfterUpdate()

On Error GoTo errhandler

Dim dbTemp As DAO.Database
Dim rsTemp As DAO.Recordset
Dim strSQL As String


DoCmd.Hourglass True

sYear = Me.YearSelection.Value
    Me.CustomerID = Me.CustomerSelection.Column(0)
    CustomerID = Mid(Me.CustomerID.Value, 2, 36)

Set dbTemp = CurrentDb()
Set rsTemp = dbTemp.OpenRecordset("SELECT Field1 FROM Table where CustomerID = '" & CustomerID & "' and MonthYear = #01/01/" & sYear & "#")
Set rsTemp2 = dbTemp.OpenRecordset("SELECT Field1 FROM Table where CustomerID = '" & CustomerID & "' and MonthYear = #02/01/" & sYear & "#")


Me.JanCharges.Value = rsTemp("Charges")
Me.FebCharges.Value = rsTemp2("Charges")

rsTemp.Close
rsTemp2.Close
Set rsTemp = Nothing
Set rsTemp2 = Nothing
Set dbTemp = Nothing

Exit_ClientSelection_AfterUpdate:
Exit Sub


errhandler:
 MsgBox "Error " & Err.Number & ": " & Err.Description & " in " & _
   VBE.ActiveCodePane.CodeModule, vbOKOnly, "Error"
    Resume Exit_ClientSelection_AfterUpdate

End Sub

Both of these commands basically happen 60 times (5 categories consisting of 12 fields(months)). Then even further down I close all the recordsets and set them to Nothing. All of this works just takes a little longer than I want. What is a more efficient to accomplish this?

\$\endgroup\$
  • 1
    \$\begingroup\$ This post has been auto-flagged for having more than 20 comments. I have reviewed those comments and categorized them as either obsolete, chatty, not-constructive, or other, and have purged the lot of them. Please keep discussions and debates out of the comments, and perhaps in a chat room. All comments should be polite at all times. \$\endgroup\$ – rolfl May 21 '15 at 14:11
  • \$\begingroup\$ Multiple people have suggested ADO over DAO. I agree with this generalization for the majority of scenarios, and especially for someone just getting into programming. ADO is newer and has most of the methods that available to DAO plus more. There are however a few methods that DAO offers easier access to and a few that are not available in ADO at all, so DAO does still have it's place. And there is nothing wrong with using both in one program (doesn't have to be an either-or scenario) Google ADO vs DAO and sit down and have a read when you get time. \$\endgroup\$ – CBRF23 May 21 '15 at 20:28
4
\$\begingroup\$

I'm going to leave the specifics to more proficient reviewers, as I'm not all that familiar with DAO and Access VBA.

If possible, switch to SQL Server ADODB.


DEADC0DE

Unless I'm missing something, you're inlining the SQL statements and Dim strSQL As String is an unused declaration.

These can be hard to spot without an add-in though. MZ-Tools has a review source code feature that can find unused variables and procedures; Rubberduck has a code inspections feature that can find that too, and a number of other code issues (disclaimer: I own and maintain , see tag info).


UX Issue?

DoCmd.Hourglass True

You're turning the hourglass on, but you never turn it back off. I would expect this:

Exit_ClientSelection_AfterUpdate:
    DoCmd.Hourglass False        
    Exit Sub

The error handler is problematic:

MsgBox "Error " & Err.Number & ": " & Err.Description & " in " & _
    VBE.ActiveCodePane.CodeModule, vbOKOnly, "Error"

This cannot reliably be reporting the source of the error, unless there's only 1 module in your VBA project. Proof:

Module1

Public Sub Boom()
    MsgBox VBE.ActiveCodePane.CodeModule
End Sub

Module2

Public Sub DoSomething()
    Module1.Boom
End Sub

If Module2 is the opened module when you run DoSomething, the message will say Module2, not Module1 where the "error" would have happened.

And it requires adding a reference to the VBIDE extensibility library in your VBAProject, which requires special security handling. Not ideal.

\$\endgroup\$
  • 2
    \$\begingroup\$ Actually, now that I look at it. It's possible that if the VBE isn't open, that would blow up as well. \$\endgroup\$ – RubberDuck May 19 '15 at 20:42
  • \$\begingroup\$ Thanks for the input and yes the Hourglass is turn off later. I think that's actually just some rement of me fooling around. Regarding the err handling I'm taking advice from you and RubberDuck and re-examing it's usefullness. \$\endgroup\$ – mrbungle May 19 '15 at 20:48
  • \$\begingroup\$ @mrbungle see, that's what I meant earlier when I said context code matters: you never know what issues can be found in a code review, in code that isn't immediately related to your primary concern ;-) \$\endgroup\$ – Mathieu Guindon May 19 '15 at 21:15
  • \$\begingroup\$ Yes, I appreciate the feedback, first time Code Review. Thanks again for your input and my form now loads in about 3 seconds and still working on my proper where clause. \$\endgroup\$ – mrbungle May 19 '15 at 21:20
3
\$\begingroup\$

Disclaimer: I don't know much about DAO. I typically use ADODB.

The expensive part of this code is opening and closing the recordset repeatedly. So, instead of opening and closing it every time this is triggered, open it once when the form is first loaded.

Private dbTemp As DAO.Something 'I don't know the right type
Private rsTemp As DAO.Recordset
Private rsTemp2 As DAO.Recordset

Private Sub Form_Load()
    ' code to open the entire recordset without a filter
End Sub

Private Sub Form_Unload()
    ' code to close and dispose of the recordsets
End Sub

Of course, now you don't have the specific record you're looking for when the event fires. You can use the Find and FindNext methods to filter the entire recordset down to what you need in the event procedure.

https://msdn.microsoft.com/EN-US/library/office/ff194787.aspx


Of course, this is if I can't talk you into switching to ADODB for your purposes here. It supports parameterized queries, which (in theory) should execute a bit faster. This is because, to the best of my knowledge, DAO requests all of the data and then filters it down in memory on the local machine. An ADODB parameterized query allows for the plan to be cached and only returns the results of the statement.

Dim rst As ADODB.Recordset
Dim cmd as ADODB.Command

Const sql as String = _
    "SELECT Field1 " & _
    "FROM Table " & _
    "WHERE CustomerID = ? and MonthYear = ?"

With cmd 
     .CommandType = adCmdText
     .CommandText = sql

     .Parameters.Append .CreateParameter(...)
     .Parameters.Append .CreateParameter(...)

     Set rst = .Execute
End With

' some code to set january's value

cmd.Parameters(1).Value = feburaryDate

Set rst = cmd.Execute

' some code to set february's value

Note how we simply pass the query a new value, instead of duplicating all of the query text? This is just one of ADODB's benefits.


One other, rather minor note.

I bet this code is all through your code base.

 MsgBox "Error " & Err.Number & ": " & Err.Description & " in " & _
   VBE.ActiveCodePane.CodeModule, vbOKOnly, "Error"

It's well worth extracting the logic into a dedicated module or class to standardize your error handling UI.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the detailed answer and I'll look at the recommendation with the err handler. Your answer did get me to look at the Move and I think that might be useful for me and without too much editing of current code. I edited my original question with update including my idea of using the 'Move` method. \$\endgroup\$ – mrbungle May 19 '15 at 20:47
  • \$\begingroup\$ Thanks for your input as well, still working on the getting my where clause proper but what I have is loading the form now in 3 secs. You definitley got me in the right direction. \$\endgroup\$ – mrbungle May 19 '15 at 21:22
  • \$\begingroup\$ Try something like Where field Is Between date1 AND date2. =;)- \$\endgroup\$ – RubberDuck May 19 '15 at 23:04
  • \$\begingroup\$ I can't do that. Each date field is set up as the first of each month for each customer with their KPIs and thresholds. Mgt sets the KPIs for each category and month and then the SSRS report runs with actual numbers, compares to KPIs , makes a comparison and color codes appropriately. customer is up or down. \$\endgroup\$ – mrbungle May 20 '15 at 1:03
2
\$\begingroup\$

I wonder about the table design. It seems the CustomerID is a string whereas generally it is best to use long integers. Also has the CustomerID field in the table been indexed or set to primary key? Using a SQL where clause is much faster for filtering records that using findnext on a recordset, so do as much as you can in SQL. I note that the SQL statement is hard written into the openrecordset statement. I prefer the following Set rst = dbs.OpenRecordset(SQL, dbopensnapshot). SQL is a string variable and I can build the SQL statement up on the fly using VBA. For example in my application, the field list is fixed and only the where clause changes between cycles so I can write FieldList = "SELECT Field1, Field2, .. FROM Table " and then a separate Where Clause ", eg. "WHERE CustomerID = " & CustomerID. Then I create the SQL statement as follows SQL = FieldList & WhereClause. There are some advantages to doing things this way including being able to generate SQL statements that can be copied into empty queries and run to check they are working properly. Separating the field list from the where clause should let you change your code so that you can grab all needed fields from a table in one go. I tend to create a user defined type with all the fields that the form needs and populate the fields of the user defined types (~struct) from the database. This means that I can validate the data I am getting back from the database before anything is shown on the form. What if there is a null value where an integer is expected. By putting all the data into a struct first, I can confirm the correct data types and can run formatting code. Loading form fields from a struct is easy. Me.charges = s.charges. If I was a bit more modern, I would make a class module with properties for all the form fields, then I could use letters and getters to validate the data as it comes in.

The dbopensnapshot tells ms-access not to continuously check whether other users have edited the record so there is much less code running in the background. Is the MonthYear field in the database indexed.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.