Copy data to another worksheet

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

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

StackExchange Code Review Q#85450, answer score: 4