Efficiently create and sort a Collection

The goal of my code is to sort data into two categories. It must use a local copy of the initial data from Collar (Top View).csv. My code creates a Collection of items called Collars using the initial data file, then moves each Collar into its respective category based upon its E dimension. I would like feedback on if I could do this more efficiently and readable, but other feedback is welcomed.

```
Option Explicit
Option Base 1
Dim CollarCol As New Collection
Dim BatchNum As String

' Calls for creation of a collection of collars and then calls that to be sorted.
Sub SortButton_Click()

' Clear current values
Range("D3:L30").Clear

' Create local copy. Cannot open live copies of files.
FileCopy "O:\IQC_Inspection\EngineeringData\Collar (Top View).csv", _
ActiveWorkbook.Path + "\" + "Collar (Top View).csv"

' Get user input for desired batch number
On Error GoTo ErrorHandler
BatchNum = InputBox(Prompt:="Enter batch number: ")
If (BatchNum = 0) Then Exit Sub ' exit for cancel button

Set CollarCol = New Collection
Call PopulateCollarCol
Call SortCollarCol
Exit Sub

ErrorHandler:
MsgBox Err & ": " & Error(Err)

End Sub

' Populates the Collection named CollarCol
Private Sub PopulateCollarCol()

Workbooks.Open ActiveWorkbook.Path + "\" + "Collar (Top View).csv"
Dim Index As Integer, EndIndex As Integer
Dim NewCollar As Collar
EndIndex = FindEnd(BatchNum)

For Index = FindStart(BatchNum) To EndIndex
Set NewCollar = New Collar
' If first measure, add to collection
If (Cells(Index, 11) = 0) Then '
NewCollar.SetBatchNum (Cells(Index, 9))
NewCollar.SetSerialNum (Cells(Index, 10))
NewCollar.SetDimE (Cells(Index, 13))
CollarCol.Add Item:=NewCollar, key:=CStr(NewCollar.GetSerialNum)
Else ' see if remeasure is done for DimE
If (Cells(Index, 15) <> " ") Then
Dim EditCollar As New Collar

General Impression

• It's better than most VBA I see. I think you generally did a pretty good job.

• Event Handlers shouldn't have very much code in them. How would you run this code "headless" (without a person interacting with the UI) if you needed to? I would consider breaking the SortButton_Click() event procedure into at least one or two more subroutines.

• I would actually recommend moving almost all of this logic into a class module. Keep your code behinds clean of all business logic. Code behinds should be mainly responsible for dealing with UI events and calling on classes that hold the business logic.

-
The string literal Collar (Top View) shows up a lot. Extract a constant to store it in. Be careful however, you use it in two different contexts. In some places it refers to a file name and in others it is a sheet name. So, you actually need two different constants. It's perfectly okay to let one constant reference the other though. This is completely legit and compilable code.

Private Const sheetName As String = "Collar (Top View)"
Private Const fileName As String = sheetName & ".csv"

SortButton_Click

• You're turning the error handling on pretty late. What happens if there's an issue with the FileCopy command? The code will break on that line. Probably not what you want to happen. Generally speaking, if you're using On Error GoTo it should be the first line after the sub declaration.

-
Be explicit about scope.

Sub SortButton_Click()

Scope is public by default in VBA, unlike .Net where it's private by default. That alone is a good reason to be explicit about how things are scope. It will reduce confusion for anyone (including yourself) who may move between the two languages. It's one less thing to remember.

Also, did you actually mean to make this Public? I can't think of a good reason for an event handler to be public. If you need to call the code inside of it, it would be much better to extract the logic into a public subroutine of it's own.

-
Using Range all on its own implicitly calls ActiveSheet.Range. It's always better to be explicit and in turn, it's rarely recommended to work on the active worksheet. There might not be another option here though. This could be one of those rare times.

-
Give this a newline for readability.

BatchNum = InputBox(Prompt:="Enter batch number: ")  
If (BatchNum = 0) Then Exit Sub ' exit for cancel button

BatchNum = InputBox(Prompt:="Enter batch number: ")  

If (BatchNum = 0) Then Exit Sub ' exit for cancel button

Speaking of readability, you might want to ditch the one-line If in favor of the more verbose If block syntax.

PopulateCollarCol

-
You've repeated this code from the click event.

ActiveWorkbook.Path + "\" + "Collar (Top View).csv"

You should be passing that filepath into the subroutine as an argument simply to keep the code DRY, but there's another issue here. What if the user clicks on a different workbook after the click event starts, but before the code execution gets here? You could execute in a different path. (Unlikely, but possible.)

-
When I first saw this, I expected to give my spiel about implicitly declared variants, but you declared these variables correctly. Well done. I see this get screwed up a lot, but you didn't.

Dim Index As Integer, EndIndex As Integer

-
Nothing guarantees that someone won't come behind you and call this function before there is a valid (Not Nothing) Collar collection to work with before you add it. It's a potential bug. A simple fix would be

If CollarCol Is Nothing Then Set CollarCol = New Collection

-
If you want to make this more efficient, don't open and close the csv file as a workbook. Opening a workbook is an expensive operation. Instead, use an adodb recordset to read in the closed file and loop through the recordset instead. Here is one example of how to get the data into a recordset.

FindStart & FindEnd

I'm a bit torn on these. On one hand, they do one thing and do it well. They're also nicely decoupled. On the other, they share some copy/pasted code. DRYing these out to share the common code would couple them together in a way I'm not sure I care for. You could have FindStart() call FindEnd() if you chose to do so.

Now, assuming that by "efficient" you meant you want to squeeze every last bit of performance out of the code you could do something along the following lines, but I'm not sure I'd really recommend it. Take it for what it's worth to you.

To find the starting index, you first have to find the ending index. You also call both of these functions in rapid succession. This means that you're .Finding the same value twice in a row. What you could do (and again, I'm not sure I recommend actually doing this) is take advantage of passing arguments ByRef and turn your two functions into a single Sub that overwrites the values of some out parameters. This is more efficient because

StackExchange Code Review Q#78155, answer score: 8