Implementation of the Access version DBLookup() in VB6

Question

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

Answer 1

Potential Memory Leak

At the very beginning you declare an error handler for an undefined line label, but you never use it. This means that if an error occurs, the record set never gets properly closed and released. You should put the clean up code in a "finally" block using the Exit Function pattern. (If you look at questions tagged vba + error-handling you'll find one of my many answers on the topic.)

SQL Injection

You're concatenating arbitrary user input into a SQL query. This is terribly unsafe. It allows a malicious user virtually full control of your database. I recommend this Tom Scott video for a primer on the subject.

Style

1. The indentation is all kinds of jacked up.
2. I have no idea why some things are prefixed with a c.

• Magisch: Hey. Thank you for your time and input. I have a few things to adress some of what you said: SQL Injection this is not an issue since the function will be called only from my own program code hereforth. The values it will be called by are strictly preset and numeric, disallowing people to fudge stuff with SQL Injections.
• RubberDuck: SQL Injection is always an issue. "This will only be called from my code."... No offense meant, but I've heard that before... I get that this is neigh impossible to implement without concatenation, but I would certainly do some sanitation and sanity checking.
• Magisch: this is code for an internal application at my company that is subject to input only by barcode handscanners. There are really no avenues for someone unauthorized to inject data into the SQL database here. The user executing this code has only insert, update, select and delete rights, only on the proper tables. Every single end user of the application has similar or higher native access in the database.
• RubberDuck: Ok... But you can't say you weren't aware of the issue.
• Magisch: I know of the issue and its definately a huge problem for alot of people, my point was, it just wasn't in this case. I'd let the end users type the sql queries by hand, if I think that they could be bothered.

Answer 2

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 early and avoid sending malformed SQL to the database altogether.

• Magisch: "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." - I've asked this exact question on Stackoverflow before and was told that all I can do is wait for and handle DBerrors properly.
• Mathieu Guindon: @Magisch that's because I haven't seen that SO question. If you can avoid a trip to the database, it's usually best to avoid it.
• Magisch: I've also designed the returns on invalid inputs specifically because the functions calling the DBLookup function are specifically coded for this kind of error typing. Usually I wouldn't do this but im porting old code and I dont want to comb through tons of code to replace everything to fit a different standart.
• Magisch: stackoverflow.com/questions/33080189/… if you want to give it a bash.
• Mathieu Guindon: I'm just saying, it's bad practice. And this is where I wish Rubberduck worked in Visual Studio 6... you could fearlessly refactor that code :-(