Simple for loop, improve speed?

[kungfujam]

The below bit of code is running very very slowly. I cant see why this should be but there seems to be almost a second delay when I'm using f8 to run through the code bit by bit when I get to the end if...any clues?

Sub firstResponse()
    Dim rCell As Range
        
    For Each rCell In Range("S2:S462")
        If rCell.Offset(0, 3).Value = 1 Then
            rCell.Value = "CLICK"
        ElseIf rCell.Offset(0, 1).Value = 1 Then
            rCell.Value = "OPEN"
        ElseIf rCell.Offset(0, 2).Value = 1 Then
            rCell.Value = "BOUNCE"
        ElseIf rCell.Offset(0, 1).Value = "" Then
            rCell.Value = ""
        Else: rCell.Value = "NONE"
        End If
    Next rCell
    
End Sub

[SpiritualMadMan]

Are these Cells in an Excel instance or a MSFlexGrid Instance?

[BitBlt]

You might try declaring your "rCell" variable as a "Cell" rather than as a "Range". It might help with reducing some unnecessary overhead.

[kungfujam]

Thanks for both replys. I switched off screen updating i.e.

Sub Foo()
Application.ScreenUpdating = False
...code...
Application.ScreenUpdating = True
End Sub

And that was good. No big improvement though. My workbook is very large (as is the sheet in question. Searching 600 rows with 20 columns each. The slowness seems to come when allocating the current cell with a value, the rCell.Value = stage.

SpiritualMadMan I'm sorry, I've been doing VBA for a week. Could you clarify what you mean by instance. I'm working in Excel.

[SpiritualMadMan]

Understandable.

I thought you were working with an Excel instance inside a VB6 project.

But, from what has been said it is actually a VBA "macro" in a Workbook?

My Excel VBA work is extremely limited.

Updating the display always takes time and while you may not think it was much of an improvement it does add up in a loop.

The only suggestion I can come up with is to take a look at how your Workbook is constructed.

What I am saying is that the way the Sheet is structured makes simplifying the loop "difficult".

If only one cell offset was being checked then you could do a select case after assigning the cell contents to a temporary variable.

I am not sure, because I haven't done this particular task before, but, I think it takes additional time to go retrieve the value each time instead of getting the value once and comparing it multiple times.

But, it looks like you have specific ranges of values based on the cell offset and that makes it hard to simplify.

If we had some idea of how the sheet was being constructed it might help us help you.

But, right now I can't see where an improvement can be had.

Wish I did more Excel VBA stuff.

Sorry.

[kungfujam]

Cheers SpirtualMadMan,

Is this forum not for VBA in macros? Apologies if it is not. I've updated my code to ensure that it's not screen updating affecting the speed. Apparently using = vbNullString instead of = "" is also timmesaving.

Sub firstResponse()
    Dim rCell As Range
    Application.ScreenUpdating = False
    For Each rCell In Range("S2:S462")
        If rCell.Offset(0, 3).Value = 1 Then
            rCell.Value = "CLICK"
        ElseIf rCell.Offset(0, 1).Value = 1 Then
            rCell.Value = "OPEN"
        ElseIf rCell.Offset(0, 2).Value = 1 Then
            rCell.Value = "BOUNCE"
        ElseIf rCell.Offset(0, 1).Value = vbNullString Then
            rCell.Value = vbNullString
        Else: rCell.Value = "NONE"
        End If
    Next rCell
    Application.ScreenUpdating = True
    
End Sub

The data is in a list format. Perhaps there is a function I can use that takes advantage of this. I've a good mind that it's the rCell.Value = "whatever" bit that is taking all the time. Why would the reallocation of a cell be taking so much time. I am using about 500x30 = 1500 cells laid out in a table. There are empty cells.

I also have numerous sheets in the workbook. Could this be affecting anything?

[SpiritualMadMan]

No Fault about VBA, VBA and VB6 are pretty much the same thing anyway. Except VBA adds the unique bits and pieces for the Office Apps.

I am only guessing that going out and getting data from a cell takes a discrete amount of time, *possibly* a little more than a variable.

You said the data was in a list structure?

What would the list column headings be?

Are you inputting the list into Excel, then running your routine?

Are you "tied" to Excel? Or, asked differently, is the data source the Excel Worksheet?

This is an old version of a program I wrote that has a data importing function

http://www.houseofmyrrh.com/text2dao.htm

It was originally concieved to import delimited text files and create a database from them for the old version of the e-Sword Bible Study Program... It also (there are bugs mostly places where I haven't trapped for nulls in the data) allows the user to open an unknown database and see it's contents and structure...

There might be something about it that strikes your fancy?

If you were only testing a single coordinate position (offset) then I'd say use a Select Case instead...

Another trick is to, as much as possible, place the Cell test you think you will encounter most frequently at the top of your testing structure.

In other words if you look at the sheet and it looks like most of the Cells are empty, then do the empty test first so you drop out of the elseif list sooner.

[hkdani]

Whenever your working an object try to use the With statement and
include as many properties as possible.

For Each rCell In Range("S2:S462")
If rCell.Offset(0, 3).Value = 1 Then
rCell.Value = "CLICK"
ElseIf rCell.Offset(0, 1).Value = 1 Then
rCell.Value = "OPEN"
ElseIf rCell.Offset(0, 2).Value = 1 Then
rCell.Value = "BOUNCE"
ElseIf rCell.Offset(0, 1).Value = vbNullString Then
rCell.Value = vbNullString
Else: rCell.Value = "NONE"
End If
Next rCell

Change the above to the following:

For Each rcell In Range("S2:S462")
    With rcell
        If .Offset(0, 3).Value = 1 Then
            .Value = "CLICK"
        ElseIf .Offset(0, 1).Value = 1 Then
            .Value = "OPEN"
        ElseIf .Offset(0, 2).Value = 1 Then
            .Value = "BOUNCE"
        ElseIf .Offset(0, 1).Value = vbNullString Then
            .Value = vbNullString
        Else: .Value = "NONE"
        End If
    End With
Next

Microsoft recommends doing this whenever possible to improve performance by preventing the program from having to load the object again every time it appears in code. Which is what you are doing.

This should save your program from having to load the object 9 times every time it goes through one loop.

[SpiritualMadMan]

Doh! I didn't even see that one! And, it's a good and "obvious" one, too.