Code which pulls from SQL and dumps in Excel

Question

I have a working code that pulls data from SQL and dumps in a column. Unfortunately it is extremely slow as it takes around half a second to pull each data point and it needs to be much faster. Are there any ways I can make this more efficient? Is there a different way to do this rather than go row by row?

Sub PopulatePreviousPrice()
Dim mydata As New SQLConnection
Dim sql As String


Call Input_SQLAdd_and_DatabaseName
Dim Duplicate As Long
Duplicate = -2147217900
Dim NoPrice As Integer
NoPrice = 3021
#Error code for SQL error
On Error GoTo Errorhandler

Dim tbl As ListObject
Dim PriceDate As Date
Dim PriceDateText As String
Dim sqlVal
Dim j As Integer





Set tbl = ActiveSheet.ListObjects("DailyTable")

j = 1

PriceDate = tbl.DataBodyRange(1, 25)
PriceDateText = CStr(Format(PriceDate, "dd/mmm/yyyy hh:mm:ss"))

For j = 1917 To 1 Step -1



PriceDate = tbl.DataBodyRange(j, 25)
PriceDateText = CStr(Format(PriceDate, "dd/mmm/yyyy hh:mm:ss"))


sql = "SELECT ClosingPrice " & _
  " FROM ZIPTrading.instrument.DailyPriceBondPref" & _
  " WHERE PriceDate = (SELECT max(PriceDate) From ZIPTrading.instrument.DailyPriceBondPref WHERE PriceDate < '" & PriceDateText & "' AND FIGI = '" & tbl.DataBodyRange(j, 1) & "')" & _
  " AND FIGI = '" & tbl.DataBodyRange(j, 1) & "';"



sqlVal = mydata.SelectResult(sql)



    If IsNull(sqlVal(0, 0)) = False Then

        tbl.DataBodyRange(j, 20) = sqlVal(0, 0)

    Else

    End If
sqlVal(0, 0) = ""

Next j


MsgBox ("All previous prices in table")


Errorhandler:
                If Err = Duplicate Then
                MsgBox ("Error! Data for this date already exists in the data base or data entry error. Check date and BBG pulled values")
                ElseIf Err = NoPrice Then
                    Resume Next
                    'Some securities have no price data at all and this ends the macro without warning. It's dealt with here
                Else
                msg = "Error # " & Str(Err.Number) & " was generated by " _
                & Err.Source & Chr(13) & "Error Line: " & Erl & Chr(13) & Err.Description
                MsgBox msg, , "Error", Err.HelpFile, Err.HelpContext

                End If


End Sub

Answer 1

First of all let me make an important comment not related to performance:

Security issues

I hope your table is only accessible by by a limited number of trusted people. I say this because building a SQL string like this, without further verification, is generally a bad idea. Just do a short online search for SQL injection attack to understand what I mean. Or see bobby-tables.com for a quick reference.

Performance

Now, regarding the performance. The general problem seems to be that you are querying the database for each row separately. That is usually not how you should work with a database. You can pull in larger result sets using an ADODB.Recordset or other means. Unfortunately, we cannot really see how you pull in the data since you do not provide information about mydata. In particular, we do not know what sqlVal is as you declared it (implicitly) as variant. Accordingly, I can only say something on general terms.

If the table in the database you are pulling the data from is not too large, you could probably pull in all data and filter in memory, which is faster.

If the database table is indeed very large, which I presume, you might want to dynamically generate a filtering condition based on the FIGI and dates, which I assume to be able to differ, in your table. (If the dates are always the same, this would allow a simplification.) The goal is to pull all relevant data in one go.

How to do this best really depends on the database you are talking to. If it supports CTEs (common table expressions) and has syntax to create tables on hard-coded values, it is probably best to generate such a table out of your FIGI/date pairs and do a join to build up the condition. Otherwise, you can just use a generated OR (FIGI = xxx AND PriceDate < yyy) chain in the WHERE clause. However, based on your probaly 1917 hard-coded rows, the performance might not be too good. Either filter would be applied in the subquery which would require a new GROUP BY clause on the FIGI. Then, you just join the result to the table, to get your desired values.

If your database provider supports aggregate window functions you can do the filtering on the max PriceDate using those. Then, no subquery and only an IN condition on the FIGI is required.

Without knowing more about the database you talk to, I cannot say any more regarding the query structure, since basically all databases use different SQL dialects.

That said, I wonder why you do not just use an actual database tool, e.g. Access, to query the main database and then just dump the result into the Excel file. (You can insert entire ADODB.Recordsets and you can read in data from an Excel sheet using SQL via ADO, as well.)

General Remarks

Since this is Code Review, I will also provide some thoughts about your code.

First of all, the readability really suffers from the superfluous vertical whitespace, which has been noted in a comment already, and the partial lack of indentation. To fix the indentation, you might want to visit the online VBA indenter. (Full disclosure - I am involved in the project this website belongs to.)

Second, the three executable lines in front of the For loop, do not do anything since the values are overwritten immediately in the loop, which has hard-coded bounds.

Instead of hardcoding the bounds as magic numbers, you might want to consider introducing a constant with a meaningful name.

Another thing I noticed is that you do not exit the the procedure before running into the error handler, i.e.: the error handler gets executed every time you execute the procedure. You should put an Exit Sub in front of it.

Now, a few nit-picky things:

• The Call statement you use at the top, is deprecated. It would be more idiomatic to not use it.
• You should always specify the accessibility of a procedure. If you really want it to be public, you should state that.
• Declaring a variable As New is not the same as declaring it and then setting it to a new instance. If you declare a variable As New it will only be instantiated the first time it is used and it will be re-instantiated on the next use after setting it to Nothing. Since the execution engine has to check each time whether the object still exists, there is an overhead involved.
• The use of Integer in favour of Long is largely useless. Internally, the Integer is saved as a Long. See this SO answer.
• It is generally better practice to declare variables as close to first usage as possible, contrary to what Microsoft previously suggested. (They have reverted their stand on this in more recent style guides.)
• You could separate the different concerns in this sub a bit more into separate subs and functions, following the single responsibility principle, although, admittedly, the sub already does not do all too many things.

Answer 2

There is really very little reason to write code for this at all. Write a view or stored procedure and send it to an ADO recordset in your Excel VBA, then use the Range.CopyFromRecordset method built into Excel. To do this in C#, you could use the Office.Excel.Interop class.

Range.CopyFromRecordset Method (Excel)