Efficiency through functions, I am lost

Question

I struggle with VBA - I tend to write as though I am the Macro Recorder and as a result write really ugly macros and end up making things far more complicated than needs be.

Can you possibly have a look and help identify some efficiencies? I want to learn to write good code, but need to compare and contrast and its hard from looking at other peoples examples.

Sub ColumnSearch()
'Filepath variables - the filename changes daily but always contains the name notifications, this seemed to be the easiest method

Dim filepath As String
filepath = ActiveWorkbook.Path + "\"
Application.ScreenUpdating = False
Dim file As String
Dim fullfilepath As String
file = Dir$(filepath & "*Notifications*" & ".*")
fullfilepath = filepath & file
Application.EnableEvents = False
Workbooks.Open (fullfilepath)
Windows(file).Activate
Application.EnableEvents = True


'variables set as string and range respetively

Dim strDoN As String, strOffice As String, strARN As String, strPIN As String, strAN As String, strAT As String, strSoS As String
Dim rngDoN As Range, rngOffice As Range, rngARN As Range, rngPIN As Range, rngAN As Range, rngAT As Range, rngSoS As Range
Dim rng2DoN As Range, rng2Office As Range, rng2ARN As Range, rng2PIN As Range, rng2AN As Range, rng2AT As Range, rng2SoS As Range

Dim myRange As Range
Dim NumCols, i As Integer

'str variables set as the text in row 1 (title cells)
strDoN = "Date of Notification"
strOffice = "Office Centre"
strARN = "Appeal Reference Number"
strPIN = "PIN"
strAN = "Appellant Name"
strAT = "Appeal Type"
strSoS = "SoS Decision Date"

Sheets("New Appeals").Activate

'For loop to find the address of the strings above

For i = 1 To 11
    Select Case Cells(1, i).Value
        Case strDoN
            Set rngDoN = Cells(1, i)   '
        Case strOffice
            Set rngOffice = Cells(1, i)
        Case strARN
            Set rngARN = Cells(1, i)
        Case strPIN
            Set rngPIN = Cells(1, i)
        Case strAN
            Set rngAN = Cells(1, i)
        Case strAT
            Set rngAT = Cells(1, i)
        Case strSoS
            Set rngSoS = Cells(1, i)
        Case Else
            'no match - do nothing
    End Select
Next i

'Identify the count of cells to be copied from one sheet to the other
RowLast = Cells(Rows.Count, rngOffice.Column).End(xlUp).Row
Cells(RowLast - 1, rngOffice.Column).Select
Range(Selection, Selection.End(xlUp)).Offset(1, 0).Copy

'activate the other workbook, run the same search for loop but with rng2 being set (rng and rng2 can be different as there are sometimes extra columns that are not required
Workbooks("Book2.xlsm").Activate
Sheets("New Appeals").Select



For i = 1 To 11
    Select Case Cells(1, i).Value
        Case strDoN
            Set rng2DoN = Cells(1, i)   '<~~ set the range object to this cell
        Case strOffice
            Set rng2Office = Cells(1, i)
        Case strARN
            Set rng2ARN = Cells(1, i)
        Case strPIN
            Set rng2PIN = Cells(1, i)
        Case strAN
            Set rng2AN = Cells(1, i)
        Case strAT
            Set rng2AT = Cells(1, i)
        Case strSoS
            Set rng2SoS = Cells(1, i)
        Case Else
            'no match - do nothing
    End Select
Next i

Dim RowLast2 As Long

'find the last cell that was updated (every day the list will grow, it has to be pasted at the bottom of the last update)

RowLast2 = Cells(Rows.Count, rng2Office.Column).End(xlUp).Row
Cells(RowLast2, rng2Office.Column).Offset(1, 0).Select
Selection.PasteSpecial

Workbooks(file).Activate
Sheets("New Appeals").Select

'start from scratch again but with the next variable etc

RowLast = Cells(Rows.Count, rngARN.Column).End(xlUp).Row
Cells(RowLast - 1, rngARN.Column).Select
Range(Selection, Selection.End(xlUp)).Offset(1, 0).Copy
Workbooks("Book2.xlsm").Activate
Sheets("New Appeals").Select

RowLast2 = Cells(Rows.Count, rng2ARN.Column).End(xlUp).Row
Cells(RowLast2, rng2ARN.Column).Offset(1, 0).Select
Selection.PasteSpecial

Workbooks(file).Activate
Sheets("New Appeals").Select


End Sub

If this is inapropriate let me know and I'll delete it if needed!

Answer 1

I would consider the following:

• Macro description: The comments below the subroutine header should be concise and explain what the macro does, if it is not clear from its name. Your subroutine searches columns. You might want to include what is searched, i.e., "searches a predefined set of strings, selects [...] and copies from [...] to [...]. I would avoid details such as "this seemed to be the easiest method".
• Notation / Variable names: It is good practice to give consistent names to your variables. In VBA CamelCase is commonplace. Also, prepending the object type in the variable name is very common, i.e., strDoN as String. If you do so though, make sure you do it everywhere (so filepath should be strFilePath). See here for the naming conventions.
• Type declaration: Place all Dim statements at the beginning of the subroutine.
• Events: Be careful with enabling and disabling events. If you disable events they won't be re-enabled automatically, so in case of an error (exception) there should be additional actions that re-enable the events. See this post for details on error handling.
• As Chris Neilsen mentioned in the comments, avoid using Select and Activate.
• Proper Dim-ing: When you do Dim NumCols, i as Integer you actually do Dim NumCols as Variant, i as Integer. If you want both of them to be integers, use Dim Numcols as Integer, i as Integer. Also, prefer Long to Integer.
• Explicit Declarations: Put Option Explicit on top of your modules/worksheets. This means that every variable that is used should have been declared first (with a Dim statement). This is useful to avoid bugs from typos, and to have all your variables defined in a single place. Some of your variables, such as RowLast are not defined explicitly now, and they are of Variant type, while their type could had been more specific.
• Avoid hardcoding: It is good practice to not refer explicitly to whatever the user can change in a worksheet. Example: Sheets("New Appeals").Activate will work if the sheet name is New Appeals, but if the user changes the name, it will break. Use the sheet's ID instead. Similarly, in your code you assign string variables to hardcoded strings ("Date of Notification", etc). It is safer if you design an area in your sheet from where you can pull this data every time.
• Dealing with lots of Cases: the best solution is to use a Dictionary object. It is more elegant, and the code is less cluttered. An example is here.
• Copying and Pasting: Use the Range.Copy and Range.PasteSpecial Methods instead of the Selection ones. also, it is not always necessary to Activate a sheet in order to copy/paste there. The Range object can do useful stuff (searching, specialcells, etc.). Check it out.
• Fully qualify Ranges: When you copy-paste data from different sheets/files, always use the full name of your Range/Cells objects, to avoid bugs. Relevant SO post here.
• Dealing with Large Ranges: Passing data between Excel and VBA can be time-consuming when the numbers get bigger. A good practice is to pass the excel data to a Variant Array, do whatever processing and then dump them back to excel. This is a good read.
• Use of With blocks: When you refer to an object's properties/methods multiple times, it enhances readability. Example here.

I hope this helps. This is by not means an exhaustive list, but it might be useful to start with. Happy coding!