Copy data to another worksheet

Question

I have the following but it takes long time to run, it is a simple wherein the user select a file and data from Sheet1 is copied to another workbook.

Sub ImportApp(ByVal filepath_Report As String, file_name1 As String, wsOutput As Worksheet)
    Application.ScreenUpdating = False
    Set wbReport = Workbooks.Open(file_name1)
    Set wsReport = wbReport.Sheets(1)
    Select Case wsOutput.Name
        Case Is = "Downtilt Tracker"
            wsReport.Activate
            With Rows(1)
                Set d = .Find("Completed Date")
                If d Is Nothing Then
                    MsgBox "Invalid Down-Tilt Tracker." & vbNewLine & "Please import a valid Down-Tilt Tracker."
                    wbReport.Close False
                    Sheets("Control").Activate
                    Application.ScreenUpdating = True
                    Exit Sub
                End If
            End With
    End Select

    wsReport.Activate: wsReport.Cells.Copy: wsOutput.Activate: Cells(1, 1).Select: ActiveSheet.Paste: Cells.EntireColumn.AutoFit: wsOutput.Range("A1:AB" & 1048576).HorizontalAlignment = xlCenter
    wsOutput.Cells(1, 1).Select
    Application.CutCopyMode = False
    wbReport.Close False
End Sub

Define "a long time". 10 seconds? A few minutes? How large are the files that you're opening? — RubberDuck
– RubberDuck, Commented Mar 30, 2015 at 23:42
Just by chance, would the source workbook happen to be stored on a network drive? — RubberDuck
– RubberDuck, Commented Mar 30, 2015 at 23:47
Linked data sources then? Lots of formulas? I find it difficult to believe this code is running that long. Step through the code line by line and see which one seems to "freeze" it for a while. — RubberDuck
– RubberDuck, Commented Mar 30, 2015 at 23:55

Comintern · Accepted Answer · 2015-03-31 01:53:25Z

First things first - your code has some readability issues due to the use of the colon operator:

wsReport.Activate: wsReport.Cells.Copy: wsOutput.Activate: Cells(1, 1).Select: ActiveSheet.Paste: Cells.EntireColumn.AutoFit: wsOutput.Range("A1:AB" & 1048576).HorizontalAlignment = xlCenter

This makes it incredibly difficult to tell at a glance what the code is doing - reading down in a lot easier than reading left to right when you are trying to work through a sequence. This is much, much better:

wsReport.Activate
wsReport.Cells.Copy
wsOutput.Activate
Cells(1, 1).Select
ActiveSheet.Paste
Cells.EntireColumn.AutoFit
wsOutput.Range("A1:AB" & 1048576).HorizontalAlignment = xlCenter

Second, you have an unused variable in the Sub declaration - filepath_Report is never used. I'd remove it.

Speaking of your Sub declaration, it looks like the intent is to pass all of the parameters by value, but in fact only the first parameter is passed by value - ByRef is the default and each parameter needs the ByVal modifier to override the default. The code below demonstrates:

Private Sub ByValDemo()

   Dim one As String, two As String

   one = "Variable one set in ByValDemo"
   two = "Variable two set in ByValDemo"
   Call IsThisByVal(one, two)
   Debug.Print one
   Debug.Print two

End Sub

Private Sub IsThisByVal(ByVal one As String, two As String)

   one = "Variable one set in IsThisByVal"
   two = "Variable two set in IsThisByVal"

End Sub

Debug output of running ByValDemo is:

Variable one set in ByValDemo
Variable two set in IsThisByVal

To declare both strings as ByVal (the Worksheet has to be ByRef), you need to do this:

Sub ImportApp(ByVal filepath_Report As String, ByVal file_name1 As String, wsOutput As Worksheet)

