1
\$\begingroup\$

I am becoming more critical of the code I write. This is leading me to questions like "Could I write my code to be more efficient?" Of course the answer to that is a resounding YES. So, I posit the question to those who know much more than I do. What could I do to make this more efficient?

The purpose of this code is to take data from an Access form and save it into two (or three if a condition is met) tables.

Note: I originally asked a form of this question on Stack Overflow.

Private Function SaveSerials()
Dim rs As DAO.Recordset
Dim rs2 As DAO.Recordset
Dim rs3 As DAO.Recordset
Dim qry As QueryDef
Dim i as Long
If Not BasicInclude.DebugMode Then On Error GoTo Error_Handler Else On Error GoTo 0
    Dim out As String: out = Validate(Me.Name, msclblstyle)
    If out = "" Then
        Set qry = dbLocal.QueryDefs("qryDeletefromLabelTemp")
        qry.Parameters(0) = "%1" & Me.txtOperator.Value & "%"
        qry.Execute
        Set rs = dbLocal.TableDefs("dbo_Part_Serial_Number").OpenRecordset(dbOpenDynaset, dbSeeChanges)
        Set rs2 = dbLocal.TableDefs("dbo_Label_Trace_Temp").OpenRecordset(dbOpenDynaset, dbSeeChanges)
        For i = 1 To Me.lstData.ListCount - 1
            rs.AddNew
            rs2.AddNew
            rs.Fields("Part_No").Value = Me.txtPart_No.Value
            rs2.Fields("PartNumber").Value = Me.txtPart_No.Value
            rs.Fields("Label_Date").Value = Me.txtLabel_Date.Value
            rs.Fields("Serial_No").Value = Me.lstData.Column(0,i)
            rs2.Fields("PartSerialNumber").Value = Me.lstData.Column(0,i)
            rs.Fields("Operator").Value = Me.txtOperator.Value
            rs2.Fields("Operator").Value = "%1" & Me.txtOperator.Value & "%"
            rs.Fields("Work_Order").Value = Me.cboWork_Order.Value
            rs2.Fields("WorkOrder").Value = Me.cboWork_Order.Value
            rs.Fields("Date_Code").Value = Me.txtDate_Code.Value
            rs2.Fields("Date_Code").Value = Me.txtDate_Code.Value
            rs.Fields("Notes").Value = Me.txtNotes.Value
            rs.Fields("Flex_Circuit_Date_Code").Value = Me.txtFC_Date_Code.Value
            rs.Fields("Flex_Circuit_Int_Date_Code").Value = Me.txtFCI_Date_Code.Value
            rs.Fields("Substrate_Date_Code").Value = Me.txtSubstrate_Date_Code.Value
            rs.Fields("SMF_Date_Code").Value = Me.txtSMF_Date_Code.Value
            rs.Fields("r").Value = Me.chkR.Value
            rs.Update
            rs2.Update
        Next
        If Me.txtPart_No Like "PS*" Then
            Set qry = dbLocal.QueryDefs("qryClear_tbl_BarcodeList")
            qry.Parameters(0) = Environ(GetEnvironString(esComputerName))
            qry.Execute
            Set rs3 = dbLocal.TableDefs("tbl_BarcodeList").OpenRecordset(dbOpenDynaset, dbSeeChanges)
            For i = 1 To Me.lstData.ListCount - 1
                rs3.AddNew
                rs3.Fields("Part_No").Value = Me.txtPart_No.Value
                rs3.Fields("Host").Value = Environ(GetEnvironString(esComputerName))
                rs3.Fields("Serial_No").Value = Me.lstData.Column(0,i)
                rs3.Update
            Next
            DoCmd.OpenReport "rpt_BarcodeList", acViewPreview
        End If
    SaveSerials = 0
    Else
        MsgBox "Please fill out the following fields: " & vbCrLf & out, vbCritical, "Error"
        SaveSerials = 1
    End If
Error_Exit:
set rs = Nothing
set rs2 = Nothing
set rs3 = Nothing
Exit Function
Error_Handler:
    MsgBox "The following error has occured" & vbCrLf & vbCrLf & _
           "Error Number: " & Err.Number & vbCrLf & _
           "Error Source: SaveSerials" & vbCrLf & _
           "Error Description: " & Err.Description _
           , vbOKOnly + vbCritical, "An Error has Occured!"
           SaveSerials = -1
