current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite
2

While DoCmd.RunSQL is all well and good for simple code, the number of times I've run into problems with unescaped apostrophes and the like was starting to nark a bit. With that in mind I crafted the below, a first attempt at a SQL wrapper for VBA which constructs and executes an arbitrary INSERT INTO query using parameters.

'Fields should be a comma-delimited string
Public Sub InsertInto(TableName As String, Fields As String, Values As Variant)
    Dim qdf As DAO.QueryDef
    Dim strParams As String
    Dim i As Long
    Dim strSQL As String

On Error GoTo Err_InsertInto
    'Assigns strParams value in format "[p1],[p2],[p3],...,[pN]"
    For i = 0 To UBound(Values)
        strParams = strParams & "[p" & i & "],"
    Next i
    strParams = Left(strParams, Len(strParams) - 1) 'Strip trailing comma

    'Construct SQL
    strSQL = "INSERT INTO " & TableName & "(" & Fields & ") " & _
             "VALUES (" & strParams & ");"
    Set qdf = CurrentDb.CreateQueryDef("", strSQL)

    'Give values to parameters
    For i = 0 To UBound(Values)
        qdf.Parameters("[p" & i & "]") = Values(i)
    Next i

    qdf.Execute

Exit_InsertInto:
    Set qdf = Nothing
    Exit Sub

Err_InsertInto:
    Select Case Err.Number
        Case 13   'Type mismatch i.e. Values is not an array
            Values = Array(Values)
            Resume
        Case Else 'Unhandled Error
            Set qdf = Nothing 'Release QueryDef object

            'Error is unhandled here, force it to 'bubble up' to calling code
            Dim iErrNum As Long, strErrDesc As String
            iErrNum = Err.Number
            strErrDesc = Err.Description
            On Error GoTo 0
            Err.Raise iErrNum, "SQLOps.InsertInto", strErrDesc
    End Select
End Sub

Likely to be called in a manner similar to (this example was used as a successful test case):

InsertInto "tblErrorLog", _
"ErrorNum,Description,User,When,Origin", _
Array(10,"Error's Description",Environ("USERNAME"),Now(),"A Ghost")

So far it seems to be working fine and does indeed alleviate the issues with unescaped apostrophes but I'm wondering whether this can be improved so I could use it as a sort of template for crafting similar functions for other SQL operations e.g. UPDATE SET / SELECT FROM.

Can this code be improved and are there any unforeseen bugs I should watch out for?

share|improve this question
3  
Welcome to Code Review! Looks like an interesting way of solving this problem. I hope you get some good reviews! –  Phrancis Dec 2 '14 at 15:57
1  
Welcome to Code Review! While you wait for your review, you might find this question interesting. –  RubberDuck Dec 2 '14 at 16:14

1 Answer 1

up vote 3 down vote

This code can't handle fields with spaces in them unless you specifically pass them pre-wrapped in square brackets. Things such as My Field would need to go into your parameter as "field1, [My odd field], field3". So, a definite area for improvement would be to handle that corner case.

I also don't really care for the comma delimited string parameter. I feel that it would be much cleaner and nicer to pass in a string array representing the field names. So, something along the lines of this psuedo code might be a bit cleaner.

Dim fields As Variant
fields = Array("field1","My odd field","field3")

InsertInto "Table1", fields, values)

' ...

Public Sub InsertInto(...)
    AddBracketsToStrings fields

    '...

End Sub

The next thing I notice is these comments:

'Assigns strParams value in format "[p1],[p2],[p3],...,[pN]"
...
'Construct SQL
...
'Give values to parameters
...

The comments are nice, and add a bit of clarity to the code, but these kinds of comments indicate an opportunity to extract subroutines/functions. Each of these comments could be replaced with a well named function.

ConvertValuesToParameters values
Set qdf = CurrentDb.CreateQueryDef("", ConstructSQL(TableName, Fields, strParams))
SetParameterValues qdf, values

Which brings me to the SQL Injection issue. It's never a good idea to concatenate SQL strings together. It's much better to use parameterized queries. This may require that you switch to using ADODB instead of DAO. I'm not sure. I don't know if DAO supports parameterized queries.

Case 13   'Type mismatch i.e. Values is not an array

That's a good comment, but a constant value would be better.

Const TypeMismatchError As Long = 13

Select Case Err.Number
    Case TypeMisMatchError 
        ' ...
share|improve this answer
    
Is there a way to construct a SQL statement in VBA without concatenating somewhere? I can avoid it in the VALUES(...) part of the SQL but I'm not sure how I could avoid it with the table name and field names. Google is turning me a blank on that one. ADODB is something I might have to look into but I've never used it in the past. Great point about the field names though and I've updated my code to handle that. –  Aiken Dec 3 '14 at 10:55
    
Honestly, I have no idea how I would address it with DAO. I almost exclusively use ADODB. And no, I suppose not. You will certainly have to concatenate values somewhere along the line. The important part is not to concat strings from the user into your SQL. But given what you're trying to do, I don't know how I would change it. –  RubberDuck Dec 3 '14 at 11:05

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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