patternModerate
Copying and comparing two sheets before deleting duplicates
Viewed 0 times
deletingcomparingtwocopyingandbeforesheetsduplicates
Problem
I am running this code as a macro in Excel, which copies two columns from sheet 1 and pastes them into sheet two. It then compares the first column to a column in sheet two before deleting any duplicates. The two columns I am copying are product code and price. I need to compare the product code to the existing ones.
There is just under 30,000 rows that it has to compare so I know that it is always going to take some time, but I was wondering if there was any way to speed it up or even just make my code more streamline and efficient.
Private Sub CommandButton1_Click()
Dim MasterList As New Dictionary
Dim iListCount As Integer
Dim x As Variant
Dim iCtr As Integer
Dim v As Variant
Dim counter As Integer, i As Integer
counter = 0
With Sheets("Sheet2")
.Range("M:M").ClearContents
.Range("S:S").ClearContents
Sheets("Sheet1").Range("C:C").Copy
Sheets("Sheet2").Range("M1").Select
ActiveSheet.Paste
Sheets("Sheet1").Range("X:X").Copy
Sheets("Sheet2").Range("S1").Select
ActiveSheet.Paste
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
' Get count of records in master list
iListCount = .Cells(Rows.Count, "A").End(xlUp).Row
'Load Dictionary:
For iCtr = 1 To iListCount
v = .Cells(iCtr, "A").Value
If Not MasterList.Exists(v) Then MasterList.Add v, ""
Next iCtr
'Get count of records in list to be deleted
iListCount = .Cells(Rows.Count, "M").End(xlUp).Row
' Loop through the "delete" list.
For iCtr = iListCount To 1 Step -1
If MasterList.Exists(.Cells(iCtr, "M").Value) Then
.Cells(iCtr, "M").Delete shift:=xlUp
.Cells(iCtr, "S").Delete shift:=xlUp
End If
Next iCtr
End With
Application.ScreenUpdating = True
Application.Calculation = xlCalculationAutomatic
MsgBox "Done!"
End SubThere is just under 30,000 rows that it has to compare so I know that it is always going to take some time, but I was wondering if there was any way to speed it up or even just make my code more streamline and efficient.
Solution
- It's poor form to have this much logic in an event procedure. The first step is to move 99% of this code into another module and call it from the event procedure.
- Don't activate and select. This slows down your code and makes it bug prone. Use a variable to reference the worksheets and ranges you need instead.
-
There's no reason to select and paste. The
Copy method takes an optional Destination parameter.Instead of this
Sheets("Sheet1").Range("C:C").Copy
Sheets("Sheet2").Range("M1").Select
ActiveSheet.PasteUse this
Sheets("Sheet1").Range("C:C").Copy Sheets("Sheet1").Range("M1")-
Anytime you turn sheet calculation or screen updating off, you need to use an error handler to make sure it always gets turned back on.
- Look into using an ADODB connection to query the workbook instead of working directly with the COM object. It's typically a much, much faster approach.
Code Snippets
Sheets("Sheet1").Range("C:C").Copy
Sheets("Sheet2").Range("M1").Select
ActiveSheet.PasteSheets("Sheet1").Range("C:C").Copy Sheets("Sheet1").Range("M1")Context
StackExchange Code Review Q#98472, answer score: 10
Revisions (0)
No revisions yet.