In addition to @RubberDuck's excellent input (which I completely agree with), I have the following concerns:
Global Connection
Take note that the Database connection object cn
is only valid for a single query
Nope, that's not how ADODB.Connection
works. The reality here is that you have a function that consumes a connection that it doesn't own, encapsulated by an object that is global. Why isn't dbConn
a function that returns the initialized connection? And if it initializes a connection, then why isn't it called InitializeConnection
or GetConnection
? And if cn
is "only valid for a signle query", then why would it ever need to be visible to anything other than the procedure that needs an opened connection?
You're closing the recordset, but not the connection: if dbConn
starts by closing the previous connection before opening a new one, this means you're leaving a database connection opened for much, much longer than you actually need to, and that's dirty. If dbConn
doesn't do that, then your connection object simply falls in limbo, and that's outright wrong.
Call
Call dbConn
'Initiate Database connection object cn
That's an obsolete statement. This instruction is completely equivalent:
dbConn
'Initiate Database connection object cn
It doesn't look like a procedure call, only because of the poor naming of the dbconn
procedure: if the name started with a verb...
InitializeConnection
'Initiate Database connection object cn
Why is that comment not on top of the instruction it's commenting?
'Initiate Database connection object cn
InitializeConnection
Now, if cn
was local as it should be, that comment wouldn't even be needed, as the code would speak for itself:
Dim cn As ADODB.Connection
Set cn = InitializeConnection
As New
This is bad:
Dim rs As New ADODB.Recordset
Why? You might think that this:
Dim foo As New Collection
Is exactly the same as that:
Dim foo As Collection
Set foo = New Collection
But it will bite you. What does this code do?
Public Sub TestMe()
Dim foo As New Collection
foo.Add "bar"
Set foo = Nothing
foo.Add "surprise!"
End Sub
If you were expecting a Runtime error 91 - Object reference not set, you've been bitten. This blows up as expected:
Public Sub TestMe()
Dim foo As Collection
Set foo = New Collection
foo.Add "bar"
Set foo = Nothing
foo.Add "surprise!" 'error 91
End Sub
Instead of running the query off the rs
object with rs.Open
, get a recordset instance from the connection with cn.Execute
instead:
Set rs = cn.Execute(SQL)
Magic Return Values
Your function has several ways of "failing". A number of them involve an actual runtime error that the client code must handle. And if my German is correct, 3 of them involve returning a valid string that the client code must check (or treat as valid, and propagate to the UI?). Don't use your return value for that, raise proper errors: the client code should already be handling runtime errors, all you need to do is come up with a way of telling it exactly what went wrong.
An enum works great for that:
Public Enum DLookupError
ErrNoRecordsReturned = vbObjectError + 42
ErrMultipleRecordsReturned
End Enum
Now you can call Err.Raise DLookupError.ErrNoRecordsReturned, "DLookup", "Es ist ein Fehler in der Abfrage aufgetreten."
to raise a runtime error that can be handled as such, effectively only ever returning an actual string result to your client code when there's such a result to return.
I left out the general "failure" error, since that's simply an error that you should bubble up to the caller.
Responsibilities
Code that's sprinkled with rs.Open
calls all over the place, is code with poorly separated concerns. If your application needs to access a database, then you need an object whose responsibility is do to exactly that, so that the rest of the code can work at a higher abstraction level and not be bothered with connections and recordsets.
I've done exactly that in a previous life, and put everything up right here on Code Review, so you can see if you want something similar:
The entire function could be pretty much replaced with this (error-handling omitted):
Dim sql As String
sql = "SELECT " & fieldName & " FROM " & tableName & " WHERE " & predicate
Dim result As String
result = SqlCommand.QuickSelectSingleValue(sql)
Some other considerations
- Parameters are implicitly passed
ByRef
; they can/should be passed ByVal
instead.
- Prefer
camelCase
for locals (I know VB6 isn't case-sensitive and that's a royal PITA at times); I'd go with fieldName
, tableName
and predicate
.
- You're not validating any of the parameters; an empty string is a valid condition resulting in invalid SQL. Granted that's handled by the error-handling there, but you could fail early and avoid sending malformed SQL to the database altogether.