Sanity check: What's wrong with this code?

[Rhino]

This sub worked fine several times then suddenly stopped working. Can anyone
help me figure out what is wrong?

-----------------------------------------------------------------------------------------------------
Public Sub getLastRevisionDate()

'Set the title for the MsgBoxes which will be displayed.
Const TITLE As String = "Custom Properties"

'Set the name of the custom property.
Const REVISION_DATE_KEY As String = "LAST_REVISION_DATE"

'If there is no document open, there is nowhere to get custom properties
either. Inform the user.
If Application.Documents.count = 0 Then
MsgBox "There are no documents open so no custom properties can be
found. Open a document before trying this macro again.", vbOKOnly, "Custom
Properties"
Exit Sub
End If

'If the desired property does not exist in this document, inform the user.
'Otherwise, display the value of the desired property.
If (ActiveDocument.CustomDocumentProperties(REVISION_DATE_KEY).value =
False) Then
MsgBox "The " & REVISION_DATE_KEY & " is not defined for the Active
Document.", vbOKOnly, TITLE
Else
Dim revisionDate As String
revisionDate =
ActiveDocument.CustomDocumentProperties(REVISION_DATE_KEY).value
MsgBox "The value of the " & REVISION_DATE_KEY & " is " & revisionDate &
".", vbOKOnly, TITLE
End If

End Sub
-----------------------------------------------------------------------------------------------------

I get this error message on the second If statement, the one that is
checking the value of the LAST_REVISION_DATE property: "Run-time error '5':
Invalid procedure call or argument

For what it's worth, I tried hardcoding the name of the key in the brackets
and putting quotes around it but it made no difference at all. Also, there
is an active document open within Word.

Before the statement started crashing, it worked fine for several
iterations. Prior to that, it also failed every time but those times the
error message complained about a missing object. Then, it just started
working right about the time I was getting really really frustrated. For the
life of me, I don't know why this statement has been alternating between
working fine and not working at all: I haven't changed it in hours. There's
obviously some kind of technique or practice that I'm not following which is
causing me to fall over my own feet; I just wish I could figure out what it
is....

Anyway, if you can help me figure out what's wrong here, I'd be deeply
grateful.

This code is so close to finished it hurts. I really want to finish this.

 

[Jezebel]

The problem is that your if statement is checking, not whether the value
exists, but whether the value is equal to FALSE. This throws an error if the
value is not defined at all. Simpler is to retrieve the value and trap the
error if not defined ...

On error resume next
revisionDate = ActiveDocument.CustomDocumentProperties(REVISION_DATE_KEY)
On error goto 0

If len(revisionDate) > 0 then
Msg = "The value of the " & REVISION_DATE_KEY & " is " & revisionDate &
"."
else
Msg = "The " & REVISION_DATE_KEY & " is not defined."
end if
Msgbox Msg

Separately, there's no point putting the 'Dim revisionDate' statement within
the If clause. 'Dim' is a compiler directive, not a runtime instruction, so
the variable is always declared anyway.

 

[Rhino]

See remarks interspersed below....

The problem is that your if statement is checking, not whether the value
exists, but whether the value is equal to FALSE. This throws an error if
the value is not defined at all.

As Homer Simpson would say, "D'oh!". Yeah, I should have seen that. I found
a similar fragment in a Google newsgroup search and didn't think through the
difference between what problem the post was handling and what I was doing.
I thought it was the way VBA tests for existence of a field but now I see
that it isn't.

Simpler is to retrieve the value and trap the error if not defined ...

On error resume next
revisionDate = ActiveDocument.CustomDocumentProperties(REVISION_DATE_KEY)
On error goto 0

If len(revisionDate) > 0 then
Msg = "The value of the " & REVISION_DATE_KEY & " is " & revisionDate
& "."
else
Msg = "The " & REVISION_DATE_KEY & " is not defined."
end if
Msgbox Msg

Thanks for the suggestion!

Separately, there's no point putting the 'Dim revisionDate' statement
within the If clause. 'Dim' is a compiler directive, not a runtime
instruction, so the variable is always declared anyway.

Sure, but isn't it always better to declare the variable to be a specific
type rather than let it default to variant? That's really the only reason
I'm doing it.

--
Rhino

 

[Jezebel]

Sure, but isn't it always better to declare the variable to be a specific
type rather than let it default to variant? That's really the only reason
I'm doing it.

Absolutely. I was just being lazy. My point is that the declarations should
all be at the top of the function, not interspersed with other code.

 

[Rhino]

Ahh, okay, thanks for clearing that up

Just for interest sake, why should the declarations always be at the top of
the function? Is there any real functional benefit? In Java, my main
language these days, it's really just a style thing: some programmers feel
that declarations ought to be at the top where they are more visible, others
prefer to do declarations just before the variable is needed but both
approaches work just fine.

By the way, I've got my CustomProperty-related subs working just fine thanks
to your suggestion. Thanks for your help with this (and other questions)!
[/QUOTE]
Sure, but isn't it always better to declare the variable to be a specific
type rather than let it default to variant? That's really the only reason
I'm doing it.[/QUOTE]

Absolutely. I was just being lazy. My point is that the declarations
should all be at the top of the function, not interspersed with other
code.
[/QUOTE]

 

[Jezebel]

Coding styles and conventions are methods that programmers have found,
through experience, to reduce the opportunities for bugs and to improve
maintainability. Declaring your variables is one (try writing a large app
without using 'option explicit'!); declaring them all before any code is
another.

There is one functional issue: remove the option explicit from your module,
and insert another reference to the variable *before* the declaration (which
is the sort of thing that happens when code gets modified by someone
else) -- the code runs OK if the IF condition is not met, and throws a
'duplicate declaration' error if it is.