• 1

• 75.5k
• 18
• 194
• 467

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