General function to test for empty/no-value controls

Question

I'm working on a very CRUD-heavy MS Access application, which has over 50 forms for entering and working with data. This, of course, involves lot of testing to see if a textbox, recordset, etc. has a value and then doing something with it.

To prevent mistakes such as testing for IF not rst.BOF and rst.EOF or simply having to look up a test I don't remember, I've created a single function that runs all of my empty/no value tests. Simply pass NOVALUE() an object variable and it uses the Typename() function to select the correct test and return a true/false. No more thinking about tests and no chance of introducing a bug at this point.

The function's purpose is to see if the user has input/selected something or if a recordset has not returned any records. I don't care whether the .RecordSource of a cascading combo/listbox has no rows because the object will also have no value. Also, I'm only using native Access controls, so the function will not test a TreeView or other ActiveX control.

Public Function NoValue(ObjectToTest As Object) As Boolean
 Dim noVal As Boolean
 NoValue = True 
 If ObjectToTest Is Nothing Then
 noVal = True
 Else
 Select Case TypeName(ObjectToTest)
 Case Is = "CheckBox"
 noVal = IsNull(ObjectToTest.Value)
 Case Is = "ComboBox"
 If ObjectToTest.ItemsSelected.Count = 0 Then
 noVal = True
 Else
 noVal = False
 End If
 Case Is = "Label"
 If Len(ObjectToTest.Caption) = 0 Then
 noVal = True
 Else
 noVal = False
 End If
 Case Is = "Listbox"
 If ObjectToTest.ItemsSelected.Count = 0 Then
 noVal = True
 Else
 noVal = False
 End If
 Case Is = "Recordset" '* ADO
 If ObjectToTest.State = adStateClosed Then
 noVal = True
 ElseIf ObjectToTest.BOF And ObjectToTest.EOF Then
 noVal = True
 Else
 noVal = False
 End If
 Case Is = "Recordset2" '* DAO
 If ObjectToTest.BOF And ObjectToTest.EOF Then
 noVal = True
 Else
 noVal = False
 End If
 Case Is = "TextBox"
 If Len(ObjectToTest.Value & vbNullString) = 0 Then
 noVal = True
 End If
 Case Else
 ' Only using native access types, not activex, so there shouldn't be anything here
 ' Should I err.Raise ?
 End Select
 End If
 NoValue = noVal
 End Function

Here's the reasoning behind the tests:

• Begin with a test for ObjectToTest Is Nothing to make sure that the object variable has been initialized.
• ADO Recordset: If both .bof and .eof are true, an ADO recordset has no records. I've added the test for closed recordsets because I've occasionally come across them.
• DAO Recordset: Just the standard test for .bof and .eof. There is no way to check for a closed DAO recordset.
• Checkbox: Testing for null, because 0 and -1 (false/true) are values.
• Combobox/Listbox: There are two possible tests that can be performed here, depending on how you want to handle table values that are not in the object's .RowSource.
• Labels: Although these aren't data-bound controls they can have their captions set in code, so I've included them for the sake of consistency -- I really don't want to have to think about testing controls.
• Testing the to see if object's .ControlSource Is NULL will return false if the .ControlSource has a value (i.e. there is a value in the underlying table). A combo/listbox with an invalid value in the underlying table (a value that doesn't appear in the .RowSource) not be NULL, but no item will be selected in the list.
• Testing .ItemsSelected.Count will tell us whether the user has selected an item, regardless of the value in the underlying table. Before the record is saved, the underlying value in the table may be null, but .ItemsSelected.Count will reflect the user's selection. This is what I'm interested in.
• Textboxes: A textbox that contains an empty string will not be null, even though it has no value for my purposes. So, the function tests for both NULL and an empty string. I always use the constant vbNullString instead of "" in my code so there is no chance of confusing " " with "".

Questions:

• Are all of my tests correct? Are there any edge-cases that I have missed?
• My SELECT ... CASE should probably contain a "CASE ELSE" to catch any other object that gets passed to the function. Does it make the most sense to raise an error at this point, or to take some other action?
• Anything else I may have missed or not thought of?

Answer 1

Case Is = "CheckBox"

That syntax is very weird and unusual, surprising even.

This would be more common, and simpler:

Case "Recordset"

TypeName makes a stringly-typed check, which is bug-prone. You say you're not going to use it with any ActiveX controls, but can you be sure no one in the future will ever try to do that? ADODB.Recordset isn't an ActiveX control - neither is DAO.Recordset. So you've already started using it for more than controls.

Consider using the much more robust TypeOf {object} Is {type} approach instead - because of how TypeOf expressions work, you'll have to put the condition in each Case statement for it to work:

Select Case True
 Case TypeOf ObjectToTest Is ADODB.Recordset
 'we have an ADODB.Recordset instance
 Case TypeOf ObjectToTest Is DAO.Recordset
 'we have a DAO.Recordset instance
 '...
End Select

