0

Is there anything especially memory-inefficient for the below code snippet? It is making Excel seem to run forever for large values (>50,000) of Long2. It works fine for medium and small values of Long2, but for larger values, it does not run a proportionally longer time, it seems to never finish. Pushing Ctrl-Break gives only the spinning progress wheel and Not Responding, even though I have DoEvents earlier in my code to display a progress bar.

Am I reading off the worksheet over and over? It is my impression that I am reading off the Range objects in memory, but please correct me if I am wrong.

Please excuse my Hungarian notation.

''A' and 'AV' Matches
If boolMatchesSheetExists = True Then
    With ThisWorkbook.Sheets("Matches")
        Long2 = Application.CountA(.Columns(1))
        Set Range2 = .Range(.Cells(1, 1), .Cells(Long2, 1))
        Set Range3 = .Range(.Cells(1, 19), .Cells(Long2, 19))
        Set Range4 = .Range(.Cells(1, 36), .Cells(Long2, 36))
    End With
    For Each Cell1 In Range1
        ''A' Matches
        If Application.CountIfs(Range2, Cell1.Value, Range3, "<>" & Cell1.Value, Range4, "A") > 0 Then
            .Cells(Cell1.Row, 2).Value = 1
        Else
            .Cells(Cell1.Row, 2).Value = 0
        End If
        ''AV' Matches
        If Application.CountIfs(Range2, Cell1.Value, Range3, "<>" & Cell1.Value, Range4, "AV") > 0 Then
            .Cells(Cell1.Row, 3).Value = 1
        Else
            .Cells(Cell1.Row, 3).Value = 0
        End If
        ''A' Claims Duping Against Self
        If Application.CountIfs(Range2, Cell1.Value, Range3, Cell1.Value) > 0 Then
            .Cells(Cell1.Row, 4).Value = 1
        Else
            .Cells(Cell1.Row, 4).Value = 0
        End If
    Next Cell1
End If

Edit: Per @200_success 's suggestion, this code populates a summary sheet ("Summary") with boolean results (0, 1) for whether matches of particular types ('A', 'AV', 'Self-Dup') were found. If you look closely at the above code you'll notice there is a missing With block that should contain the whole thing - in reality, there is one: With ThisWorkbook.Sheets("Summary").

Community
  • 1
  • 1
puzzlepiece87
  • 1,537
  • 2
  • 19
  • 36
  • May want to ask on code review? – findwindow May 03 '16 at 16:20
  • 1
    Consider **Set**ting `Range1` – Gary's Student May 03 '16 at 16:21
  • 4
    If your code is working, then you might be best using [Code Review](http://codereview.stackexhange.com) which is specifically for reviewing working code. That being said, look at using [`Range.End(xlUp)`](http://stackoverflow.com/a/30947741/4240221) instead of `CountA` to get the used cells. – SierraOscar May 03 '16 at 16:21
  • Wherever this question ends up, please include a description of what this code accomplishes. – 200_success May 03 '16 at 16:23
  • @MacroMan I may still do that, but this is a pretty mature, large program. I've pinpointed the sometimes issue to this snippet, so I was hoping to get feedback on my practices for this particular snippet. I appreciate your suggestation - I'm not sure yet if I made the right call on SO being the slightly better place to ask. – puzzlepiece87 May 03 '16 at 16:31
  • @Gary'sStudent That gets done outside this snippet, but good catch :) – puzzlepiece87 May 03 '16 at 16:36
  • 1
    Working with the Range objects, you are working with the worksheet - the CountIfs are accessing the worksheet, the `.Cells.Value = 1` are changing the worksheet. If you want to work in memory, assign the ranges to an array, do your calcs to an array, and assign the array back to the worksheet. – OldUgly May 03 '16 at 18:19
  • Thanks @OldUgly! If you make that an answer, I'll accept it if there's nothing better within 24 hours. – puzzlepiece87 May 03 '16 at 18:21
  • If you create a lot of objects, a good practice is to set them to `Nothing` after you are done with them. This recovers the memory associated with the object. You may be seeing a problem around this snippet, because of an accumulation of other problems elsewhere. - but it seems your question is more related to "speed" than to "memory" – OldUgly May 03 '16 at 18:21
  • @OldUgly What happens when I reuse Range1 over and over again? Set Range1 to this, set it to that, etc.? Do the previous objects stay in memory but lose the Range1 pointer? Do I need to reset Range1 to nothing before each new assignment? – puzzlepiece87 May 03 '16 at 18:23
  • @puzzlepiece87 - Short answer - No. Reusing the object is OK. – OldUgly May 03 '16 at 18:25
  • @OldUgly Regarding speed vs memory, I am not sure what the actual problem is. I am guessing memory since I don't see why things would break or take a much longer time with large values of Long2, but it's impossible for me to say because Excel doesn't throw any exceptions and I can't Ctrl-Break to monitor progress - pushing Ctrl-Break just gives the spinning progress wheel and Not Responding errors. I was able to pinpoint this snippet by break-executing one code section at a time until I reached this one. – puzzlepiece87 May 03 '16 at 18:29
  • 1
    Memory problems generally manifest as Excel consuming all your RAM until it crashes (2MB limit - unless 64bit). You can do some time trials with changing values of Long2 - you will find it doesn't grow linearly. – OldUgly May 03 '16 at 18:41

1 Answers1

1

Working with the Range objects, you are working with the worksheet - the CountIfs are accessing the worksheet, the .Cells.Value = 1 are changing the worksheet.

When you have very large ranges, moving data from a worksheet into an VBA variable and back again, looping, etc., can consume a lot of time.

Chip Pearson has a nice discussion on how to quickly move data from a Range into an array and vice-versa. Looping through arrays in VBA is very quick.

Community
  • 1
  • 1
OldUgly
  • 2,129
  • 3
  • 13
  • 21
  • FYI to future readers, this answer eventually lead me here: http://stackoverflow.com/questions/37174152/array-countif-substitute-countmatch – puzzlepiece87 May 11 '16 at 22:16