First of all let me make an important comment not related to performance:
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.
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.)
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:
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.