VBA copy paste loop

I have 2 Excel files: the first is the source file "Practice_New.xlsx" and the second is a mapping file "A_File.xlsx". A_File is a mapping file which contains cell reference of the source file ("Practice_New.xlsx") to the target file (I need to create this file, say "Practice_New_Output.xlsx").

I have written this code to achieve that but it's taking huge time to complete. Data volume in the source Excel is more than 500 rows sometime. The main issue in my code is that it's opening and closing same file every time in the loop and that is why it's taking huge time. I am not very good in VBA coding. Can anyone please help me to tune up this code to perform better?

Sub COPYCELL()

Dim wbk As Workbook

Dim x%

Application.DisplayAlerts = False

strParamFile = "C:\Users\rezaul.hasan\Desktop\Practice\A_FILE.xlsx" 

Workbooks.Open Filename:="C:\ Important\A_FILE.xlsx"

Sheets("Sheet1").Select

TargetFilename = Range("G2").Value

SourceFilename = Range("A2").Value

SourceTabName = Range("B2").Value

Set wbkt = Workbooks.Add

wbkt.SaveAs Filename:=" C:\ Important \" & TargetFilename & ".xlsx", FileFormat:=51

wbkt.Close

strFirstFile = " C:\ Important \" & SourceFilename & ".xlsx" 'Take the source excel

strSecondFile = " C:\ Important \" & TargetFilename & ".xlsx" 'take the target excel

Set wbkM = Workbooks.Open(strParamFile)

Set sh1 = Sheets("Sheet1")

lr = Range("C" & Rows.Count).End(xlUp).Row

For x = 2 To lr

Source = sh1.Range("C" & x).Value

Target1 = sh1.Range("E" & x).Value

Target2 = sh1.Range("F" & x).Value

Set wbkS = Workbooks.Open(strFirstFile)

With wbkS.Sheets(SourceTabName)

   .Range(Source).Copy

End With

Set wbk = Workbooks.Open(strSecondFile)

With wbk.Sheets("Sheet1")

.Range(Target1, Target2).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks:=False, Transpose:=False

End With

wbk.Save

wbk.Close

wbkS.Close

Next

wbkM.Close

End Sub

Rdster's answer already helped you out with the performance and Open/Closing the workbooks issues over and over again, but since you stated you are new to VBA, here are some more tips to improve your code:

1. Always Declare your variables

VBA (as long as other VB languages) does not force you to declare your variables by default, but this is always a good practice and help you prevent typos in your code. To force the variables declaration, always use Option Explicit on top of your modules. This will make all variable declarations mandatory. You can make VBE to automatically add this line for you in your modules in by ticking the option "Require Variable Declaration" in Tools -> Options.

Personally I don't like using Identifier type characters when declaring my variables. I find it much clearer to read Dim x As Integer instead of Dim x%, but that's a matter of taste

1. Use proper reference to the Ranges and Cells

When you use Range("A1").Value in your code, VBA uses the global reference and works with whatever Worksheet is active at the moment of the code execution. So, suppose I have Sheet1 running my code and all of a sudden I decide to open Sheet2, VBA will get the value of "A1" in Sheet2 instead of Sheet1 and this can cause a mess in your code.

When you use ThisWorkbook.Worksheets("Sheet1").Range("A1").Value instead, you can activate whatever Worksheet you want and VBA will still get the right value.

1. Avoid using .Select

Whenever possible, it is recommended to avoid using .Select. Long story short: it slows down your code and usually there are better ways to perform the desired action without it. Here is a very helpful question regarding this topic.

So, this:

Workbooks.Open Filename:="C:\ Important\A_FILE.xlsx"

Sheets("Sheet1").Select

TargetFilename = Range("G2").Value

SourceFilename = Range("A2").Value

SourceTabName = Range("B2").Value

Would be better if coded with something like this:

Dim wbkA as Workbook
Set wbkA = Workbooks.Open("C:\Important\A_FILE.xlsx")
With wbkA.Worksheets("Sheet1")
    TargetFileName = .Range("G2").Value
    SourceFileName = .Range("A2").Value
    SourceTabName = .Range("B2").Value
End With

As you can see, I have declared the variable with the proper variable type, and instead of using Select I am, instead, giving proper reference to the Ranges I need to work with.

1. Prefer working with Worksheets instead of Sheets

The Sheets object is a parent object for Worksheets and Chart Sheets. If a workbook has 3 worksheets and 1 chart sheet, in VBA:

• Sheets.Count will include both types. 4

• Worksheets.Count will include only worksheets. 3

This is usually not a problem when you have only Worksheets in your Workbook, but the time will come when you have a Chart Sheet, so it is better to be prepared.

1. Remember to indent your code

To help your future you or to help a colleague to understand your code, it is much easier to read this:

Sub example()
    For x=0 to 10
        If x=1 then
            x=x+1 
        ElseIf x=2 Then
            x=x*1
        End If
    Next
End Sub

Than this:

Sub example()
For x=0 to 10
If x=1 then
x=x+1 
ElseIf x=2 Then
x=x*1
End If
Next
End Sub

Of course this is a very simple example, but you get the point.

1. Use meaningful variable names

Again, this will benefit your future you and anyone else that tries to read your code. Variables named like lr are not really helpful and should be renamed as something like lastRow. Right now, you know what your code do and so it is easy for you to recall the variable names and purposes but in a couple of years or even months when you revisit your code, you will probably have to lose some time trying to understand what the code is doing and one of the reasons is because of poor naming choices.

1. Take advantage of With blocks

With blocks can be very helpful and improve the readability of your code. This is just an example where the With block is welcome:

With ThisWorkbook.Worksheets("Sheet1")
    .Cells(1, 1).Value = 3
    .Cells(1, 2).Value = "Something"
    .Cells(1, 3).Value = 2
    .Cells(1, 4).Value = "Another thing"
    .Cells(1, 5).Value = 1

    With .Range("A1:I1").Borders
        .LineStyle = xlContinuous
        .Weight = xlThin
    End With
End With

As you can see, I'm avoiding calling ThisWorkbook.Worksheets("Sheet1") over and over again by using the With block.

On the other hand, there are some times where they are really unnecessary, such as when you only need to reference it once. In your code you have this:

With wbkS.Sheets(SourceTabName)

   .Range(Source).Copy

End With

With wbk.Sheets("Sheet1")

.Range(Target1, Target2).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks:=False, Transpose:=False

End With

But you could simply have this (note the use of _(line continuation), also for readability):

```
wbkS.Worksheets(SourceTabName)

StackExchange Code Review Q#147376, answer score: 7