HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Implementation of the Access version DBLookup() in VB6

Submitted by: @import:stackexchange-codereview··
0
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 Function

Solution

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:

  • 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 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 ear

Code Snippets

Call dbConn
'Initiate Database connection object cn
dbConn
'Initiate Database connection object cn
InitializeConnection
'Initiate Database connection object cn
'Initiate Database connection object cn
InitializeConnection
Dim cn As ADODB.Connection
Set cn = InitializeConnection

Context

StackExchange Code Review Q#107751, answer score: 7

Revisions (0)

No revisions yet.