Macro Causing VERY slow calculations

[hvaleagues]

I have a Macro causing slow calculation in my workbook. File size is only 2.6 mb. If I run it without macros it calculates quickly. If I enable macros, it is slow as can be. The macro I run is to sum coloured cells and is as follows.

Function SumColoured(oRange As Range, lColorIndex As Variant) As Variant
    Dim oCell As Range
    Dim oArea As Range
    Dim dSum As Double
    Application.Volatile
    For Each oArea In oRange.Areas
        For Each oCell In oRange.Cells
            If oCell.Interior.ColorIndex = lColorIndex Then
                dSum = dSum + Val(oCell.Value)
            End If
        Next
    Next
    SumColoured = dSum
End Function

Is there something in this code that slows down my calculations? Is there another more efficient way of doing it?

I have rows with cells that are shaded either rose or green. I need to be able to have 3 totals per row, one totals all the cells, the second totals the rose shaded cells and the last totals the green shaded cells. The code above works, however it really slows down all calculations.

Any insight would be greatly appreciated, as I a new to macros.

 

[prajul89]

First 2.6 MB is quite a size
Second the macro is actually a user defined function it usually slows things down especially if it has loops.

 

[prajul89]

Why is Sumcolored is defined as Variant??

 

[Jonmo1]

Application.Volatile makes it slower than normal.
This line makes the function recalculate every time anything on the sheet changes.
Regardless if what changed is related to the ranges referenced by the function.


Generally speaking, in terms of sheet development..

It is best to use coloring and formatting for disply purposes only.
When you start trying to use formatting for extra functionality, it is more headache than it's worth.

You might be better off coming up with another way to flag the cells you want to sum.

by putting an x or something in an adescent column
Then you can use Excel builtin functions like SUMIF.

 

[Gary McMaster]

I could be wrong but something doesn't look right in the inner loop.

For Each oCell In oRange.Cells

It seems it should be something like:

For Each oCell In oArea

If there are 10 areas in oRange, it looks like it will process every cell in oRange 10 times instead of processing each cell in each area once. It seems this function may be taking 10X longer than it should?

Gary

 

[Jonmo1]

I could be wrong but something doesn't look right in the inner loop.

For Each oCell In oRange.Cells

It seems it should be something like:

For Each oCell In oArea

If there are 10 areas in oRange, it looks like it will process every cell in oRange 10 times instead of processing each cell in each area once. It seems this function may be taking 10X longer than it should?

Gary

Good catch!

Not only will it take 10 times longer...
The resulting Sum will also be 10X larger than it should.
Again, assuming there are 10 areas in oRange.

 

[hvaleagues]

First off thank you for all the prompt replies! Let me clarify that I found this code online and cut and pasted it into my spreadsheet. I did not write it myself and can't comment on why certain things are in there. The code works and accomplishes what I want, but it takes about 4 seconds (I timed it) to recalculate anytime I do anything, even select an empty cell!

Jonmo1 - about the application being volatile, this describes exactly whats happening. Can I just delete that line or do I have to put something else in its place?

Application.Volatile makes it slower than normal.
This line makes the function recalculate every time anything on the sheet changes.
Regardless if what changed is related to the ranges referenced by the function.

 

[Jonmo1]

1st, I would recommend making the change Gary pointed out.
Apparrently whoever you copied it from had it wrong...

And actually, it doesn't even need the loop inside the loop.
Even if tehre are multiple areas, you can still go through all cells without the areas loop.


It should be

Function SumColoured(oRange As Range, lColorIndex As Variant) As Variant
    Dim oCell As Range
    Dim dSum As Double
    Application.Volatile
    For Each oCell In oRange
        If oCell.Interior.ColorIndex = lColorIndex Then
            dSum = dSum + Val(oCell.Value)
        End If
    Next
    SumColoured = dSum
End Function

And yes, you can remove the application.volatile line.
It won't break excel or anything..

 

[hvaleagues]

Jonmo1 - I replaced my code with yours and it solved my problem! My formulas based on shading still function and the calculation time is back to normal!

I really appreciate your taking the time to rewrite the code for me and special thanks to Gary also for catching the real issue at hand. You guys ROCK!

 

[Jonmo1]

Glad to help, thanks for the feedback.