I think apart from the performance your code also has some other issues. Let me first come to the most problematic, then make some suggestions regarding the performance, and finally make some general comments.
The biggest problem I see here is that procedure also copies functions. This can be very problematic if the references within the formulas are not absolute references. Then they will point somewhere totally different than before.
From the point above, I guess that you are only intending to copy values. In this case, you can avoid using the the rather slow copy function and simply copy the values accessing them via the Value
or Value2
property of the cells. (The latter is faster and behaves more reliably, see this question on Stack Overflow.)
To speed up things even somewhat more, you can read the entire range you like to copy into an array via Value2
and then process it. You can also set entire ranges from arrays by setting the Value
property of a range. Using arrays is the fastest thing you can do to copy large ranges. (See this blog post testing the various ways to copy the values in ranges in Excel.)
Now, the general comments on the code:
The use of Target
in the procedure, or at least its name, is rather confusing. Apparently, it is only used in the condition that is tested and it is never used as a target to copy or the like. It seems as if the check might belong into another procedure calling this one.
Your code will throw an error whenever Sheet(CE1 Data)
is not the active sheet. The problem is that in:
Set rngQuantityCells = Sheets(CE1 Data).Range(B120, Range(B120).End(xlDown))
the second Range
is not qualified, i.e. it is always implicitly referring to the active sheet. It would be a good practice to always qualify calls to Range
, even if it's from ActiveSheet
.
If the sheet Sheet(CE1 Data)
is not active, the paste range will not be inside the cleared range because the paste range will be on the active sheet. (This obviously only applies after qualifying the offending Range
from the second comment.)
You might want to save the worksheet in a variable and not query the Sheets collection all the time. Personally, I would even inject the worksheet to use as a parameter of type Worksheet
.
Given that you apparently only use this procedure from the worksheet Sheet(CE1 Data)
, you might consider using a With
statement to access the sheet.
You are using quite a number of magic numbers in the procedure, like the bounds in which Target
may be or the explicit ranges. It might be better to use constants or named ranges. If you ever want to change from where this procedure reads or to where it writes, named ranges would make your life easier.
Finally, you might consider to remove most blank lines and indent according to nesting like in:
For Each rngSinglecell In rngQuantityCells
If rngSinglecell.Value = Primary Then
Range(Range(A & rngSinglecell.Row), Range(D & rngSinglecell.Row)).Copy Range(K & Rows.Count).End(xlUp).Offset(1)
ElseIf rngSinglecell.Value = Backup Then
Range(Range(A & rngSinglecell.Row), Range(D & rngSinglecell.Row)).Copy Range(P & Rows.Count).End(xlUp).Offset(1)
End If
Next
EDIT:
I did not really realize that you wrote a handler for the worksheet changed event. So, the parameter Target
has to be there. However, it would be good practice to separate the two current responsibilities of your handler, to decide what kind of thing to do on a worksheet changed event and to copy over the cells. This can be achieved by extracting everything inside the conditional into its own sub, which preferably should not depend on the active sheet. Then my comments above apply to that sub.
One more thing: Your idea to change only the data that changed has a few issues. Whenever you change a value in column B, you have to rewrite everything because rows will potentially drop out or enter one of the two target lists. Moreover, the Target
of the worksheet changed event can contain more then one cell. In this case it might actually be faster to simply copy everything instead of figuring out which row to change in the target lists for every cell in Target
.
Considering that Target
may contain more than one cell, you might also want to consider whether your condition on Target
is correct in all circumstances.