Skip to main content
Updated based on feedback.
Source Link
AJD
  • 2.4k
  • 1
  • 10
  • 18

Yes, there is a way to do this better.

Always Option Explicit.

No need (almost always) for .Select, this is a human activity and explicitly referencing the ranges in VBA is more efficient. This also creates extra work for the VBA engine.

You hide your active window, but you do not unhide it.

Don't be afraid to use you favourite search engine to look up even the simplest commands, especially if you are fixing up code generated by the macro recorder. Sometimes these commands have methods and properties that make life easier (e.g. see what I have done with the copy method - look up the "Excel Range.Copy" method to see what gems are there).

However, with the amount of code you have here, I am really surprised that you would notice any slowing down!

Sub CopyData()
    Dim mainWB  As Workbook
    Dim mainWS  As Worksheet
    Set mainWB = ActiveWorkbook
    Set mainWS = mainWB.Sheets(1)

    Dim wbSource As Workbook
    Set wbSource = Workbooks.Open(Filename:="H:\Formatting.xlsx")
    'ActiveWindow.Visible = False ' Why?

    wbSource.Sheets(1).Range("A10:CX15").Copy mainWS.Range("A10:CX15")
    wbSource.Sheets(1).Range("A25:M26").Copy mainWS.Range("A1") 'Haven't tested this line to see what effect the different range size has.
    wbSource.Sheets(1).Range("D49").Copy mainWS.Range("AF8")
    wbSource.Sheets(1).Range("D51").Copy mainWS.Range("AF10")
    wbSource.Close
End Sub

And the usual staples of setting Application.ScreenUpdating, Application.EnableEvents, Application.Calculation will assist in managing what happens or is shown when the routine is run. ActiveWindow.Visible could be handy, but remember to turn it back on after doing the work. Your ActiveWindow may not be what you think it is.

Yes, there is a way to do this better.

Always Option Explicit.

No need (almost always) for .Select, this is a human activity and explicitly referencing the ranges in VBA is more efficient. This also creates extra work for the VBA engine.

You hide your active window, but you do not unhide it.

Don't be afraid to use you favourite search engine to look up even the simplest commands, especially if you are fixing up code generated by the macro recorder. Sometimes these commands have methods and properties that make life easier (e.g. see what I have done with the copy method - look up the "Excel Range.Copy" method to see what gems are there).

However, with the amount of code you have here, I am really surprised that you would notice any slowing down!

Sub CopyData()
    Dim mainWB  As Workbook
    Dim mainWS  As Worksheet
    Set mainWB = ActiveWorkbook
    Set mainWS = mainWB.Sheets(1)

    Dim wbSource As Workbook
    Set wbSource = Workbooks.Open(Filename:="H:\Formatting.xlsx")
    'ActiveWindow.Visible = False ' Why?

    wbSource.Sheets(1).Range("A10:CX15").Copy mainWS.Range("A10:CX15")
    wbSource.Sheets(1).Range("A25:M26").Copy mainWS.Range("A1") 'Haven't tested this line to see what effect the different range size has.
    wbSource.Sheets(1).Range("D49").Copy mainWS.Range("AF8")
    wbSource.Sheets(1).Range("D51").Copy mainWS.Range("AF10")
    wbSource.Close
End Sub

Yes, there is a way to do this better.

Always Option Explicit.

No need (almost always) for .Select, this is a human activity and explicitly referencing the ranges in VBA is more efficient. This also creates extra work for the VBA engine.

You hide your active window, but you do not unhide it.

Don't be afraid to use you favourite search engine to look up even the simplest commands, especially if you are fixing up code generated by the macro recorder. Sometimes these commands have methods and properties that make life easier (e.g. see what I have done with the copy method - look up the "Excel Range.Copy" method to see what gems are there).

However, with the amount of code you have here, I am really surprised that you would notice any slowing down!

Sub CopyData()
    Dim mainWB  As Workbook
    Dim mainWS  As Worksheet
    Set mainWB = ActiveWorkbook
    Set mainWS = mainWB.Sheets(1)

    Dim wbSource As Workbook
    Set wbSource = Workbooks.Open(Filename:="H:\Formatting.xlsx")
    'ActiveWindow.Visible = False ' Why?

    wbSource.Sheets(1).Range("A10:CX15").Copy mainWS.Range("A10:CX15")
    wbSource.Sheets(1).Range("A25:M26").Copy mainWS.Range("A1") 'Haven't tested this line to see what effect the different range size has.
    wbSource.Sheets(1).Range("D49").Copy mainWS.Range("AF8")
    wbSource.Sheets(1).Range("D51").Copy mainWS.Range("AF10")
    wbSource.Close
End Sub

And the usual staples of setting Application.ScreenUpdating, Application.EnableEvents, Application.Calculation will assist in managing what happens or is shown when the routine is run. ActiveWindow.Visible could be handy, but remember to turn it back on after doing the work. Your ActiveWindow may not be what you think it is.

Source Link
AJD
  • 2.4k
  • 1
  • 10
  • 18

Yes, there is a way to do this better.

Always Option Explicit.

No need (almost always) for .Select, this is a human activity and explicitly referencing the ranges in VBA is more efficient. This also creates extra work for the VBA engine.

You hide your active window, but you do not unhide it.

Don't be afraid to use you favourite search engine to look up even the simplest commands, especially if you are fixing up code generated by the macro recorder. Sometimes these commands have methods and properties that make life easier (e.g. see what I have done with the copy method - look up the "Excel Range.Copy" method to see what gems are there).

However, with the amount of code you have here, I am really surprised that you would notice any slowing down!

Sub CopyData()
    Dim mainWB  As Workbook
    Dim mainWS  As Worksheet
    Set mainWB = ActiveWorkbook
    Set mainWS = mainWB.Sheets(1)

    Dim wbSource As Workbook
    Set wbSource = Workbooks.Open(Filename:="H:\Formatting.xlsx")
    'ActiveWindow.Visible = False ' Why?

    wbSource.Sheets(1).Range("A10:CX15").Copy mainWS.Range("A10:CX15")
    wbSource.Sheets(1).Range("A25:M26").Copy mainWS.Range("A1") 'Haven't tested this line to see what effect the different range size has.
    wbSource.Sheets(1).Range("D49").Copy mainWS.Range("AF8")
    wbSource.Sheets(1).Range("D51").Copy mainWS.Range("AF10")
    wbSource.Close
End Sub