Find and copy pasting optimisation

I am trying to apply this simple macro to merge two big sized sheets (around 30000 rows each), but the process is too slow and never ends. The macro works perfectly with smaller sheets. Could you give me any advice for the optimization of my code please ?

My Macro consists in an Userform :

```
Public listChoice As String

'Using your code to get the sheet names for the ListBox rowsource.

Private Sub UserForm_Activate()

For n = 1 To ActiveWorkbook.Sheets.Count
With SelectSheet
.AddItem ActiveWorkbook.Sheets(n).Name
End With
Next n

End Sub

'Including an update event for the ListBox

Private Sub SelectSheet_AfterUpdate()

listChoice = SelectSheet.Text

End Sub

'Including a test just to demonstrate that the result is still retained. You don't need this, it demonstrates the results on the screenshot.

Private Sub CommandButton1_Click()

Dim sh1 As Worksheet
Dim sh2 As Worksheet
Dim sh3 As Worksheet
Dim lc As String
Dim letter2 As String
Dim letter3 As String
Dim mrgKeyRange1 As Range
Dim mrgKeyRange2 As Range
Dim cell As Range
Dim lastC1 As Integer
Dim lastC2 As Integer
Dim lastC3 As Integer
Dim lrow As Integer
Dim currentR As Integer
Dim key As Variant

lc = listChoice

'closing the UserForm
Unload Me

Set mrgKeyRange1 = Application.InputBox("Select the range by wich the rows of the current sheet will be merged with the sheet " & lc, Type:=8) 'type 8 serve a fargli pigliare un range
Set mrgKeyRange2 = Application.InputBox("Select the corresponding range in " & lc, Type:=8)

Application.ScreenUpdating = False
Application.EnableEvents = False
Application.Calculation = xlCalculationManual 'if there are many calculations, it helps to speed the macro up

Set sh1 = ActiveSheet
Set sh2 = ActiveWorkbook.Sheets(lc)
Set sh3 = Sheets.Add

'renaming the new sheet
If Len(sh1.Name) >CR note: the macro begins to be veeeeeery slo

The first thing I expect to see in any code module - be it a standard module, a class module, a worksheet module, ThisWorkbook, or a UserForm's code-behind, is this:

Option Explicit

With this, your code wouldn't compile... because you're not declaring every variable that you're using - in the UserForm_Activate handler, n is an implicit Variant that VBA allocates on the fly... and Variant should be avoided for routine tasks such as looping:

Private Sub UserForm_Activate()

    For n = 1 To ActiveWorkbook.Sheets.Count
        With SelectSheet
            .AddItem ActiveWorkbook.Sheets(n).Name
        End With
    Next n

End Sub

There are a few things to improve here. A For Each loop performs best when iterating an array; when iterating a collection of objects (such as the Sheets collection of the ActiveWorkbook), it's best to use a For Each loop:

Dim sheet As Worksheet
For Each sheet In ActiveWorkbook.Worksheets
    SelectSheet.AddItem sheet.Name
Next

Notice this loop is iterating the Worksheets collection - the Sheets collection contains sheets that aren't worksheets, such as chart sheets.

A new sheet reference is "captured" at each iteration, so you don't need to access the Sheets or Worksheets collection every time - it's simply given to you by the iteration mechanism; that's why a For Each loop performs better with collections. Keep For...Next loops for iterating arrays.

Public listChoice As String

Given that a UserForm is really a class module with a designer and a default instance, it should be used as an object - and in object-oriented code, this listChoice module-level variable is a public field.

A public field makes the String value readable from the calling code. The problem is that is also makes the value writable from the calling code... which doesn't always make sense and makes it easier to introduce bugs.

I like that you've abstracted away the ListBox, so the caller doesn't need to know that the selection is coming from a specific control. A better and more object-oriented way to do exactly that is to expose a property:

Public Property Get SelectedSheetName() As String
    With SelectSheet
        If .ListIndex = -1 Then Exit Property 'nothing is selected
        SelectedSheetName = .List(.ListIndex)
    End With
End Property

Notice that this eliminates the need for a public field for the client code to access the selected sheet name.

Now, looking at the rest of the code, it seems the listChoice field / SelectedSheetName property can very well be Private, given it's only used in the module it's declared in: variables and members should always have the tightest possible scope - I like that all members are Private, but that Public field makes this code possible:

Merge_UserForm.listChoice = "potato"
Merge_UserForm.Show vbModeless

And then the user could click the CommandButton1 without making a valid selection in the SelectSheet listbox, and then this line would blow up:

Set sh2 = ActiveWorkbook.Sheets(lc)

Side note, what's the lc local variable needed for? listChoice is already there, in scope, waiting to be used... but I'll get back to this in a moment.

I think the form, CommandButton1 in particular, is responsible for way too many things. A UserForm is a view, a user interface: a UI exists because a program needs to collect user input.

I think these InputBox calls are a missed opportunity to have two RefEdit controls on that form, to collect Range selections from the user without ugly and annoying InputBox prompts - not to mention that Excel.Application.InputBox is a bit confusing when there's also the standard VBA.Interaction.InputBox. And you're not validating that the selected range is actually on the lc sheet, which can lead to some interesting bugs.

Why do you need that ListBox at all anyway then? Let the user select a range, and extract the sheet's name from that range!

So back the form's responsibilities: collecting user input. What are the actual inputs you need? mrgKeyRange1 and mrgKeyRange2? I'm not sure I completely understand exactly what's happening (haven't looked at the actual "do work" code yet), but it seems to me your UI could look something like this:

Then there would be some logic to validate the selected ranges and make sure the form can't be OK'd without consistent input values (e.g. if the selected columns/rows need to line up, or if the two ranges must be on separate sheets, etc.) - the entire role and purpose of a UserForm is to collect and validate the user's input.

So the only code that should be in a form's code-behind, is simple code that deals with simple mechanics, e.g. making sure that the object instance doesn't get destroyed when the user decides to X-out and cancel everything:

```
Private cancelled As Boolean

Public Property Get IsCancelled() As Boolean
IsCancelled = cancelled
End Property

Publ

StackExchange Code Review Q#140204, answer score: 5