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

• RubberDuck: Define "a long time". 10 seconds? A few minutes? How large are the files that you're opening?
• Anurag Singh: 3 Min, the file size is about 3Mbps with formatting
• RubberDuck: Just by chance, would the source workbook happen to be stored on a network drive?
• Anurag Singh: no on local drive.
• RubberDuck: 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.

Answer 1

• 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
  

Answer 2

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

• Comintern: 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).
• RubberDuck: 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.
• Anurag Singh: Thanks a lot I was successfully able to bring down the run time from 3 mins to 1 min...