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