Transfer of data instead of copy.paste.clear used

Question

I am using this code in a worksheet to check the condition, and based on the condition, copying the entire row of values in a different range of particular condition if true.

For copying I am clearing the destination range each time and copying the same data with updated information on the same destination. This makes the code running longer. Can I make the code to copy only the updated cells and merge onto the destination ranges where the old data exists?

Private Sub Worksheet_Change(ByVal Target As Range)
Application.ScreenUpdating = False
If (Target.Row >= 120 And Target.Row < 1400) And (Target.Column >= 1 And Target.Column <= 4) Then
 Dim rngSinglecell As Range
 Dim rngQuantityCells As Range
 Dim intCount As Integer
 Sheets("CE1 Data").Range("K2:S1400").Clear
 Set rngQuantityCells = Sheets("CE1 Data").Range("B120", Range("B120").End(xlDown))
 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
End If
Application.ScreenUpdating = True
Application.CutCopyMode = False
End Sub

Answer 1

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:

1. 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.

2. 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.

3. 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.)

4. 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.

5. 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.

6. 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.

• santu: Hello Doerner, Thanks a lot for the detailed review and suggested edits in my code. It certainly takes some time for me to understand all your points and implement.