1
\$\begingroup\$

I'm curious if there's any way I can substantially improve its speed (it runs through >200k rows). I think I'll most likely need this once, so please bear with me since it probably looks terrible.

Thanks for reading!

Code I would like reviewed:

Option Explicit

Public Sub RemoveDuplicates()
    Dim r As Range
    Dim rBefore As Range
    Dim wsSource As Worksheet
    Dim ws As Worksheet
    Dim c As Range
    Dim c_ As Range
    Dim v As Variant
    Dim v_ As Variant
    Dim dupCount As Long
    Dim xlApp As Application
    Dim i As Long
    Dim size As Long

    Set xlApp = Excel.Application
    With xlApp
        .ScreenUpdating = False
        .DisplayStatusBar = True
        .StatusBar = "Running Script..."
    End With


    Set wsSource = xlApp.ThisWorkbook.Worksheets("source")
    Set rBefore = wsSource.Range(wsSource.Cells(2, 1), wsSource.Cells(wsSource.UsedRange.Rows.Count, 1))

    Set ws = xlApp.ThisWorkbook.Worksheets("testSheet")
    Set r = ws.Range(ws.Cells(2, 1), ws.Cells(ws.UsedRange.Rows.Count, 1))
    size = r.Count
    i = 1
    For Each v In r
        Set c = v

        DoEvents
        dupCount = 0
        For Each v_ In r
            Set c_ = v_

            DoEvents
            If CStr(c.Value) = CStr(c_.Value) Then
                dupCount = dupCount + 1
                If dupCount > 1 Then
                    c_.Rows(c_.Row).EntireRow.Delete
                End If
            End If
        Next
        xlApp.StatusBar = Format(i & " out of " & size) & " Rows Complete"
        i = i + 1
    Next

    With xlApp
        .ScreenUpdating = True
        .DisplayStatusBar = False
        .DisplayAlerts = False
        .ThisWorkbook.Save
        .DisplayAlerts = True
        .ThisWorkbook.Close
    End With
End Sub
\$\endgroup\$
  • 2
    \$\begingroup\$ Any reason why Range.RemoveDuplicates can't be used here? \$\endgroup\$ – Mathieu Guindon Aug 24 '18 at 17:48
  • 1
    \$\begingroup\$ @MathieuGuindon None other than I didn't know about it until your comment; I'll check it out! \$\endgroup\$ – jcrizk Aug 24 '18 at 17:52
2
\$\begingroup\$

Seems Range.RemoveDuplicates could turn it all into a much more efficient one-liner.

Stylistically though, the code could use some help anyway.

The rather procedure begins with a good dozen variable declarations. Code is easier to follow when you don't need to constantly scroll up and down and back to know what's what: by declaring variables closer to their usage, you reduce the cognitive load and avoid that scrolling.

Dim xlApp As Application
Set xlApp = Excel.Application

Now, there's no need for this variable; Application is free, globally-scoped, and you're not owning that instance - just copying a reference to an existing object. Don't do that, use the globals that are right there waiting to be referenced instead. This includes any worksheet that exist at compile-time in ThisWorkbook:

Dim wsSource As Worksheet
Set wsSource = xlApp.ThisWorkbook.Worksheets("source")

Every worksheet has a CodeName property, that you can change in the properties toolwindow (F4). Locate that "source" worksheet in the Project Explorer, then look at its (Name) property - change it from what defaults to e.g. Sheet1, to a meaningful identifier, e.g. SourceSheet. And now you have a free, global-scope worksheet object referring to that specific sheet, readily usable everywhere in that VBA project - which makes this wsSource variable redundant as well.

Dim rBefore As Range
Set rBefore = wsSource.Range(wsSource.Cells(2, 1), wsSource.Cells(wsSource.UsedRange.Rows.Count, 1))

It's not clear whether that r stands for Range (in which case it's useless Hungarian Notation), and Before what this range is. So we try to look at other usages to infer its meaning... and we don't find any: the variable is assigned, but the assigned reference isn't used anywhere - the variable is redundant and can be removed entirely.

Dim ws As Worksheet
Set ws = xlApp.ThisWorkbook.Worksheets("testSheet")

Again, the CodeName could be TestSheet, and ws wouldn't be needed, provided that the worksheet exists at compile-time. Otherwise, kudos for fetching it from the Worksheets collection.

Dim r As Range
Set r = ws.Range(ws.Cells(2, 1), ws.Cells(ws.UsedRange.Rows.Count, 1))

UsedRange isn't a reliable way to get the last row of a column.

r isn't a meaningful name. testRange would tell us it's from the TestSheet, but wouldn't convey what it means - any single-letter variable name can never tell us anything whatsoever about something's purpose. Unless it's i for a For...Next loop counter, which any programmer knows about; here i isn't used like that, it's more like some processedRows.

DoEvents inside the loop, combined with updating the status bar at every single iteration, is contributing to slowing things down.

You're disabling ScreenUpdating, automatic Calculation, etc.; but you leave EnableEvents enabled, so a worksheet event is fired whenever you delete a row. You could use Union to combine all the rows you want to delete, and then delete them all in one single operation - instead of deleting them one at a time. But then again, Range.RemoveDuplicates makes that moot.

There are a number of issues (it's a constant work-in-progress), but Rubberduck could have picked up a number of the observations I made above. Kudos for Option Explicit, the meaningful name, and the explicit Public modifier on your macro!

\$\endgroup\$
  • \$\begingroup\$ Stylistically, I know it's garbage, but there's a wealth of info NuGets that didn't even cross my mind. Thanks for the detailed answer! \$\endgroup\$ – jcrizk Aug 24 '18 at 21:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.