View Full Version : VBA Code Freezing
Hi Guys,
I have the following code which works fine when Application.ScreenUpdating = False is NOT turned on but simply freezes when it IS turned on...
I don't quite understand why this is happening. When it freezes and I press ESC, the code which is highlighted is the FIRST "End if" line.
Any help would be greatly appreciated!
P.S The code to highlight the cells was left in to see what the macro was executing.
Kind Regards,
Giri
Public Sub UUT()
Dim c As Variant
Dim n As Integer
Dim row As Integer
Dim stringArray(1) As String
Dim integerArray(1) As Double
row = Range("W7").End(xlDown).row
For c = row To 7 Step -1
Range("W" & c).Select
stringArray(0) = Range("W" & c).Value
If stringArray(0) = "External Product" Then
Range("W" & c).EntireRow.Delete
End If
Next c
row = Range("V7").End(xlDown).row
For c = row To 7 Step -1
Range("V" & c).Select
integerArray(0) = Range("V" & c).Value
integerArray(1) = Range("U" & c).Value
If integerArray(0) = 0 And integerArray(1) = 0 Then
Range("V" & c).EntireRow.Delete
End If
Next c
End Sub
p45cal
07-15-2011, 04:31 AM
Could it be that it's not frozen, but that
Range("W7").End(xlDown).row
has found the last row in the sheet (more than a million in xl2007 up) and is simply taking a long time?
Temporarily put a line in that loop like:
Application.statusbar="Processing row " & c
which should change even if Application.ScreenUpdating is set to False.
Don't forget to reset it to "" after.
re:" The code to highlight the cells was left in to see what the macro was executing."
While the code selects cells, if screenupdating is turned off you won't see this happening.
Aflatoon
07-15-2011, 04:50 AM
I would have thought an Autofilter might be quicker too.
Thanks for the replies.
p45cal, you're right - the macro isn't freezing. It's just taking a long time to do what it's supposed to do. It took about 30 secs for the macro to complete.
The excel sheet I ran the macro on had 13 rows and 173 columns... not sure if this is considered big and explains the time it took for the macro to complete??
Is there any way I could shorten this period?
Aflatoon, I was actually using AutoFilters to complete this process but there are a few other things that need to be done afterwards which I have not posted the code up for. So I was hoping just to get everything done through a macro.
Thanks for the help guys.
Giri
Aflatoon
07-15-2011, 05:08 AM
I meant that you could use autofilters in your code, rather than looping. :)
p45cal
07-15-2011, 05:14 AM
30secs is a long time for such a small task There are several things that might be slowing things down. First things first;
Add a line immediately after:
row = Range("W7").End(xlDown).row
which reads:
Msgbox row
and the same after the line:
row = Range("V7").End(xlDown).row
run the macro. What values did the message boxes show?
p45cal, the first message box reads 173 and the 2nd reads 127.
Aflatoon, LOL ok, I've never used autofilters in VBA before but I will look into that.
Thanks guys.
Giri
p45cal
07-15-2011, 05:47 AM
OK, so that's not too bad - not in the 65000 or 1 million range. Keep the lines in for now jut to keep tabs, so next question is why do you set up arrays to contain values, then compare those values then delete a row? Why not compare the values directly? (what's in those cells? and what are you really trying to do?, in words, not vba code)).
It could be that we can use autofilter instead, but I want to be sure that we'll be looking for the right thing.
What version of Excel, btw?
Another couple:
1.Have you got loads of formulae in the worksheet, perhaps on other sheets referring to the cells you're deleting? If so turn automatic calculation off while the code is running.
2. Do you have any event code (eg. sheet selection change or sheet change) running too?
Also, remove those .Select lines altogether, do turn screenupdating off too.
For c = row To 7 Step -1
Range("W" & c).Select
stringArray(0) = Range("W" & c).Value
If stringArray(0) = "External Product" Then
Range("W" & c).EntireRow.Delete
End If
Next c
can be reduced to:
For c = row To 7 Step -1
If Range("W" & c) = "External Product" Then Rows(c).Delete
Next c
So I changed the code like you specified but it is still running slowly... there may have been a slight improvement.
With regards to your questions, there are no formulas on this or any other worksheet. And this is basically the only code in the workbook. I am running Excel 2007. It's quite odd because I have another macro in another workbook that does basically the same thing, which runs instantaneously....
Something that may or may not be relevant - two of the columns contain drop down boxes which can be used to change the data in the cell. Besides that, it seems to just be a regular worksheet. I would post up the spreadsheet but it's related to work so I would rather not.
I appreciate all your help p45cal!
Giri
Option Explicit
Public Sub UUT()
Dim c As Variant
Dim n As Integer
Dim x As Integer
Dim y As Integer
Dim row As Integer
Dim stringArray(1) As String
Dim integerArray(1) As Double
Application.ScreenUpdating = False
row = Range("W7").End(xlDown).row
For c = row To 7 Step -1
If Range("W" & c).Value = "External Product" Then
Rows(c).Delete
End If
Next c
row = Range("V7").End(xlDown).row
For c = row To 7 Step -1
If Range("V" & c).Value = 0 And Range("U" & c).Value = 0 Then
Rows(c).Delete
End If
Next c
Application.ScreenUpdating = True
End Sub
Aflatoon
07-15-2011, 06:25 AM
Looping twice is also unnecessary:
Public Sub UUT()
Dim c As Long, lngRow As Long
Dim rng As Range
Application.ScreenUpdating = False
lngRow = Range("W7").End(xlDown).row
For c = lngRow To 7 Step -1
If Range("W" & c).Value = "External Product" Or (Range("V" & c).Value = 0 And Range("U" & c).Value = 0) Then
If rng Is Nothing Then
Set rng = Rows(c)
Else
Set rng = Union(rng, Rows(c))
End If
End If
Next c
If Not rng Is Nothing Then rng.EntireRow.Delete
Application.ScreenUpdating = True
End Sub
p45cal
07-15-2011, 06:30 AM
Could you also answer: "so next question is why do you set up arrays to contain values, then compare those values then delete a row? Why not compare the values directly? (what's in those cells? and what are you really trying to do?, in words, not vba code))"
There being dropdows (data validation?) might have a bearing - test it yourself by duplicating the sheet and removing all such boxes (easy if they're data validation - F5, Special… Data Validation (all) OK, Data Validation in the ribbon, Data Validation…, Clear all, OK) then running the macro on that new sheet.
Aflatoon, Thanks so much for that! The macro is now being completed in a few seconds! So the issue was probably the two loops??
I had a question about the following line:
If rng Is Nothing Then
What has "rng" been set to?
p45cal, Thanks for all your help too!
I think I'm going to go to bed now .. lol. Have a good weekend!
Kind Regards,
Giri
Aflatoon
07-15-2011, 07:28 AM
rng is a Range object. Each time a matching row is found, the code tests to see if rng has already been assigned a range; if not, it assigns the found range, but if so, it needs to use Union to add the found range to the ranges already found. Then at the end all the ranges are deleted in one go rather than doing several deletes, which can be quite slow.
Paul_Hossler
07-15-2011, 08:06 AM
This works if you're sure that there's no blank cells and there's are least something in W8
lngRow = Range("W7").End(xlDown).row
It'd be safer IMHO to start at the bottom and go up to the first non-blank cell
lngRow = Range("W" & ActiveSheet.Rows.Count).End(xlUp).Row
This way if you did have data in W7 - W27 and W29 - W49, lngRow would return 49, instead of 27
Paul
AFlatoon, thanks for your explanation! Definitely a method I will keep in mind for the future.
Paul, thanks for the suggestion!
Kind Regards,
Giri
Powered by vBulletin® Version 4.2.5 Copyright © 2024 vBulletin Solutions Inc. All rights reserved.