HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

VBA macro to move data across workbooks

Submitted by: @import:stackexchange-codereview··
0
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.

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 Sub


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.

Solution

It's a nitpicky thing, but I prefer explicit scoping.

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.Value


Would be better written as

Dim fileName As String
fileName = Worksheets("Cover").Range("B5").Value


Note 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 = ThisWorkbook


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.

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 rst


I 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.Value
Dim fileName As String
fileName = Worksheets("Cover").Range("B5").Value
Dim currentWB As Workbook
Set currentWB = ThisWorkbook

Context

StackExchange Code Review Q#94667, answer score: 6

Revisions (0)

No revisions yet.