Improve Macro Efficiency

Question

Macro Improvement| Hello This is my first post on this site,I love the community here I am rookie in macros but I have tried my best to create one functioning macro, I would like to hear opinion of professionals where I could improve my macro, mainly efficiency of it. The task I am trying to perform with this macro is to Open Workbook based on cells in my MainB workbook, then compare 3 strings in these two workbooks and if they match copy and paste them to original file, close the previous and continue. The error I have right now is after the macro encounters the non-existent file location it closes main workbook and does not continue. If by any chance it continues then it gives me an error message, which it shouldn't as I have specified what to do 'OnError'.

 Sub DoCopyandRepeat()

Dim MainB As Workbook
Dim CopyB As Workbook
Dim wsM As Worksheet
Dim wsC As Worksheet
Dim A, B, C, D, E, F, G, H As Variant
Dim X As Integer

Set MainB = ThisWorkbook

Set wsM = MainB.Worksheets("Sheet1")

AfterError:

For X = 3 To 10 Step 1

If Cells(X, 23).Value = "" Then
Workbooks.Open Filename:="C:\Users\XY\OneDrive - XX\Desktop\Macro book"

Set MainB = ThisWorkbook
Set wsM = MainB.Worksheets("Sheet1")
MainB.Activate

Workbooks.Open Filename:="C:\Users\XY\OneDrive - XX\Desktop\Folder1\Folder2\" & Worksheets("Sheet1").Cells(X, 5) & "\Folder3\" & Worksheets("Sheet1").Cells(X, 12) & "\" & Worksheets("Sheet1").Cells(X, 14)
    On Error GoTo Reset:

    End If
    
Set CopyB = ActiveWorkbook
Set wsC = CopyB.ActiveSheet

wsC.Range("E4").Copy
wsM.Activate
Range("AE2").PasteSpecial xlPasteValues, xlPasteSpecialOperationNone, True, False

wsC.Range("C4").Copy
wsM.Activate
Range("AF2").PasteSpecial xlPasteValues, xlPasteSpecialOperationNone, True, False

wsC.Range("E6").Copy
wsM.Activate
Range("AG2").PasteSpecial xlPasteValues, xlPasteSpecialOperationNone, True, False

wsC.Range("E5").Copy
wsM.Activate
Range("AH2").PasteSpecial xlPasteValues, xlPasteSpecialOperationNone, True, False
    
A = Range("AE2")
B = Cells(X, 15)
ActiveSheet.Range("AE3") = StrComp(A, B, vbTextCompare)

C = Range("AF2")
D = Cells(X, 12)
ActiveSheet.Range("AF3") = StrComp(C, D, vbTextCompare)

E = Range("AG2")
F = Cells(X, 18)
ActiveSheet.Range("AG3") = StrComp(E, F, vbTextCompare)

G = Range("AH2")
H = Cells(X, 15)
ActiveSheet.Range("AG3") = StrComp(E, F, vbTextCompare)

If Cells(3, 31) = 0 And Cells(3, 32) = 0 And Cells(3, 33) = 0 Then
    CopyB.Activate
    Range("G4:G10").Copy
    MainB.Activate
    Cells(X, 23).PasteSpecial xlPasteValues, xlPasteSpecialOperationNone, Transpose:=True
    CopyB.Close
    
ElseIf Cells(3, 32) = 0 And Cells(3, 33) = 0 And Cells(3, 34) = 0 Then

    CopyB.Activate
    Range("G6:G10").Copy
    MainB.Activate

    CopyB.Activate
    Range("G5").Copy
    MainB.Activate
    Cells(X, 23).PasteSpecial xlPasteValues, xlPasteSpecialOperationNone
    
    CopyB.Activate
    Range("G4").Copy
    MainB.Activate
    Cells(X, 24).PasteSpecial xlPasteValues, xlPasteSpecialOperationNone
    CopyB.Close
    
Else
    Cells(X, 23) = "failure"

CopyB.Close

End If

ActiveWorkbook.Save
Application.Wait (Now + TimeValue("0:00:05"))

Reset:

Next X
Resume AfterError

End Sub

Answer 1

The On Error issue

Your On Error GoTo line should be before the code you want to handle.

If you step through your code using F8 in the VBE, if the workbook you want to open doesn't exist for example, the code has executed before your On Error handler, hence you are receiving an error on screen.

To avoid the error appearing on screen and for your code to perform as expected, try like this;

...
Set MainB = ThisWorkbook
Set wsM = MainB.Worksheets("Sheet1")
MainB.Activate

On Error GoTo Reset

Workbooks.Open Filename:="C:\Users\XY\OneDrive - XX\Desktop\Folder1\Folder2\" & Worksheets("Sheet1").Cells(X, 5) & "\Folder3\" & Worksheets("Sheet1").Cells(X, 12) & "\" & Worksheets("Sheet1").Cells(X, 14)

End If
...

This way, if you step through the code you would see the On Error code is executed the line before your Workbooks.Open line and thus if an error is thrown, the code now knows to goto Reset

As a simple example, the following sub has an error handler and tries to divide by zero (which you cannot do!).

Sub foo()

Debug.Print 1 / 0
On Error GoTo Safety:

Exit Sub
Safety:
Debug.Print "Safety!"
End Sub

This example throws an error;

Run time error '11' Division by zero

Now if we move the Error handler above the 1/0 line,

Sub foo()

On Error GoTo Safety:

Debug.Print 1 / 0

Exit Sub
Safety:
Debug.Print "Safety!"
End Sub

This example with output Safety! to the Immediate window in the VBE.

---

As for a review of your code for improvements etc, this question would be better suited for another Stack Exchange site: Code Review.