Building an OPC client in Excel

I am pretty deep into building a very multi-faceted little app in Excel with VBA. It does a number of tasks, all centered around using OPC to get tag values from several PLC's and doing various things with the info, like publishing a webpage (using a module I found, not mine), creating log files and tables and sounding some audible alarms for the office.

What I'm doing now is, upon a button push, connecting to the server (RSLinx), then entering a loop that first reads the tag values, then does each of the above tasks if the associated checkbox is checked. This will run fine indefinitely so long as the user doesn't screw with it or Linx or let the computer lock.

I am a beginner coder so, please give me some feedback on the code itself, but what I'm really trying to do is make this bulletproof, so that it will run without fail. I've added some things like on a selectionchange event, if you're connected, give a messagebox that says not to make changes while running. But, I know this can be much better.

Also, I already know this might be done better in various other ways, but this is a little beginner pet project for me and I'm going to see it through before I move on. All criticism is welcome.

```
Option Explicit ' variables must be declared
Option Base 1 ' array starts at index 1

'Dim OPCServer1 As OPCServer
Dim WithEvents OPCGroup1 As OPCGroup
Dim MyOPCItems() As OPCItem
Dim NumberOfTags As Integer
Dim ReadInterval As Double

Sub OPC_Connect()
debugEvent "OPC - OPC_Connect()"
status_update ("Connecting...")
On Error GoTo Error_OpcConnectionFailure
Dim GrpName As String
Dim i As Integer
Dim g As Integer
Dim h As Integer
Dim row_tag_name As String

For h = 1 To 7
Module1.SavedThisTime(h) = False
Next h

ThisWorkbook.connected = True
GrpName = Cells(5, 2)
'NumberOfTags = Cells(6, 2)
NumberOfTags = 0
status_update ("Adding Tags...")
For g = 1 To 10000
If Not IsEmpty(Cells(4 + g, 4

Options

Option Explicit  ' variables must be declared

Kudos for 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 1    ' array starts at index 1

Same thing here, the comment basically explains what the statement does. Be careful with 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.

Naming

The VBA naming guidelines recommend 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.

Now VBA is not case-sensitive, so appropriate and meaningful naming is crucial, otherwise you will constantly be fighting your IDE.

Speaking of meaningful names:

Dim i As Integer
Dim g As Integer
Dim h As Integer

For i = 1 To NumberOfTags

A better name for 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.

For g = 1 To 10000
    If Not IsEmpty(Cells(4 + g, 4).value) Then

A better name for g could be currentRow.

For h = 1 To 7
    Module1.SavedThisTime(h) = False
Next h

This one is totally cryptic. One cannot infer the meaning of 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.

Dsmvwlng

That's right: disemvoweling. 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.

Error Handling

Error_OpcConnectionFailure:

    OPC_Disconnect
    MsgBox ("Connection Failed:" & vbNewLine & vbNewLine & "Check Your server name and ensure RSLinx is Running.")
    Exit Sub

Error_TagNotFound:

    OPC_Disconnect
    MsgBox ("Connection Failed. Check tag: " & vbNewLine & vbNewLine & row_tag_name)
    Exit Sub

The only difference between these two subroutines, is the string they're passing to the message box.

A procedure should only have exactly 1 On Error GoTo statement, and exactly 1 error-handling subroutine.

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

Instead, On Error GoTo ErrHandler, or as I like to put it, On Error GoTo CleanFail:

Public Sub Foo()
    On Error GoTo CleanFail

    '...

CleanExit:
    Exit Sub

CleanFail:
    'handle error
    Resume CleanExit
End Sub

Now, this 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.

StackExchange Code Review Q#73348, answer score: 10