Bad form (1 Viewer)

[Engineer Chris]

This is my first post! In the code below, I am looping through the controls on a form to determine whether or not they all meet the conditions necessary to allow the data to be saved.

The code works, but I've read so many times that using the goto statement is 'bad form' so I don't want to keep it this way but I haven't come up with anything better. Please help.

Public Function AllowSave(fForm As Form, lCriteria As Long) As Boolean

'**********************************************************************
'Input: fForm as form - Should be calling form
'       lCriteria as long - Represents criteria to check.  Should be a
'                           bitwise combination where multiple
'                           criteria are to be checked
'Checks for special formatting in any of the identified form's controls,
'based on the criteria specified in lCriteria.  If that formatting
'exists, AllowSave returns 'False', otherwise it returns 'True'.
'**********************************************************************

'****************************************
'Error handling and variable definition
'****************************************

    On Error GoTo ErrorHandler
    sErrorModuleTemp = sErrorModule
    sErrorModule = "AllowSave (fForm As Form): fForm = " & fForm.Name
    AllowSave = False
    Dim c As Control

'****************************************
'Check controls for 'unacceptable' formatting
'****************************************

    For Each c In fForm.Controls
        If lCriteria And ControlColor.Duplicate = ControlColor.Duplicate Then
            If c.Properties("forecolor") = ControlColor.Duplicate Then
                Exit Function
NotGoodProgramming:
                '
            End If
        End If
    Next

    AllowSave = True

ExitFunction:

    sErrorModule = sErrorModuleTemp
    Exit Function

ErrorHandler:

    Select Case Err.Number
        Case 2455
            Resume NotGoodProgramming
        Case Else
            If GlobalErrHandle(Err, sErrorForm, sErrorModule) Then
                Resume Next
            End If
    End Select

    Resume ExitFunction

End Function

By the way, there will be multiple checks eventually which is why there may seem to be an extra 'if' statement.

Thanks

 

[MarkK]

You can just use 'Resume' or 'Resume Next'. You don't have to use 'Resume <label name>'.

 

[Engineer Chris]

Sorry - I should have explained better. I use 'resume next' in the 'case else' part of the select statement. However, that leads the code to the command 'exit function'. However, if the error code is 2455, I want to bypass that 'exit function' command and then resume. Hopefully that makes a little more sense.

 

[MarkK]

What is error 2455? Presumably not all controls c have a forecolor property? I would test for control type in the loop, and only execute that If block expression against controls that have the property, so...

for each c in frm.controls
   if c.controltype = acTextbox or c.controltype = acCombobox then
      debug.print c.name, typename(c)
   end if
next

...or if the error is something else, test for the condition that would cause that error before it occurs, and handle it that way.

See how your code just blows up, falls back to error handling, and skips the problem? So for me, looking at it as the second developer, your code should educate me about what the problem is you are handling. If only some subset of control is valid to use in this case, then don't be lazy. Subset the controls. This informs the next developer who comes to service this code, what is going on.

For this reason, writing code that falls back to error handling as a routine means of solving a problem is commonly not considered a best practice.
hth
Mark