HiveBrain v1.2.0
Get Started
← Back to all entries
patternModerate

Copying and comparing two sheets before deleting duplicates

Submitted by: @import:stackexchange-codereview··
0
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.

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 Sub


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.

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


Use 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.Paste
Sheets("Sheet1").Range("C:C").Copy Sheets("Sheet1").Range("M1")

Context

StackExchange Code Review Q#98472, answer score: 10

Revisions (0)

No revisions yet.