current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite

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.

share|improve this question
up vote 6 down vote accepted

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.

share|improve this answer
    
Could you elaborate on the idea of using ADO? I've looked through your link and did some googling but it seems a bit above my head. – Aei Jun 25 '15 at 19:56
1  
@Aei I added some untested code that should help you get started. – RubberDuck Jun 25 '15 at 20:15
2  
I'd recommend using a .csv specific connection string rather than specifying Excel. I haven't formally benchmarked them, but the first time I switched from Jet/Excel to Jet/Text it was a big enough improvement that I've never looked back. – Comintern Jun 26 '15 at 1:52
    
@RubberDuck I'm having trouble getting rid of an error in the With block. The error occurs in the .Open line and displays "Could not find installable ISAM." It seems to be a generic error but I can't identify what's causing the problem. Also, I should have mentioned this in the original post, but I'm using Excel 2007. – Aei Jun 26 '15 at 14:03
1  
Fixed by using .Provider = "Microsoft.ACE.OLEDB.12.0", and after a minor issue in the rst.Open line, everything works perfectly now. I appreciate all the help. – Aei Jun 26 '15 at 15:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.