My VBA has become really rusty but I need it for now for some data downloading.
This routine works but I'm certain that over the last 10 (at least) years something quicker/less code has surfaced to do the same thing. Any help appreciated. Please forgive my non-standard casing for VBA.
Public Function GetSymbols() As Variant
Dim ws As Worksheet
Set ws = Sheets(SymbolSheetName)
Dim tempRange, cCell As Range
Set tempRange = ws.Range(SymbolStartCell)
Set tempRange = Range(tempRange, tempRange.End(xlDown))
Dim symbolsArray() As String
Dim cntr As Integer
cntr = 0
For Each cCell In tempRange
ReDim Preserve symbolsArray(cntr)
symbolsArray(cntr) = cCell.Value
cntr = cntr + 1
Next cCell
GetSymbols = symbolsArray
End Function
1 Answer 1
As Integer
would be a problem with more than 32,767 cells - better to use As Long
here.
Set tempRange
in two consecutive assignments is suspicious, and the unqualified Range
member call in the second expression will throw error 1004 whenever ws
isn't also the ActiveSheet
(unless that code is written in the code-behind for that particular worksheet... in which case both SymbolSheetName
and ws
are redundant; simply use the code name to refer to any worksheet that exists in ThisWorkbook
at compile-time).
Dim tempRange, cCell As Range
is declaring cCells
as a Range
, and leaves tempRange
as an implicit Variant
.
Assigning a Variant
cell's value to a String
array element without validating the value would throw type mismatch errors if the cells contain any #VALUE!
(or any other type of) worksheet errors; you should use If Not IsError(cCell.Value)
to first validate whether the conversion can happen in the first place.
ReDim Preserve
inside a loop is the killer; you're looking at a Range
, and you know in advance how many cells you're looking at - you could size the array once and that would already be an improvement, but...
...you never need a loop to get a Variant
array out of every cell in a Range
:
Dim values As Variant
values = sourceRange.Value ' boom, values now contains a 2D variant array
If you want a single-dimension array and the range is small enough, you can use Application.Transpose
to get it:
Dim values As Variant
values = Application.Transpose(sourceRange.Value)
Note that the way you get the cells you're looking for could lead to unexpected results if there are any empty cells in the range: by starting at the top and going xlDown
from the first cell, your last cell is going to be at the first empty one. Usually we start at the bottom of the sheet instead, and go xlUp
, making the last cell the first non-empty cell starting from the bottom, thus making fewer assumptions about the shape and content of the data - something like this:
Dim lastRow As Long
lastRow = sheet.Range("A" & sheet.Rows.Count).End(xlUp).Row
Lastly, in the last decade Rubberduck appeared (a free & open-source VBIDE add-in I made with a couple of other VBA nerds) and can analyze your code and warn you about several things I noted in this answer.
-
\$\begingroup\$ Thanks. I use xlDown because the dataloading tool I am using will only produce usable rows. I wrote hundreds of macros using common types like Dim a,b,c as Range, never knew the first ones would turn Variant, somewhere I am sure I read you could do that that way. I wont get string errors, the range is a one column array of strings \$\endgroup\$dinotom– dinotom2022年02月26日 16:24:29 +00:00Commented Feb 26, 2022 at 16:24
-
\$\begingroup\$ ...why does the Transpose method create an array where the first index is 1 and not 0? \$\endgroup\$dinotom– dinotom2022年02月26日 16:34:39 +00:00Commented Feb 26, 2022 at 16:34
-
2\$\begingroup\$ @dinotom the array comes from a
Range
, there's no row or column 0 in Excel, so Range arrays are 1-based so the indexes line up with cell rows/columns. \$\endgroup\$Mathieu Guindon– Mathieu Guindon2022年02月26日 17:15:59 +00:00Commented Feb 26, 2022 at 17:15 -
1