I don't do much MS-Access, but if "form controls" in Access are anything like Excel's, then the Case blocks for these would look like:

 Case TypeOf ObjectToTest Is Access.Label
 '...

That way you can handle this case separately when the time comes:

 Case TypeOf ObjectToTest Is MSForms.Label
 '...

---

HasNoValue isn't instinctive, or desirable. Consider the client code that checks whether a control has a value:

If Not HasNoValue(SomeControl) Then

Avoid double-negatives wherever possible. So, invert the whole logic, so that the client code doesn't have them no matter what:

If HasValue(SomeControl) Then

Or

If Not HasValue(SomeControl) Then

Much cleaner.

---

If ObjectToTest Is Nothing Then
 noVal = True

You're hiding a bug here: if client code passes a null object reference (aka Nothing), you want to blow up right there and then - not pretend you were given a control that "has no value": you're misleading the client code into believing everything is fine & well, when there's a serious problem going on. Best fail fast and hard than make the bug even harder to track down by ignoring invalid state.

This would be my guard clause:

If ObjectToTest Is Nothing Then Err.Raise 91, , "ObjectToTest cannot be a null reference."

Given invalid input, scream - don't push the bug further down the call stack.

• DataWriter: Thank you for your helpful review. I've already incorported many of yoru suggestions. I'll post an actual reply as soon as I can sneak away from my work, but I didn't want to go another day without acknowledging your help. Thanks, again!
• Mathieu Guindon: @DataWriter thanks! Remember to not edit your original question with the updated code, as that would invalidate the existing answers here. See /help/someone-answers for a clear descriptions of what your options are.

Answer 2

no chance of introducing a bug at this point - Oh ye of little faith :) there will always be bugs. Always. But seriously, this is very good for the most part. I only have a few suggestions.

Since your function returns a bool, I would suggest renaming it to HasNoValue(...).

Your function also has too many responsibilities. What happens when you support more controls or types? You'd have to make this function much longer. I'd recommend making each type have its own validation check.

Also, I would host these validation checks in a separate module called Validation.

For each type, you could name its function to something like TypeHasNoValue like CheckboxHasNoValue. When calling the code, you could use the module name in front to fully qualify it like Validation.CheckboxHasNoValue(...)

In module Validation:

Public Function CheckboxHasNoValue(ByRef checkbox As Chekbox) As Boolean
 CheckboxHasNoValue = IsNull(checkbox.Value)
End Function

I know this might seem like overkill, but if you later want to include something like a default (something like an empty item), then you can also check for that in this function whose responsibility is checking a Checkbox for a value.

In your main module, your function becomes something like:

Public Function HasNoValue(ObjectToTest As Object) As Boolean
 Dim noVal As Boolean
 NoValue = True 
 If ObjectToTest Is Nothing Then
 noVal = True
 Else
 Select Case TypeName(ObjectToTest)
 Case Is = "CheckBox"
 noVal = Validation.CheckboxHasNoValue(ObjectToTest)
 Case Is = "ComboBox"
 noVal = Validation.ComboBoxHasNoValue(ObjectToTest)
 Case Is = "Label"
 noVal = Validation.LabelHasNoValue(ObjectToTest)
 Case Is = "Listbox"
 noVal = Validation.ListboxHasNoValue(ObjectToTest)
 Case Is = "Recordset" '* ADO
 noVal = Validation.RecordsetHasNoValue(ObjectToTest)
 Case Is = "Recordset2" '* DAO
 noVal = Validation.Recordset2HasNoValue(ObjectToTest)
 Case Is = "TextBox"
 noVal = Validation.TextBoxHasNoValue(ObjectToTest)
 Case Else
 ' Only using native access types, not activex, so there shouldn't be anything here
 ' Should I err.Raise ?
 End Select
 End If
 NoValue = noVal
 End Function

As for your question about Case Else, yes I think you should have one. But about the behavior, that really depends on your application. Do you have error handling? Do you have a log? Do you just want to silently fail because maybe it's not critical? It really depends. But, yes, you should have one for completion sake.

Answer 3

I was finding that when I inspected a control on a form for a value an error was being raised if the form had no recordset but the control had a controlsource.

