Given the following code, I am still refactoring it:
Private Function FieldChanged(Field As Access.Control) As Boolean
Dim retVal As Boolean
If OnlyOneFieldIsNull(Field.OldValue, Field.Value) Then
retVal = True
ElseIf IsNull(Field.OldValue) And IsNull(Field.Value) Then
retVal = False
Else
If Field.OldValue <> Field.Value Then
retVal = True
End If
End If
FieldChanged = retVal
End Function
Private Function OnlyOneFieldIsNull(OldValue As Variant, NewValue As Variant) As Boolean
Dim retVal As Boolean
retVal = (IsNull(OldValue) = True And IsNull(NewValue) = False) Or (IsNull(OldValue) = False And IsNull(NewValue) = True)
OnlyOneFieldIsNull = retVal
End Function
I’ve pulled out the FieldChanged function from the BeforeUpdate event already, see the TDD – 036 article for that. I still have way to many If blocks in that first function, and I don’t really like the readability of the second function. It is clear to me what it is doing by the name though: OnlyOneFieldIsNull, takes two values and returns a boolean.
All the function is really doing is checking both conditions where 1 is null and 2 is not null, or 1 is not null and 2 is null. Maybe just using Not instead of a true false check will be more readable:
Private Function OnlyOneFieldIsNull(OldValue As Variant, NewValue As Variant) As Boolean
Dim retVal As Boolean
retVal = (IsNull(OldValue) And Not IsNull(NewValue)) Or (Not IsNull(OldValue) And IsNull(NewValue))
OnlyOneFieldIsNull = retVal
End Function
It’s a little shorter. What if I add one more line to get rid of the parentheses and Or condition?
Private Function OnlyOneFieldIsNull(OldValue As Variant, NewValue As Variant) As Boolean
Dim retVal As Boolean
retVal = IsNull(OldValue) And Not IsNull(NewValue)
If Not retVal Then retVal = Not IsNull(OldValue) And IsNull(NewValue)
OnlyOneFieldIsNull = retVal
End Function
Actually, the above introduces possible logic errors because I’m trying to be clever. I think I’ll just create new variable names to document the conditions better.
Private Function OnlyOneFieldIsNull(OldValue As Variant, NewValue As Variant) As Boolean
Dim retVal As Boolean, OldIsNullNewIsNot As Boolean, OldIsNotNullNewIs As Boolean
OldIsNullNewIsNot = IsNull(OldValue) And Not IsNull(NewValue)
OldIsNotNullNewIs = Not IsNull(OldValue) And IsNull(NewValue)
retVal = OldIsNullNewIsNot Or OldIsNotNullNewIs
OnlyOneFieldIsNull = retVal
End Function
Eh, I suppose I could make each of the variables functions which would then make this more readable as well. Ah, ok, I’ll do it.
Private Function OnlyOneFieldIsNull(OldValue As Variant, NewValue As Variant) As Boolean
OnlyOneFieldIsNull = OldIsNullNewIsNot Or OldIsNotNullNewIs
End Function
Private Function OldIsNullNewIsNot(OldValue As Variant, NewValue As Variant) As Boolean
OldIsNullNewIsNot = IsNull(OldValue) And Not IsNull(NewValue)
End Function
Private Function OldIsNotNullNewIs(OldValue As Variant, NewValue As Variant) As Boolean
OldIsNotNullNewIs = Not IsNull(OldValue) And IsNull(NewValue)
End Function
Ok, now it’s obvious I think what’s going on. I also removed the retVal variable I usually use because I don’t think I’ll need to revisit these functions and if I rename them I still just need to rename one line. So far so good. Now I shall turn my attention to the bigger function:
Private Function FieldChanged(Field As Access.Control) As Boolean
Dim retVal As Boolean
If OnlyOneFieldIsNull(Field.OldValue, Field.Value) Then
retVal = True
ElseIf IsNull(Field.OldValue) And IsNull(Field.Value) Then
retVal = False
Else
If Field.OldValue <> Field.Value Then
retVal = True
End If
End If
FieldChanged = retVal
End Function
So after the function to check if one field is null I check if both fields are null. That’s taking care of all the edge case null conditions. I think I can boil this function down to 2 lines. And edge case Null check and a non-null check. I will continue refactoring to my satisfaction next time.