Dim
at the module level. Use Private
. Yes, they both do the same thing, but using Private
is a visual indication that these variables belong to the module scope. i
variable as a counter for each loop. The eye will instantly recognize it as a loop counter. Or even better, extract some methods so you don't have to stress about re-using variables. (I'll come back to that in a bit.)Cells
.Cells
the way you have used it here is equivalent to calling ActiveSheet.Cells
. Almost never should we be working with the active sheet because the user can change it on us while the code is executing. Avoid activate and select by explicitly setting a variable to the range you want to work with.Sheets
. This codeThisWorkbook.Sheets('Setup')
. If the user clicks on another workbook, your code is going to blow up because it can't find the 'Setup'
worksheet.WithEvents
. (Kudos for that by the way. I don't see many programmers take advantage of that.)Call
keyword. It's archaic and only available for back ward compatibility reasons.Option Explicit
- requiring variable declaration is the very first step toward writing clean VBA code. It shouldn't need a comment though: comments should say why, not what - any VBA programmer looking at an Option
statement will know what it's for. And those who don't, can Google it.Option Base
though, as it tends to make things confusing - arrays are known to start at index 0, and collections at index 1. Using Option Base
encourages lazy array declarations - a better practice is to always specify both the lower and the upper bounds of an array, and to use LBound
and UBound
when iterating one. Shortly put, I consider Option Base 1
a code smell all by itself.PascalCase
for everything except perhaps constants, which would be YELLCASE
. Regardless of whether or not you follow these guidelines, what matters the most is consistency. Here are my own guidelines:PascalCase
for procedures (Sub
, Function
, Property
), module names (including class names), and in general, any public identifier.camelCase
for parameters, local variables and private fields.i
could be currentTag
. Well, i
is indeed commonly used as a loop counter, but in a procedure that has 3 of them, it's better to give them a meaningful name.g
could be currentRow
.h
, nor why it has to be iterated from 1 to 7. Also, SavedThisTime(h)
looks like it's a public field (well, an array) defined in a standard module - in other words, a global variable that could just as well be referred to as SavedThisTime(h)
without the Module1
qualifier.. and Module1
isn't a useful name either.GrpName
has no reason not to be called GroupName
; about 20% of programming consists in writing code. The other 80% is spent reading code - might as well make it worth the while.On Error GoTo
statement, and exactly 1 error-handling subroutine.Sub OPC_Connect()
: catching an error because a connection failed, and another because a tag was not found, smells: there should be a procedure responsible for connecting, and another for finding tags.On Error GoTo ErrHandler
, or as I like to put it, On Error GoTo CleanFail
:OPC_Connect
procedure handles both cases by displaying a message box; I believe in this case, knowing exactly what to do with an error should be the responsibility of the calling code - what the handler can do, is make that error as informative as possible, by raising the same error number, with new Source
and Description
values.Cells(4, 2)
that means ActiveSheet.Cells(4, 2)
, so if you have more sheets and user changes the active sheet -> another value -> error. Same with Sheets()
but not such critical as Cells()/Range()
.Button_Click
(UI event handler like OPC_DisconnectBtn_Click()
). It wouldn't matter what is actually going wrong by any user action that starts a macro, you have the control over what the user will see.const PI = 3.14159265358979