1

I am working on automating an excel model by copying data from other sheets into a masterfile. I have a bit of an issue that after adding the code the file went from 25mb to 60mb, without changing the content, only adding the code. Below you can find a snippet of how I automated the imports

Sub copytest() 'Procedure for retrieving data from the sourcefiles

    Dim wbTarget, wbSource As Workbook
    Dim target As Object
    Dim pathSource, fileName As String
    Dim xlApp As Application
    Dim lastRow As Long

    Application.EnableEvents = False
    Application.ScreenUpdating = False

    'path where the data source folders are located (please keep all of them in the same directory)
    pathSource = "C:\Users\vferraz\Desktop\crm stock\RAPOARTE IMPORTANTE\18.02\Rapoarte pentru Handsets\"
    Set wbTarget = ThisWorkbook

    Set xlApp = CreateObject("Excel.Application")
    xlApp.DisplayAlerts = False
    Application.CutCopyMode = False

    'Stock 0001
    Set wbSource = xlApp.Workbooks.Open(pathSource & "Stoc 0001.xls")
    wbSource.Sheets(1).UsedRange.Copy
    wbSource.Close
    Set target = wbTarget.Sheets("Stock 0001")
    target.UsedRange.Clear
    Range("A1").Select
    target.Paste

    xlApp.Quit
    Set wbSource = Nothing
    Set xlApp = Nothing

    ThisWorkbook.Sheets("Mastersheet").Activate

    Application.EnableEvents = True
    Application.ScreenUpdating = True

End Sub

In the snippet above I only added the parsing of one file (Stock 0001), but the same method is done for other 10-15 files.

Does anyone have any ideas to improve the efficiency/size of this file based on this procedure?

P.S. Im aware that the "Paste" method might be adding formats rather than values only, then I tried adding .PasteSpecial xlPasteValues instead of paste but it eventually throw errors that I couldn't identify

Update:

Based on this solution, this is the new version I tried:

Stock 0001
    Set wbSource = xlApp.Workbooks.Open(pathSource & "Stoc 0001.xls")
    lastRow = wbSource.Sheets(1).Cells.Find("*", SearchOrder:=xlByRows, SearchDirection:=xlPrevious).Row
    wbTarget.Sheets("Stock 0001").Cells.Clear
    wbSource.Sheets(1).Range("A1:C" & lastRow).Copy Destination:=wbTarget.Sheets("Stock 0001").Range("A1")
    wbSource.Clo

The line wbSource.Sheets(1).Range("A1:C" & lastRow).Copy Destination:=wbTarget.Sheets("Stock 0001").Range("A1" Throws the "copy method of range class failed error.

vferraz
  • 441
  • 4
  • 17
  • 3
    ***P.S. Im aware that the "Paste" method might be adding formats rather than values only, then I tried adding .PasteSpecial xlPasteValues instead of paste but it eventually throw errors that I couldn't identify*** So, that's that? You ask us to trim your code although you know a way of doing it but gave up? –  Mar 13 '19 at 14:30
  • 4
    Efficiency wise, there is a very good article on why you shouldn't be using `Select`, `Active` etc https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba/23913882#23913882 – Badja Mar 13 '19 at 14:31
  • I did not gave up, still looking for a solution here. I read about this method and found out it is not appropriate for this case. I dont want it to be trimmed, I wanted a suggestion of something similar to .PasteSpecial xlPasteValues which would work in this case based on somebody's previous experience – vferraz Mar 13 '19 at 14:33
  • 3
    Record a macro to see how to do pastespecial. Also avoid the use of `UsedRange`. Find the last row and the last column and then identify the range you want to copy – Siddharth Rout Mar 13 '19 at 14:33

3 Answers3

2

Instead of this

'Stock 0001
Set wbSource = xlApp.Workbooks.Open(pathSource & "Stoc 0001.xls")
wbSource.Sheets(1).UsedRange.Copy
wbSource.Close
Set target = wbTarget.Sheets("Stock 0001")
target.UsedRange.Clear
Range("A1").Select
target.Paste

Try this

wbSource.Sheets(1).Columns("").Copy Destination:=wbTarget.Sheets("Stock 0001").Range("A1")

Where I've put Columns just replace this with whatever range you are using via Range() or Cells etc Copy and Paste takes a while, and has issues if you are already copying something in another location. This just takes the data for you

Also, this piece of code will be your friend forever

With Sheets("Sheet1")
    LastRow = .Range("A" & .Rows.Count).End(xlUp).Row
End With

This finds the bottom row of Column A (or whatever your "always populated" column will be

Sub LastRow()

    Dim wb As Workbook, ws As Worksheet, LastRow As Long

    Set wb = ThisWorkbook
    Set ws = Worksheets("Data")

    LastRow = ws.Cells(ws.Rows.Count, 1).End(xlUp).Row

    With ws.Range(ws.Cells(2, 13), ws.Cells(LastRow, 13))
        'This is Range M2:M(bottom)
        .
        .
        'etc
        .
    End With

End Sub

Edit....3:

Set xlApp = CreateObject("Excel.Application")
xlApp.DisplayAlerts = False
Application.CutCopyMode = False

'Stock 0001
Set wbSource = xlApp.Workbooks.Open(pathSource & "Stock 0001.xls")

Instead of all this, please use

Set wbSource = Workbooks.Open(pathSource & "Stock 0001.xls")
Badja
  • 857
  • 1
  • 8
  • 33
  • 1
    `Columns` and `Range` are members of the `Worksheet` class; you have them invoked against `Workbook` objects; that will throw error 438 at run-time, if it compiles at all. The idea is very sound and upvote-worthy though, care to edit? – Mathieu Guindon Mar 13 '19 at 14:41
  • Thank you very much! Actually, by doing that and adding the command `wbSource.Sheets(1).Range("A1:C" & lastRow).Copy Destination:=wbTarget.Sheets("Stock 0001").Range("A1")`, I still get the error "copy method of range class failed" – vferraz Mar 13 '19 at 15:00
  • Something else is going on then - as that really only occurs when the range is too big for the destination - which they are equal in this case. Post your new code at the bottom of your post as an edit and I'll have a look – Badja Mar 13 '19 at 15:05
  • 1
    Code added to the initial post – vferraz Mar 13 '19 at 15:11
  • Have a look at the "Edit 3 " on my reply - there should be nothing wrong now – Badja Mar 13 '19 at 15:12
  • This is actually how I had it before, the `xlApp.Workbooks.Open` command is a way I found around opening 15 excels on the user screen. – vferraz Mar 13 '19 at 15:16
  • Aah. I believe then I may be out of ideas. Hopefully someone else could add a comment helping some more. Apologies. – Badja Mar 13 '19 at 15:25
1

You also need error handling in your code. When it breaks (file doesn't exist, path is invalid, sheet doesn't exist) between

Application.EnableEvents = False
Application.ScreenUpdating = False

and

Application.EnableEvents = True
Application.ScreenUpdating = True

you're going to end up with Excel in a bad state where screen updating is off and events will no longer fire. What you should have is something long the lines of

On Error GoTo ExitErr
    Application.EnableEvents = False
    Application.ScreenUpdating = False

Then after your code, you should have

ExitErr:
    Application.EnableEvents = True
    Application.ScreenUpdating = True
Frank Ball
  • 1,039
  • 8
  • 15
0

I found a way to reduce the file size back to how it used to be by adding the following line to the imports after the paste command

target.Cells.ClearFormats

In this case the formats taken from the data were cleared.

vferraz
  • 441
  • 4
  • 17