patternMinor
Concatenate data files into master document
Viewed 0 times
intoconcatenatemasterfilesdocumentdata
Problem
Here is code I wrote to concatenate data files into master file. Once the data to be pasted into the master sheet exceeds the number of rows left in the master sheet, the program will create a new master document and continue the process. Help me clean it up and make it faster if you can.
```
Option Explicit
Public NewMasterFile As Workbook
Public DataFile As Workbook
Public DataFilePath As String
Public DataFileName As String
Public FolderPath As FileDialog
Public eRow As Long
Public MasterFilePath As String
Dim myExtension As String
'Public i As Long
Public K As Long
Public R As Long
Public J As Long
Public A1 As String
Public B1 As String
Public C1 As String
Public D1 As String
Public cell As Range
Sub ConcatinateFiles()
On Error Resume Next
'Preset variables
J = 1
'i = 1
A1 = "SCEDTimestamp"
B1 = "RepeatedHourFlag"
C1 = "SettlementPoint"
D1 = "LMP"
'Optimize Macro Speed
Application.ScreenUpdating = False
Application.EnableEvents = False
Application.Calculation = xlCalculationManual
Application.DisplayAlerts = False
'Retrieve Target Master Workbook data folder Path From User
Set FolderPath = Application.FileDialog(msoFileDialogFolderPicker)
With FolderPath
.Title = "Select a master workbook data folder."
.AllowMultiSelect = False
If .Show <> -1 Then GoTo CancelSelect1
MasterFilePath = .SelectedItems(1) & "\"
End With
'In case Cancel selected
CancelSelect1:
MasterFilePath = MasterFilePath
If MasterFilePath = "" Then GoTo ResetSettings
'Retrieve Target Workbook data folder Path From User
Set FolderPath = Application.FileDialog(msoFileDialogFolderPicker)
With FolderPath
.Title = "Select a workbook data folder."
.AllowMultiSelect = False
If .Show <> -1 Then GoTo CancelSelect2
DataFilePath = .SelectedItems(1) & "\"
End With
'In case Cancel selected
CancelSelect2:
DataFilePath = DataFilePath
If DataFilePath = "" Then GoTo ResetSettings
'Target File Extension (must include wildcard "*" and ".xl??" the ? is a wildcard to search for all different types of ex
```
Option Explicit
Public NewMasterFile As Workbook
Public DataFile As Workbook
Public DataFilePath As String
Public DataFileName As String
Public FolderPath As FileDialog
Public eRow As Long
Public MasterFilePath As String
Dim myExtension As String
'Public i As Long
Public K As Long
Public R As Long
Public J As Long
Public A1 As String
Public B1 As String
Public C1 As String
Public D1 As String
Public cell As Range
Sub ConcatinateFiles()
On Error Resume Next
'Preset variables
J = 1
'i = 1
A1 = "SCEDTimestamp"
B1 = "RepeatedHourFlag"
C1 = "SettlementPoint"
D1 = "LMP"
'Optimize Macro Speed
Application.ScreenUpdating = False
Application.EnableEvents = False
Application.Calculation = xlCalculationManual
Application.DisplayAlerts = False
'Retrieve Target Master Workbook data folder Path From User
Set FolderPath = Application.FileDialog(msoFileDialogFolderPicker)
With FolderPath
.Title = "Select a master workbook data folder."
.AllowMultiSelect = False
If .Show <> -1 Then GoTo CancelSelect1
MasterFilePath = .SelectedItems(1) & "\"
End With
'In case Cancel selected
CancelSelect1:
MasterFilePath = MasterFilePath
If MasterFilePath = "" Then GoTo ResetSettings
'Retrieve Target Workbook data folder Path From User
Set FolderPath = Application.FileDialog(msoFileDialogFolderPicker)
With FolderPath
.Title = "Select a workbook data folder."
.AllowMultiSelect = False
If .Show <> -1 Then GoTo CancelSelect2
DataFilePath = .SelectedItems(1) & "\"
End With
'In case Cancel selected
CancelSelect2:
DataFilePath = DataFilePath
If DataFilePath = "" Then GoTo ResetSettings
'Target File Extension (must include wildcard "*" and ".xl??" the ? is a wildcard to search for all different types of ex
Solution
First things first.
Indentation and readability
It's extremely hard to read/follow the code, because the indentation is, well, inexistent. You've probably noticed some keywords come in pairs:
Some code blocks are special, they also define a scope - in VBA there are two levels of scope: module-level, and member-level.
Here's an example module:
Notice the indentation makes it easy to quickly identify code blocks. Proper indentation is crucial if you want other people to be able to read your code... and "other people" includes yourself in a few weeks' time, too.
Scoping and visibility
Everything you've declared in the module's declarations section is scoped at module-level: all these variables are visible to everything in that module. Furthermore, anything at module-scope that you declare as
Consider this:
The two procedures define their own
You want to avoid globals, and you want your variables as short-lived as possible - this makes the code easier to follow, and easier to maintain; your brain doesn't need to keep track of everything that's going on at once!
Parameters and return values
A good way to pass values around and to avoid global state, is to use parameters and return values. Consider:
Of course this is just over-simplified example code, but you get the idea:
Error Handling.
If something can go wrong, Murphy's Law applies: that thing will end up going wrong at one point or another.
This is your worst enemy. It's the devil incarnate: it makes you think everything is going well, and then your code starts not working as you would think it should be working, and you have no clue why, because it just keeps running: that instruction is telling VBA to take any runtime error that might happen, and ignore it completely.
Instead of shoving errors under the proverbial carpet, handle them!
I've merely scratched the surface, but this answer is getting long enough. I would suggest you clean up what you have now (by fixing indentation, tightening variable lifetime and visibility, and handling runtime errors), and post a new follow-up question: it's easier to review code that's easier to read.
Indentation and readability
It's extremely hard to read/follow the code, because the indentation is, well, inexistent. You've probably noticed some keywords come in pairs:
Sub/End Sub, If/End If, With/End With, Do/Loop, ...there's a lot more, but the thing to remember is that these pairs form code blocks.Some code blocks are special, they also define a scope - in VBA there are two levels of scope: module-level, and member-level.
Sub and Function (and Property, but we're not there yet) define a member-level scope.Here's an example module:
Option Explicit
Private foo As Integer
Public Sub DoSomething()
If foo = 0 Then
foo = 42
Else
foo = GetFoo
End If
End Sub
Private Function GetFoo() As Integer
GetFoo = foo - 1
End FunctionNotice the indentation makes it easy to quickly identify code blocks. Proper indentation is crucial if you want other people to be able to read your code... and "other people" includes yourself in a few weeks' time, too.
Scoping and visibility
Everything you've declared in the module's declarations section is scoped at module-level: all these variables are visible to everything in that module. Furthermore, anything at module-scope that you declare as
Public will also be visible to everything else in the project - in other words, a Public field in a standard code module in VBA, is a global variable.Consider this:
Option Explicit
Public Sub DoSomething()
Dim foo
foo = 42
End Sub
Public Sub DoSomethingElse()
Dim foo
foo = 12
End SubThe two procedures define their own
foo variable. How is that possible? Each procedure has its own scope - in other words, foo is a local variable and as such, it's a distinct variable in each procedure, that "dies" as soon as execution gets to End Sub - they're also re-created as if they never existed, whenever the procedure gets called.You want to avoid globals, and you want your variables as short-lived as possible - this makes the code easier to follow, and easier to maintain; your brain doesn't need to keep track of everything that's going on at once!
Parameters and return values
A good way to pass values around and to avoid global state, is to use parameters and return values. Consider:
Public Sub DoSomething()
Dim fn As String
fn = Range("A1").Value 'assumes cell contains a valid file name
Dim wb As Workbook
Set wb = OpenWorkbook(fn)
wb.Close
End Sub
Private Function OpenWorkbook(ByVal fileName As String) As Workbook
'fileName is a copy of the fn string from the calling code
Set OpenWorkbook = Workbooks.Open(fileName)
End FunctionOf course this is just over-simplified example code, but you get the idea:
OpenWorkbook returns a Workbook object that DoSomething uses to do its thing. Variables are locally declared, and there's no need for any globals anywhere.Error Handling.
If something can go wrong, Murphy's Law applies: that thing will end up going wrong at one point or another.
On Error Resume NextThis is your worst enemy. It's the devil incarnate: it makes you think everything is going well, and then your code starts not working as you would think it should be working, and you have no clue why, because it just keeps running: that instruction is telling VBA to take any runtime error that might happen, and ignore it completely.
Instead of shoving errors under the proverbial carpet, handle them!
Public Sub DoSomething
On Error GoTo CleanFail
'procedure body here
CleanExit:
'normal cleanup here
Exit Sub 'ensures error handler only runs on error!
CleanFail:
'handle runtime errors here
Resume CleanExit 'place breakpoint e.g. here, to debug
Resume 'jumps to the line that caused the error
End SubI've merely scratched the surface, but this answer is getting long enough. I would suggest you clean up what you have now (by fixing indentation, tightening variable lifetime and visibility, and handling runtime errors), and post a new follow-up question: it's easier to review code that's easier to read.
Code Snippets
Option Explicit
Private foo As Integer
Public Sub DoSomething()
If foo = 0 Then
foo = 42
Else
foo = GetFoo
End If
End Sub
Private Function GetFoo() As Integer
GetFoo = foo - 1
End FunctionOption Explicit
Public Sub DoSomething()
Dim foo
foo = 42
End Sub
Public Sub DoSomethingElse()
Dim foo
foo = 12
End SubPublic Sub DoSomething()
Dim fn As String
fn = Range("A1").Value 'assumes cell contains a valid file name
Dim wb As Workbook
Set wb = OpenWorkbook(fn)
wb.Close
End Sub
Private Function OpenWorkbook(ByVal fileName As String) As Workbook
'fileName is a copy of the fn string from the calling code
Set OpenWorkbook = Workbooks.Open(fileName)
End FunctionOn Error Resume NextPublic Sub DoSomething
On Error GoTo CleanFail
'procedure body here
CleanExit:
'normal cleanup here
Exit Sub 'ensures error handler only runs on error!
CleanFail:
'handle runtime errors here
Resume CleanExit 'place breakpoint e.g. here, to debug
Resume 'jumps to the line that caused the error
End SubContext
StackExchange Code Review Q#115345, answer score: 4
Revisions (0)
No revisions yet.