The Select...Case structure is confusing as you only have one Case. A simple If...Then would suffice.
Although it probably isn't effecting anything, turning off screen updates and then only turning it back on if your conditional executes probably isn't the best way to handle it. Excel should turn it back on for you when the Sub exits, but I wouldn't rely on that.
Per your comment, it seems like you are under the impression that you aren't using the Windows Clipboard. This isn't accurate. The call to wsReport.Cells.Copy puts the entire Worksheet on the clipboard. The .Cells property contains every possible cell in the worksheet (even unused). You can confirm this by running the following, opening Notepad and hitting Ctrl-V:
```
Private Sub CanIHazClipboard()

    Dim sheet As Worksheet

    Set sheet = Application.ActiveSheet
    sheet.Cells.Copy
    'Prints 16384, regardless of what is being used.
    Debug.Print sheet.Cells.Columns.count
    'This would actually overflow the return variable...
    'Debug.Print sheet.Cells.Rows.Count

End Sub
```
The Application methods such as .Select, .Activate, .Copy, and .Paste functions are really slow compared to using the objects that you already got references to. Use the Range properties instead:
```
Dim target As Range
'Set the "paste" range to the same cells as the source (needs to be the same size).
Set target = wsOutput.Range(wsReport.UsedRange.Address)
'If you need formulas...
target.Formula = wsReport.UsedRange.Formula
'...or if you only need to copy values.
target.Value2 = wsReport.UsedRange.Value2
```
This is not only incredibly faster, it doesn't trash the clipboard and you don't need to keep track of what is "activated".

You have an Exit Sub buried deep in a nested structure. My personal rule is that if I'm going to bail out of a routine early, that shouldn't happen at more than one level of indentation. I would personally structure the flow control in this case so that it always reaches the bottom of the Sub so you Don't Repeat Yourself with your clean-up code (I also try to avoid the .Find method):

Dim found As Boolean, cell As Range

If Not wsOutput.Name = "Downtilt Tracker" Then
    'I would assume this should also be an error condition...
Else
    For Each cell In wsOutput.UsedRange.Rows(1).Cells
        If cell.Value2 = "Completed Date" Then
            found = True
            Exit For
        End If
    Next cell

    If found Then
        'Do your copy
    Else
        'Signal your error condition.
        MsgBox "Invalid Down-Tilt Tracker." & vbNewLine & _
               "Please import a valid Down-Tilt Tracker."
    End If
End If

'Only need the clean-up code once.
Application.ScreenUpdating = True
wbReport.Close False

Mathieu Guindon · Accepted Answer · 2015-03-31 01:25:50Z

I like your indentation. It's nice to see well-indented VBA code, it needs to be pointed out!

Application.ScreenUpdating = False

That's generally a good idea, but all by itself, it makes a poor UX - and if calculation mode remains xlAutomatic, it might be a missed opportunity to gain some more performance by turning off calculations while you're working on the worksheet.

Thing is, that's a whole concern all by itself, and as such it deserves its own function. Almost all my macro-enabled Excel workbooks have this function somewhere:

Private Sub ToggleWaitMode(Optional ByVal wait As Boolean = True)
    With Excel.Application

        .Calculation = IIf(wait, xlCalculationManual, xlCalculationAutomatic)
        .Cursor = IIf(wait, xlWait, xlDefault)
        .StatusBar = IIf(wait, "Please wait...", False)

        .DisplayAlerts = Not wait
        .ScreenUpdating = Not wait

    End With

End Sub

So instead of Application.ScreenUpdating = False, you can say ToggleWaitMode, and then do ToggleWaitMode False when you're done processing.

The problem with turning ScreenUpdating off, is that if anything goes wrong, it stays off and the user (and sometimes even the dev) is left thinking Excel is frozen: whenever you play with ScreenUpdating, you must handle runtime errors:

Private Sub DoSomething()
    On Error GoTo CleanFail

    'do something

    CleanExit:
        'clean up code
        ToggleWaitMode False
        Exit Sub
    CleanFail:
        'error-handling code
        Resume CleanExit
End Sub

This way you're always sure that all exit paths involve resetting ScreenUpdating and calculation mode.

