VBA Macro to find rows and store in array runs slow

Question

So I made a simple VBA macro to run over ~7000 rows of data - the idea is that .Find finds the cells which contain "1" in column G, so that it can store the row number in an array which I shall later throw back to another sub

Unfortunately the code takes too long to run - it begs the question, have I created an infinite loop in my code? Or is asking it to loop a .find operation over 7000 cells too much for vba to handle at a reasonable speed? (i.e. do I need to improve efficiency in areas?)

Option Explicit

Public Sub splittest()

Dim sheet As Object, wb As Workbook
Dim rangeofvals As Range
Dim pullrange As Variant
Dim c As Long
Dim dynarr() As Variant

Dim xvalue As Long
Dim firstaddress As Variant
Dim count As Long
Set wb = ThisWorkbook
Set sheet = wb.Sheets("imported d")
Set rangeofvals = Range("G1:G6939")
'need to set pull range at some later point

Call OptimizeCode_Begin 'sub which turns off processes like application screen updating

xvalue = 1
ReDim dynarr(3477) 'hardcoded, replace with a countif function at some point
count = 0

With wb.Sheets("imported d").Range("G1:G6939")
     c = rangeofvals.Find(xvalue, LookIn:=xlFormulas).Row
    If c >= 0 Then
        dynarr(count) = c
       ' MsgBox firstaddress
        Do
          '  MsgBox c
            c = rangeofvals.FindNext(Cells(c, 7)).Row
            dynarr(count) = c 'apparently redim preserve would be slower
         Loop While c >= 0
        End If


Call OptimizeCode_End 'sub which turns back on processes switched off before


End With
End Sub

Questions: Is the last row of your data set static or dynamic? Are the columns where your data is stored always the same? — alowflyingpig
– alowflyingpig, Commented Dec 10, 2019 at 0:26
Get rid of Call before the OptomizeCode_Begin and OptomizeCode_End. It's been deprecated for 2 decades and is only still functional because MS is dogmatic about being backwards compatible with QBasic from 1984 (or so). Also, eliminate the underscores (_) from your function names - they are normally only used in event handlers and can cause confusion in the IDE. — FreeMan
– FreeMan, Commented Dec 10, 2019 at 14:08

the VBE - it's right for me · Accepted Answer · 2019-12-10 00:54:27Z

1

If you know the column is G and all the data is contiguous, then just loop through the rows and check the cell value directly:

Dim rows As New Collection
Dim sheet As Worksheet 
Dim lastRow, i As Integer

Set sheet = ThisWorkbook.Sheets("imported d")
lastRow = sheet.Cells(1,7).CurrentRegion.Rows.Count

For i = 1 to lastRow
    If (sheet.Cells(i,7).Value = 1) Then
        rows.Add i
    End If
Next

Unclear how the data is being used in the other sub but collection is definitely more efficient storage object for adding iteratively when the total item count is indeterminate. If you want to convert to an array then you can do so efficiently afterward since the collection tells you the item count. I'm not really sure why you would need an array specifically but I'm not going to tell you not to without seeing the client sub. Also note that the declaration of sheet was changed from Object to Worksheet since it is better to use the specific data type if possible.

edited Dec 10, 2019 at 0:54

answered Dec 9, 2019 at 23:39

the VBE - it's right for me

1687 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

alowflyingpig Over a year ago

OP wants an array not a collection. Also using sheet without referencing the actually sheet name or workbook? Suggest editing your post to include OPs variables.

the VBE - it's right for me Over a year ago

made revisions to clean up declarations. op does want array but array is not the correct way to read in an indeterminate set of values like this. like i said i cant really see why he would need array specifically but if he does it is more or less trivial to convert.

Ron Rosenfeld Over a year ago

@theVBE-it'srightforme You are absolutely correct. And, if he really wants/needs it in an array, it's trivial to fill the array from the collection.

the VBE - it's right for me Over a year ago

@RonRosenfeld yep just CollectionToArray(rows) ;-)

George c · Accepted Answer · 2019-12-09 23:28:28Z

0

...yep it was an infinite loop.

the line:

Loop While c >= 0

caused it as there is never an occasion c is less than 0 - back to the drawing board for me!

answered Dec 9, 2019 at 23:28

George c

651 silver badge10 bronze badges

Collectives™ on Stack Overflow

VBA Macro to find rows and store in array runs slow

2 Answers 2

4 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related