PDA

View Full Version : VBA Code Freezing



Giri
07-15-2011, 03:50 AM
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.

Giri
07-15-2011, 05:05 AM
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?

Giri
07-15-2011, 05:35 AM
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

Giri
07-15-2011, 06:13 AM
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.

Giri
07-15-2011, 06:52 AM
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

Giri
07-16-2011, 06:38 PM
AFlatoon, thanks for your explanation! Definitely a method I will keep in mind for the future.

Paul, thanks for the suggestion!

Kind Regards,

Giri