Taking into consideration what Mathieu Guindon wrote in his answer. I used the example given in the question to create the two functions below that I think may be of use to people reading this question!

 Public Function HasValue(ControlObject As Control, Optional ControlsValue As Variant) As Boolean
 ' IN ControlObject - The object to inspect to see whether it has a value
 ' OUT TheValue - A ByRef parameter that allows this to return the value of the object
 ' if it is determined to have one!
 Dim HasVal As Variant
 On Error GoTo Err1
 ' The code below should always set HasVal to either be True or False
 HasVal = Null
 ' Return NULL if an object has 'no value'.
 ' (NULL chosen as this is a database environment where fields are NULL if no data is stored by them.)
 ControlsValue = Null
 If ControlObject Is Nothing Then
 HasVal = False
 ControlsValue = Null
 ElseIf TypeOf ControlObject Is Label Then
 If Len(ControlObject.Caption) = 0 Then
 HasVal = False
 ControlsValue = Null
 Else
 HasVal = True
 ControlsValue = ControlObject.Caption
 End If
 Else
 ' Note that error 2424 is raised if the Value proeprty of the following controls is
 ' referred to in code when
 ' 1. The control's 'Control Source' property is set to a field in the forms recordsource.
 ' and
 ' 2. the Form.RowSource property is empty.
 '
 ' Note a control's control source property can be set to text that does not refer to the recordset
 ' in which case no error is raised! eg =MyFunction(1,2,3)
 ' So no test can be added here to predict when the above error (error 2424) is raised.
 '
 If TypeOf ControlObject Is TextBox _
 Or TypeOf ControlObject Is ListBox _
 Or TypeOf ControlObject Is ComboBox _
 Or TypeOf ControlObject Is CheckBox _
 Then
 On Error GoTo ErrNoValueProperty
 HasVal = Not IsNull(ControlObject.Value)
 ' Note that the above treats an empty string as being a value.
 'If ControlObject.Value = "" Then
 ' HasVal = False
 'End If
 ControlsValue = ControlObject.Value
 On Error GoTo Err1
 Else
 Err.Raise cnstUserDefinedErrorLowest + 1, "", "Unknown ControlObject."
 End If
 End If
TidyUpAfterNoValueProperty:
 HasValue = CBool(HasVal)
Exit Function
ErrNoValueProperty:
 If Err.Number = 2424 Then
 '
 ' 2424 = "The expression you entered has a field, control, or property name
 ' that This Database can't find."
 '
 ' See comments in code above for explaination!
 '
 HasVal = False
 GoTo TidyUpAfterNoValueProperty
 Else
 GoTo Err1
 End If
Err1:
 MyErrDescrition = BuildErrorDescription("HasValue", cnstModuleName)
 Err.Raise Err.Number, Err.Source, MyErrDescrition
 Exit Function
 Resume
End Function
Public Function NzForObjects(ControlObject As Object, ValueIfUnEntered As Variant) As Variant
 Dim ControlsValue As Variant
 On Error GoTo Err1
 If HasValue(ControlObject:=ControlObject, ControlsValue:=ControlsValue) Then
 NzForObjects = ControlsValue
 Else
 NzForObjects = ValueIfUnEntered
 End If
Exit Function
Err1:
 MyErrDescrition = BuildErrorDescription("NzForObjects", cnstModuleName)
 Err.Raise Err.Number, Err.Source, MyErrDescrition
 Exit Function
 Resume
End Function

Answer 4

A few minor things that weren't explicitly picked up on in the other answers;

If ObjectToTest.ItemsSelected.Count = 0 Then
 noVal = True
Else
 noVal = False
End If

My personal preference in Boolean assigning blocks like these is a one liner:

noVal = ObjectToTest.ItemsSelected.Count = 0

Which I find scans better and avoids the next problem (although I'm aware some people are squeamish about this sort of thing and prefer to be explicitly explicit)

---

If you do use the If block at least be consistent:

Case Is = "TextBox"
 If Len(ObjectToTest.Value & vbNullString) = 0 Then
 noVal = True
 End If
Case Else

Here you have no Else statement and instead rely on the default initial value of noVal as False, which means if your logic changes and you end up writing to the variable earlier in the procedure, this check will break. The one liner helps to avoid such inconsistencies.

---

While we're here, I'd also like to make a brief point about comments; comments are there to explain why you have taken certain decisions in your code. Your check for textboxes is, to me at least, non-obvious on first glance - so a comment to explain why would be helpful:

noVal = Len(ObjectToTest.Value & vbNullString) = 0
'Append vbNullString so the function tests for both NULL and an empty string

In fact for that matter, any time you've explained why your code does something in your question body, you should ideally have a comment instead (or your code should be self documenting with helpful variable names and abstraction, which for the most part it is)

---

Case Else
 ' Only using native access types, not activex, so there shouldn't be anything here
 ' Should I err.Raise ?

Well that depends; as @MathieuGuindon correctly points out, the last thing you want to do is to hide invalid input. The best way to think about this is to imagine what happens if you do supply an ObjectToTest which you think should have a check but doesn't. Would you want a default return value (or a default check) for unimplemented cases? Maybe - but unlikely. Would you want to be made aware that the check you are trying to make is not implemented? Probably.

So yes, I would recommend some sort of notification here - be it an error or a warning message or Stop. Personally I'd go for the error because it is explicit and logabble and even supressable, something along the lines of

Err.Raise 5, , "No check implemented for objects of type " & Typename(ObjectToTest)

Where the 5 corresponds to an invalid argument (see trappable errors). Use a named Const if you want, I'm lazy and don't bother if it's just in one place in the procedure.