...which is a good idea, because the next line can actually blow up whenever a bad file_name1 is passed to the procedure:

Set wbReport = Workbooks.Open(file_name1)

Set wsReport = wbReport.Sheets(1)

I like that: by assigning an object reference, you can work against that object. There are a few issues:

Where's the declaration? Always use Option Explicit and declare all variables! If the variable is declared at module level, then the declaration belongs inside the procedure's scope - move it there, to the smallest possible scope.
The Sheets collection contains chart sheets and actual worksheets. You probably intend to query the Worksheets collection here; if you had declared the wsReport variable As Worksheet and the Sheets(1) object were actually a chart sheet, you'd have a runtime error here.

Select Case wsOutput.Name
    Case Is = "Downtilt Tracker"

Several things here:

You're clearly using the wrong construct here - this should definitely be an If...Then block; what's wrong with this?
```
If wsOutput.Name = "Downtilt Tracker" Then
    '...
End If
```
I'm actually surprised Case Is = "string literal" actually compiles and works as intended; it's a pretty convoluted way of doing Case "string literal"...

And then things get a bit out of hand:

        wsReport.Activate
        With Rows(1)
            Set d = .Find("Completed Date")
            If d Is Nothing Then
                MsgBox "Invalid Down-Tilt Tracker." & vbNewLine & "Please import a valid Down-Tilt Tracker."
                wbReport.Close False
                Sheets("Control").Activate
                Application.ScreenUpdating = True
                Exit Sub
            End If
        End With

You have a reference to the wsReport object - you don't need to .Activate it.
The With block adds unnecessary indentation and confusion. I'd do wsReport.Rows(1).Find instead.
d is a meaningless name that doesn't tell anything about what you're doing here.
You need a reference to the "Control" sheet. Declare and assign another Worksheet variable, and use it instead of Selecting and Activateing and using implicit-context Cells.
If d isn't Nothing, you don't appear to be setting ScreenUpdating back to True. The control flow described in my introduction would solve that.

Avoid this - AT ALL COSTS:

wsReport.Activate: wsReport.Cells.Copy: wsOutput.Activate: Cells(1, 1).Select: ActiveSheet.Paste: Cells.EntireColumn.AutoFit: wsOutput.Range("A1:AB" & 1048576).HorizontalAlignment = xlCenter

There are 7 instructions on that line of code; there's no reason to do this. Ever.

And here's where I'd think your bottleneck is:

wsReport.Cells.Copy

You're copying the entire worksheet.

wsOutput.Range("A1:AB" & 1048576)

Why bother with the concatenation here? It's hard-coded anyway, and besides, A1 has its row specified inside the string literal.

wsOutput.Range("A1:AB1048576")

This might work a bit better - if not, it's at least certainly much easier to read:

wsReport.UsedRange.Copy
wsOutput.Range("A1").Paste
wsOutput.UsedRange.Columns.EntireColumn.AutoFit
wsOutput.UsedRange.HoriontalAlignment = xlCenter

I totally missed the Is = "String" syntax. I actually had to test it to see if it worked (which it does). Then I remembered that Is really isn't an operator like VB.NET - it's freaky VBA syntax for boolean Case conditionals. VBA inserts it automatically if you put an = after Case. I'm guessing it's to support the equally weird Select Case True syntax (that I shamefully admit to using sometimes). — Comintern
– Comintern, Commented Mar 31, 2015 at 2:14
It blew my mind too @Comintern. The examples I saw when I looked it up used it to find ranges (that would be better as ifs IMO). Case Is < 100 and such. — RubberDuck
– RubberDuck, Commented Mar 31, 2015 at 8:49
Thanks a lot I was successfully able to bring down the run time from 3 mins to 1 min... — Anurag Singh
– Anurag Singh, Commented Mar 31, 2015 at 15:50

Stack Exchange Network

Copy data to another worksheet

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Copy data to another worksheet

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions