FireGeek822 said:
Dirk,
Thanks for the information - this certainly makes sense and helps. I
will try to make the necessary adjustments. If you have other
suggestions for the other code, I am more than open to your
recommendations so I can improve.
Since you ask ...
Here's your original code, with comments interspersed:
Private Sub cmdContinue_Click()
On Error GoTo ErrorHandler
Me.cboDivisions.SetFocus
IssueDiv = Me.cboDivisions.Text
There's no need to set the focus to the combo box, to read its value.
Reading the Text property *does* require that, but for most purposes
what you want is the Value property, which is available at all times,
whether the control has the focus or not. It's also the default
property of all data-bound controls, so you don't have to explicitly
refer to the .Value property, though you can if you want.
x = Me.cboDivisions.ListIndex
I dont know why you're making the above assignment. I don't see where
you use "x" anywhere else, nor why you need the ListIndex for anything.
Also, I don't see a Dim statement for "x", which makes me wonder whether
you don't have Option Explicit specified for your module(s). I strongly
recommend that you specify Option Explicit for all modules -- you can
set this up by setting the "Require variable declaration" option in the
VB Editor's Tools->Options... dialog. (Modules you've already created
will have to have the Option Explicit line added by hand. If you don't
use this option, you'll continually run into errors where your
inadvertent misspelling of a variable name causes Access to think it's a
new variable. This sort of bug is hard to track down.
If IssueDiv = "" Then
MsgBox "Please select a Division" & vbCrLf & "from the drop
down box.", vbOKOnly, "Type Selection Error"
Form_frmIssuesDivisionSelection.cboDivisions.SetFocus
Don't use this form of class module reference; that is, the
"Form_<module name>" form. It works, but (a) in this case a simple ...
Me.cboDivisions.SetFocus
.... would be more efficient; (b) in other cases, if the form you
reference isn't open at the moment, that form of reference will open it
in a hidden window, which isn't usually what you would want; and (c) if
at some point you are using multiple non-default instances of a form, a
reference in that form won't reliably give you the instance you want.
Exit Sub
Else
Set db = CurrentDb()
sql = "SELECT Division FROM tblIssues WHERE Division = " & "'"
& IssueDiv & "'" & ""
Set rs = db.OpenRecordset(sql)
I see no declaration for "db", "rs", or "sql".
There's no reason to do the above moves. Since you just opened the
recordset, you know that it will be at the first record if there is one,
or at both BOF and EOF if there is none. Further, if the recordset is
empty, issuing .MoveLast or .MoveFirst will raise an error. And
finally, you don't actually want to know, in this case, how many records
there are -- you just want to know if there are any. You can find that
out with a SELECT COUNT(*) query or a SELECT TOP 1 query.
If rs.RecordCount = 0 Then
MsgBox "Your selection produced zero records.",
vbOKOnly
With Form_frmIssuesDivisionSelection
Again, don't use the above form of reference.
No need to set the focus to the form; it's still there.
.cboDivisions.SetFocus
.cboDivisions.Text = vbNullString
Don't set the .Text property; set the combo's Value to Null:
Me.cboDivisions = Null
End With
Exit Sub
Else
stDocName = "frmIssues"
stLinkCriteria = "tblIssues![Division]=" & "'" &
IssueDiv & "'"
DoCmd.OpenForm stDocName, , , stLinkCriteria
End If
End If
ExitHere:
rs.Close
db.Close
Don't close a database object you got from CurrentDb(). That's the
current database, and it does you no harm to call its Close method, but
Access won't do it. In general, you should only close what you opened.
There's no need to clear this variable, as far as I can see.
Set db = Nothing
Set rs = Nothing
Form_frmIssuesDivisionSelection.SetFocus
DoCmd.Close
If you really want to close the current form, there's no need to
SetFocus to it. Just specify its name:
DoCmd.Close acForm, Me.Name, acSaveNo
However, It would make more sense to do this only in the "Else" block
where you've opened the next form.
Exit Sub
ErrorHandler:
'Error handling code is here
end sub
I'd rewrite the original sub more like this:
'------ start of revised code ------
Private Sub cmdContinue_Click()
On Error GoTo ErrorHandler
Dim strIssueDiv As String
Dim strSQL As String
Dim rs As DAO.Recordset
If IsNull(Me.cboDivisions) Then
MsgBox _
"Please select a Division" & vbCrLf & _
"from the drop down box.", _
vbOKOnly, _
"Type Selection Error"
Else
strIssueDiv = Me.cboDivisions
strSQL = _
"SELECT TOP 1 Division FROM tblIssues " & _
"WHERE Division = '" & strIssueDiv & "'"
Set rs = CurrentDb.OpenRecordset(strSQL)
With rs
If .EOF Then
MsgBox _
"Your selection produced zero records.", _
vbOKOnly, _
"No Records Found"
Me.cboDivisions = Null
Else
DoCmd.OpenForm "frmIssues", _
WhereCondition:="[Division]='" & strIssueDiv &
"'"
DoCmd.Close acForm, Me.Name, acSaveNo
End If
.Close ' close recordset
End With
End If
ExitHere:
Set rs = Nothing
Exit Sub
ErrorHandler:
'Error handling code, usually ending up with ...
Resume ExitHere
End Sub
'------ end of revised code ------
