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

Concatenate data files into master document

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

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: 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 Function


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


The 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 Function


Of 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 Next


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!

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 Sub


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.

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 Function
Option Explicit

Public Sub DoSomething()
    Dim foo
    foo = 42
End Sub

Public Sub DoSomethingElse()
    Dim foo
    foo = 12
End Sub
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 Function
On Error Resume Next
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 Sub

Context

StackExchange Code Review Q#115345, answer score: 4

Revisions (0)

No revisions yet.