Resume Error_Exit
End Function
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CR! Please describe the purpose of your code (the post title should roughly summarize that), give as much context to reviewers as possible - see how to get the best value out of Code Review for all the info, or Simon's guide for posting a good question for a quick read. \$\endgroup\$ – Mathieu Guindon Nov 9 '17 at 20:54
  • \$\begingroup\$ @Mat'sMug in line 7 does Else On Error GoTo 0 have any effect? \$\endgroup\$ – user109261 Nov 10 '17 at 16:00
  • \$\begingroup\$ @ThomasInzina seems so.. Arguably looks like a case for precompiler directives though. \$\endgroup\$ – Mathieu Guindon Nov 10 '17 at 16:03
  • \$\begingroup\$ @Mat'sMug I was thinking the same thing. I recently seem that pattern#Const DebugMode in the public module with #If DebugMode .... #Else ... \$\endgroup\$ – user109261 Nov 10 '17 at 16:07
  • \$\begingroup\$ the debugmode variable is set when the switchboard loads based on the user account. I also sometimes set that to true via the immediate window when i am not on my own computer so i can see where the fail happens. \$\endgroup\$ – KySoto Nov 10 '17 at 16:10
1
\$\begingroup\$

On Error GoTo 0 is used in conjunction with On Error Resume Next to reset any errors that may occur between the two statements.

For this reason, On Error GoTo 0 in the ↓ line below ↓ should be changed to On Error Resume Next.

If Not BasicInclude.DebugMode Then On Error GoTo Error_Handler Else On Error GoTo 0

I would rewrite the Validation statement removing the else clause. This will make the code easier to read.

If out <> "" Then
    MsgBox "Please fill out the following fields: " & vbCrLf & out, vbCritical, "Error"
    SaveSerials = 1
    Goto Error_Exit:
End If

Setting objects to nothing is rarely necessary with today's garbage collectors.

set rs = Nothing
set rs2 = Nothing
set rs3 = Nothing

Recordset should be closed.

rs.Close
rs2.Close
rs3.Close

Grouping your recordset field will improve the readability of the code, make it easier to spot missing fields, and just makes sense.

For i = 1 To Me.lstData.ListCount - 1
    'Add record to dbo_Part_Serial_Number
    rs.AddNew
    rs.Fields("Part_No").Value = Me.txtPart_No.Value
    rs.Fields("Label_Date").Value = Me.txtDate_Created.Value
    rs.Fields("Serial_No").Value = Me.lstData.Column(i)
    rs.Fields("Operator").Value = Me.txtOperator.Value
    rs.Fields("Work_Order").Value = Me.cboWork_Order.Value
    rs.Fields("Date_Code").Value = Me.txtDate_Code.Value
    rs.Fields("Notes").Value = Me.txtNotes.Value
    rs.Fields("Flex_Circuit_Date_Code").Value = Me.txtFC_Date_Code.Value
    rs.Fields("Flex_Circuit_Int_Date_Code").Value = Me.txtFCI_Date_Code.Value
    rs.Fields("Substrate_Date_Code").Value = Me.txtSubstrate_Date_Code.Value
    rs.Fields("SMF_Date_Code").Value = Me.txtSMF_Date_Code.Value
    rs.Fields("r").Value Me.chkR.Value
    rs.Update

    'Add record to dbo_Label_Trace_Temp
    rs2.AddNew
    rs2.Fields("PartNumber").Value = Me.txtPart_No.Value
    rs2.Fields("PartSerialNumber").Value = Me.lstData.Column(i)
    rs2.Fields("Operator").Value = "%1" & Me.txtOperator.Value & "%"
    rs2.Fields("WorkOrder").Value = Me.cboWork_Order.Value
    rs2.Fields("Date_Code").Value = Me.txtDate_Code.Value
    rs2.Update
Next

You may consider extracting code related to the three recordset into their own subroutines but it is not necessary.

It is hard to discern the meaning of the return value of SaveSerials (e.g. -1, 0, 1) without studying the code. Consider using an Enum.

enter image description here

\$\endgroup\$
  • \$\begingroup\$ I use the on error goto 0 for troubleshooting and development. For production I want the user to just see the error message and let me know. The output is only being used to determine if the form is going to be closed or not. i didnt really need to set the error path to -1, its just habit. \$\endgroup\$ – KySoto Nov 10 '17 at 15:55
  • \$\begingroup\$ In that case Function SaveSerials() As Boolean makes more sense. \$\endgroup\$ – user109261 Nov 10 '17 at 15:59

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.