patternMinor
Implementation of the Access version DBLookup() in VB6
Viewed 0 times
theversionvb6dblookupimplementationaccess
Problem
I've tried to implement the access function
DBLookup() in VB6 for a project I'm doing. Does anyone have comments on how well done it is and what could be improved? Take note that the Database connection object cn is only valid for a single query, and it's being initialized whenever you call the other public function, dbConn.Public Function cDLookup(TargetField As String, TargetTable As String, cTCondition As String) As String
'Eigene Implementation von DLookup
Dim Result As String
Dim rs As New ADODB.Recordset
Dim SQL As String
On Error GoTo Fehlerbehandlung
'Zusammenbauen der Query
SQL = "SELECT " & TargetField & " FROM " & TargetTable & " WHERE " & cTCondition
Call dbConn
'Initiate Database connection object cn
rs.Open SQL, cn
If (rs.RecordCount = 1) Then
Result = cleanString(rs.GetString)
Debug.Print ("[DLOOKUP] Erfolgreich Einen Datensatz gefunden und konvertiert. Output: " & Result)
ElseIf (rs.RecordCount > 1) Then
Result = "#ErrRC"
Debug.Print ("[DLOOKUP] Es wurden " & CStr(rs.RecordCount) & " Datensätze statt einem festgestellt. Dies ist nicht erlaubt")
Else
Result = "#ErrGen"
Debug.Print ("[DLOOKUP] Es ist ein Fehler in der Abfrage aufgetreten")
End If
rs.Close
cDLookup = Result
Exit Function
Fehlerbehandlung:
Debug.Print ("[DLOOKUP] Fehler im Ausführen der Prozedur cDLookup()]")
cDLookup = "#Fehler"
Exit Function
End FunctionSolution
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
Nope, that's not how
You're closing the recordset, but not the connection: if
Call
That's an obsolete statement. This instruction is completely equivalent:
It doesn't look like a procedure call, only because of the poor naming of the
Why is that comment not on top of the instruction it's commenting?
Now, if
As New
This is bad:
Why? You might think that this:
Is exactly the same as that:
But it will bite you. What does this code do?
If you were expecting a Runtime error 91 - Object reference not set, you've been bitten. This blows up as expected:
Instead of running the query off the
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:
Now you can call
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
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):
Some other considerations
Global Connection
Take note that the Database connection object
cn is only valid for a single queryNope, 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 cnThat's an obsolete statement. This instruction is completely equivalent:
dbConn
'Initiate Database connection object cnIt 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 cnWhy is that comment not on top of the instruction it's commenting?
'Initiate Database connection object cn
InitializeConnectionNow, 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 = InitializeConnectionAs New
This is bad:
Dim rs As New ADODB.RecordsetWhy? You might think that this:
Dim foo As New CollectionIs exactly the same as that:
Dim foo As Collection
Set foo = New CollectionBut 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 SubIf 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 SubInstead 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 EnumNow 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:
- Materializing any ADODB Query
- Creating ADODB Parameters on the fly
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 passedByValinstead.
- Prefer
camelCasefor locals (I know VB6 isn't case-sensitive and that's a royal PITA at times); I'd go withfieldName,tableNameandpredicate.
- 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 ear
Code Snippets
Call dbConn
'Initiate Database connection object cndbConn
'Initiate Database connection object cnInitializeConnection
'Initiate Database connection object cn'Initiate Database connection object cn
InitializeConnectionDim cn As ADODB.Connection
Set cn = InitializeConnectionContext
StackExchange Code Review Q#107751, answer score: 7
Revisions (0)
No revisions yet.