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 Public Property Get Selection1() As Range 'todo: rename On Error Resume Next Set Selection1 = Application.Range(RefEdit1.Value) On Error GoTo 0 End Property Public Property Get Selection2() As Range 'todo: rename On Error Resume Next Set Selection2 = Application.Range(RefEdit2.Value) On Error GoTo 0 End Property Private Sub OkButton_Click() cancelled = False Me.Hide End Sub Private Sub UserForm_QueryClose(Cancel As Integer, CloseMode As Integer) If CloseMode = VbQueryClose.vbFormControlMenu Then cancelled = True Cancel = True Me.Hide End Sub
If the input ranges can be invalid and you would want to prevent the user from OK'ing the form with such invalid input, I would suggest to keep that responsibility outside the form still - by implementing it in dedicated class modules.
I don't know what the rules are, and I don't know how many there are - so I'll go and add a new class module and call it IRangeValidationRule to define a simple interface:
Option Explicit Public Function Validate(ByVal range1 As Range, ByVal range2 As Range) As Boolean End Function
And then say one of the rules is that range1 and range2 must refer to distinct sheets - we would have another class module, DistinctSheetRangeValidationRule:
Option Explicit Implements IRangeValidationRule Private Function IRangeValidationRule_Validate(ByVal range1 As Range, ByVal range2 As Range) As Boolean On Error GoTo CleanFail Dim result As Boolean result = Not range1.Parent Is range2.Parent CleanExit: IRangeValidationRule_Validate = result Exit Function CleanFail: result = False Resume CleanExit End Function
And so on for each validation rule you might have. Then the form could have this:
Private rules As New Collection Public Sub AddValidationRule(ByVal rule As IRangeValidationRule) rules.Add rule End Sub
And you could then determine whether or not the selected ranges are valid by simply iterating the rules:
Private Function IsValid() As Boolean Dim rule As IRangeValidationRule For Each rule In rules If Not rule.Validate(Selection1, Selection2) Then IsValid = False Exit Function End If Next IsValid = True End Function
And then you could handle the two RefEdit controls' AfterUpdate handlers to run the validation and disable the OkButton until the input is valid:
Private Sub RefEdit1_AfterUpdate() OkButton.Enabled = IsValid End Sub Private Sub RefEdit2_AfterUpdate() OkButton.Enabled = IsValid End Sub
So, what would the calling code look like then? This couldn't work:
Sub MergeSheets() Merge_UserForm.Show vbModeless End Sub
First, it's working off the default instance of the form, and then with the actual "do work" code gone, that wouldn't be doing anything. We need to create an instance and work with that - but first let's remove that underscore and keep the form's name PascalCase; the underscore makes it look like some event handler or interface member implementation procedure. Then we'll restrict user interactions to entering valid input or cancelling the form by using vbModal, so while the form is displayed, the user can't interact with anything other than the form:
Public Sub MergeSheets() With New MergeUserForm .Show vbModal If Not .IsCancelled Then DoWork .Selection1, .Selection2 'todo: rename all these End With End Sub
Notice the conceptual difference here: instead of fire-and-forget displaying a form and not knowing what's happening afterwards, we can see that we're displaying a form, collecting Selection1 and Selection2, allowing the user to cancel everything, and passing the inputs to some DoWork procedure that's responsible for the actual work - the form itself doesn't do much.
If we have implemented validation rules as I've shown above, we could have this:
Public Sub MergeSheets() With New MergeUserForm .AddValidationRule New DistinctSheetValidationRule .AddValidationRule New SomeOtherValidationRule .Show vbModal If Not .IsCancelled Then DoWork .Selection1, .Selection2 'todo: rename all these End With End Sub
The biggest advantage with this approach, is that you've decoupled the actual work (and input validation rules) from the UI, and you can even write unit tests for them if you want - and because the DoWork procedure is taking the user input as parameters rather than prompting for it, you can write unit tests for it too!
I'll end this answer here and let other reviewers address the actual do work procedure and its performance problems.
