VBA copy paste loop

Question

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?

A_File

Practice_New

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

Answer 1

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

2. 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.

3. 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.

4. 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.

5. 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.

6. 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.

7. 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).Range(Source).Copy
wbk.Worksheets("Sheet1").Range(Target1, Target2).PasteSpecial _
 Paste:=xlPasteValues, _
 Operation:=xlNone, _
 SkipBlanks:=False, _
 Transpose:=False

8. Remember to restore the settings that you changed

On one of the first lines of your code you have:

Application.DisplayAlerts = False

But you've never restored this setting that you change, which means that after running your code you (or the user) probably won't see any other Alert in your worksheet until you close the workbook and open it again, so right before the End Sub you should have:

Application.DisplayAlerts = True

This reactivates the alerts, so you (or the user) won't miss anything

I hope it helps.

• ThunderFrame: Note that using With ThisWorkbook.Worksheets("Sheet1") does make for more readable code, but the Worksheets("Sheet1") part is equivalent to Sheets.Item("Sheet1"), which returns an Object, so all calls to the With block variable are late-bound (that's why there's no Intellisense). It would be better to use the sheet's (meaningful) code name as With userSettings or to declare and set a Worksheet variable, and then use that in a With block: Dim currentUserSettings As Worksheet: Set currentUserSettings = ThisWorkbook.Worksheets("Sheet1") and then With currentUserSettings
• Victor Moraes: @ThunderFrame Very good points raised, but then I have a question: in OP's code, is it worth to declare new Worksheet variables just for that single statement in both wbkS.Worksheets(SourceTabName) and wbk.Worksheets("Sheet1") worksheets? I think in that case I would go with the sheet's (meaningful) code name approach and probably wouldn't bother declaring a new Worksheet variable just for that.
• ThunderFrame: From a performance perspective, a single call is not worth it, but a single late-bound call probably is. IMO, it is worth doing for the Intellisense, but as you say, if you use the sheet's code name, you get Intellisense, avoid the need for a variable declaration and assignment, and avoid the need for a With block.

Answer 2

If you are opening and closing the same file every time, just move it outside of the loop

Set wbkS = Workbooks.Open(strFirstFile)
Set wbk = Workbooks.Open(strSecondFile)
For x = 2 To lr
 Source = sh1.Range("C" & x).Value
 Target1 = sh1.Range("E" & x).Value
 Target2 = sh1.Range("F" & x).Value
 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
Next
wbk.Save
wbk.Close
wbkS.Close

• Rdster: Rezaul, did this work for you?