4
\$\begingroup\$

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
\$\endgroup\$
5
\$\begingroup\$

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 + 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.
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – mag Oct 16 '15 at 11:39
  • \$\begingroup\$ 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. \$\endgroup\$ – RubberDuck Oct 16 '15 at 11:41
  • \$\begingroup\$ 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. \$\endgroup\$ – mag Oct 16 '15 at 11:43
  • \$\begingroup\$ Ok... But you can't say you weren't aware of the issue. \$\endgroup\$ – RubberDuck Oct 16 '15 at 11:44
  • \$\begingroup\$ 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. \$\endgroup\$ – mag Oct 16 '15 at 11:45
7
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ "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. \$\endgroup\$ – mag Oct 16 '15 at 17:45
  • 2
    \$\begingroup\$ @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. \$\endgroup\$ – Mathieu Guindon Oct 16 '15 at 17:46
  • \$\begingroup\$ 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. \$\endgroup\$ – mag Oct 16 '15 at 17:47
  • \$\begingroup\$ stackoverflow.com/questions/33080189/… if you want to give it a bash. \$\endgroup\$ – mag Oct 16 '15 at 17:48
  • \$\begingroup\$ 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 :-( \$\endgroup\$ – Mathieu Guindon Oct 16 '15 at 17:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.