4
\$\begingroup\$

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
\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ 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 \$\endgroup\$ – ThunderFrame Jan 26 '17 at 19:10
  • \$\begingroup\$ @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. \$\endgroup\$ – Victor Moraes Jan 26 '17 at 19:19
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – ThunderFrame Jan 26 '17 at 19:23
4
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ Rezaul, did this work for you? \$\endgroup\$ – Rdster Nov 21 '16 at 15:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.