patternMinor
VBA macro to move data across workbooks
Viewed 0 times
workbooksmovemacroacrossdatavba
Problem
I'm new to VBA and am trying to write a macro to move data from an exported file to an excel template. The code below works, however I have to add many more ranges of data and am worried it will be hefty to run.
I took the base code from here(the closed section) and modified it to fit my needs. The file I am extracting data from will change, but will always be in the same folder, which is why I set up the path as such. I am moving the data from C2 and downwards to C7-down on the template. C56 is currently the last used cell on the template, however that is subject to change so I would prefer to get rid of the lower bound.
If there is a better way to write this, I would love some input. Since I'm still new at this, if you could explain your code as well, that would be very beneficial.
Sub MoveData()
Dim fileName As String
Sheets("Cover").Select '
range("B5").Select
fileName = Selection.Value
Dim path As String
path = "C:\Users\(name)\Documents\(folder)\" & fileName & ".csv"
Dim currentWB As Workbook
Set currentWB = ThisWorkbook
Dim openWB As Workbook
Set openWB = Workbooks.Open(path)
Dim openWs As Worksheet
Set openWs = openWB.Sheets(fileName)
openWs.range("C2:C51").Copy
openWB.Close (False)
currentWB.Worksheets("Bms").Activate
range("C7:C56").Select
ActiveSheet.Paste
End SubI took the base code from here(the closed section) and modified it to fit my needs. The file I am extracting data from will change, but will always be in the same folder, which is why I set up the path as such. I am moving the data from C2 and downwards to C7-down on the template. C56 is currently the last used cell on the template, however that is subject to change so I would prefer to get rid of the lower bound.
If there is a better way to write this, I would love some input. Since I'm still new at this, if you could explain your code as well, that would be very beneficial.
Solution
It's a nitpicky thing, but I prefer explicit scoping.
Should be
There are lots of reasons to avoid Activating and Selecting objects. Performance is just one of them.
Would be better written as
Note that I used the
Good call and good naming. Well done.
It's very slow to open a workbook. Luckily, you might not even have to open the book with the source data. I would look into querying the Excel data source with ADODB instead. Once you have a recordset, you can use the CopyFromRecordset method of Range to paste into the current workbook. It's much faster than loading the data source into Excel.
Here's some untested code to get you started.
I don't think you understand what parenthesis do in procedure calls.
There's no harm in passing a boolean literal ByVal in this case, but make sure you save yourself the headache down the road and go read that StackOverflow Q&A I linked to. Parenthesis change how the argument is passed to a procedure.
Sub MoveData()Should be
Public Sub MoveData()There are lots of reasons to avoid Activating and Selecting objects. Performance is just one of them.
Dim fileName As String
Sheets("Cover").Select '
range("B5").Select
fileName = Selection.ValueWould be better written as
Dim fileName As String
fileName = Worksheets("Cover").Range("B5").ValueNote that I used the
Worksheets collection instead of Sheets. This is because Sheets can have charts, Worksheets can't. Dim currentWB As Workbook
Set currentWB = ThisWorkbookGood call and good naming. Well done.
It's very slow to open a workbook. Luckily, you might not even have to open the book with the source data. I would look into querying the Excel data source with ADODB instead. Once you have a recordset, you can use the CopyFromRecordset method of Range to paste into the current workbook. It's much faster than loading the data source into Excel.
Here's some untested code to get you started.
Dim cn as ADODB.Connection
Set cn = New ADODB.Connection
With cn
.Provider = "Microsoft.Jet.OLEDB.4.0"
.ConnectionString = "Data Source=" & path & ";" & _
"Extended Properties=Excel 14.0;"
.Open
End With
strQuery = "SELECT * FROM [Sheet1$C2:C51]"
Dim rst As New ADODB.Recordset
rst.Open sqlQuery, cn
currentWB.Worksheets("Bms").Range("C7").CopyFromRecordset rstI don't think you understand what parenthesis do in procedure calls.
openWB.Close (False)There's no harm in passing a boolean literal ByVal in this case, but make sure you save yourself the headache down the road and go read that StackOverflow Q&A I linked to. Parenthesis change how the argument is passed to a procedure.
Code Snippets
Sub MoveData()Public Sub MoveData()Dim fileName As String
Sheets("Cover").Select '
range("B5").Select
fileName = Selection.ValueDim fileName As String
fileName = Worksheets("Cover").Range("B5").ValueDim currentWB As Workbook
Set currentWB = ThisWorkbookContext
StackExchange Code Review Q#94667, answer score: 6
Revisions (0)
No revisions yet.