2
\$\begingroup\$

My question is whether or not this is a good practice, and/or if there is a more efficient way to do it.

I have to access this Solidworks interop object and wait for it to return some value. The return is a string. In early testing I found I ran into issues if I didn't wait for the return, so I added a "DoEvents" wait loop using While len(strProp)=0 to wait for the string to populate so I would know the call had executed and returned something.

The issue I then ran into was that when there is no value in the interop object's property, the return IS a zero-length string. So it would just loop forever, even though the call had completed.

Normally I would just use a "time-out" value - e.g. get the current time, and have an exit loop statement when a certain amount of time has passed. However, in testing I found sometimes the call could actually take over a minute to complete. Since I'm processing thousands of properties at a time, I thought I would be wasting a TON of time waiting for empty ones. So I came up with the idea of setting the string to a "nonsense" value before the call to the interop object, and then checking for when it changes to know that the call has completed.

Private Function GetModelProps(ByRef swCustPropMgr As SldWorks.CustomPropertyManager, Optional ByVal dicInput As Scripting.Dictionary) As Scripting.Dictionary
'This procedure gets the properties from a custom property manager object that is passed by the calling procedure.  It returns a dictionary object containing the properties found.

'Declare constants
    Const strNonsense           As String = "SKDjf09sf!@&#(@#$&!(*^)_!@0-12-849uj0394jfIODJS(G_3r5902-129381203892193018"  'This is just a nonsense string that will never be entered in a custom property

'Declare variables
    Dim lngX                    As Long
    Dim strProps()              As String
    Dim strProp                 As String
    Dim strPropStatement        As String
    Dim strPropVal              As String
    Dim dicProps                As Scripting.Dictionary


    On Error GoTo CleanExit

'Initialize dictionary object
    Set dicProps = New Scripting.Dictionary

    'Pass values from the input dictionary if one was provided
        If Not dicInput Is Nothing Then
                If Not IsArrayEmpty(dicInput.Keys) Then
                        For lngX = LBound(dicInput.Keys) To UBound(dicInput.Keys)
                            DicMergeOnKey dicProps, dicInput, (dicInput.Keys(lngX))
                        Next
                End If
        End If

'Attempt to get the names of all the custom properties. If there are no custom properties the array will not be initialized and populated.
    On Error Resume Next
    strProps = swCustPropMgr.GetNames
    On Error GoTo CleanExit

    'Check to see if the array is empty - if it is, then there are no custom properties to get
        If Not IsArrayEmpty(strProps) Then

            'Loop through all the custom properties
                For lngX = 0 To UBound(strProps)
                'Get the custom property name
                    strProp = strProps(lngX)
                'Set initial value to a string that will never be found
                    strPropVal = strNonsense
                'The Get2 method takes one input and returns 2 strings:  Input: Property name, Output1: Property statment, Output2: Evalutated property value
                    swCustPropMgr.Get2 strProp, strPropStatement, strPropVal

                    'Sometimes this can take a while to execute, so we use the loop below to wait until the system is done processing all other events
                        While strPropVal = strNonsense
                            DoEvents
                        Wend

                    'Make the property uppercase so we don't accidentally add multiple properties due to difference in case between documents
                        strProp = UCase(strProp)

                    'Verify item numbers before adding
                        If strProp Like "PARTNO" Then
                            strPropVal = ValidateLokItem(Trim(strPropVal))
                        End If

                    'Add the returned value to the dictionary.
                        If Len(strPropVal) > 0 Then
                            If Not dicProps.Exists(strProp) Then
                                dicProps.Add strProp, strPropVal
                            Else
                                dicProps.ITEM(strProp) = strPropVal
                            End If
                        End If
                Next
        End If

'Return the dictionary to the calling procedure
    Set GetModelProps = dicProps

CleanExit:
    Set dicProps = Nothing
    Set dicInput = Nothing
    Erase strProps
End Function
\$\endgroup\$
  • 2
    \$\begingroup\$ I encourage you to give it some time between questions. What you learn in one review will likely be applicable to other code. \$\endgroup\$ – RubberDuck May 14 '15 at 14:49
  • \$\begingroup\$ Thanks RubberDuck. First time on CodeReview. Wasn't expecting a review of the whole code, just the part in question. I edited my code sample to reflect. \$\endgroup\$ – CBRF23 May 14 '15 at 17:27
  • \$\begingroup\$ Your last edit made this stub code IMO. I rolled it back. \$\endgroup\$ – RubberDuck May 14 '15 at 17:38
2
\$\begingroup\$

I like that you're being explicit about how you're passing arguments.

Private Function GetModelProps(ByRef swCustPropMgr As SldWorks.CustomPropertyManager, Optional ByVal dicInput As Scripting.Dictionary) As Scripting.Dictionary

But I can't find an instance where swCustPropMgr is being assigned to. As such, you can pass it ByVal too and eliminate an entire class of possible future bugs from the code.

A lot of times, we can replace good comments with well named procedures. I think this is one of those cases.

'Initialize dictionary object
Set dicProps = New Scripting.Dictionary

'Pass values from the input dictionary if one was provided
    If Not dicInput Is Nothing Then
           If Not IsArrayEmpty(dicInput.Keys) Then
                   For lngX = LBound(dicInput.Keys) To UBound(dicInput.Keys)
                       DicMergeOnKey dicProps, dicInput, (dicInput.Keys(lngX))
                   Next
           End If
   End If

I'd extract the whole thing into a private function.

Set properties = InitializePropertDict(inputDict)

There are other comments like this. This one is notable.

'Attempt to get the names of all the custom properties. If there are no custom properties the array will not be initialized and populated.

In other words, TryGetProperties.

In early testing I found I ran into issues if I didn't wait for the return, so I added a "DoEvents" wait loop using While len(strProp)=0 to wait for the string to populate so I would know the call had executed and returned something.

I would look deeper into this problem, because this feels hacky. (Mostly because it is a hack to make it work.)

swCustPropMgr.Get2 strProp, strPropStatement, strPropVal

 'Sometimes this can take a while to execute, so we use the loop below to wait until the system is done processing all other events
  While strPropVal = strNonsense
      DoEvents
  Wend

I honestly don't understand why this works at all. It shouldn't. VBA is single threaded so strPropVal should either be set or not by time you reach the While loop. The call to .Get2 should block the execution from moving forward until it's completed. If it's not, consult the docs and see if that function returns any values. If it does, you might be able to force it to block by "using" that return value to set a variable.

junk = swCustPropMgr.Get2(strProp, strPropStatement, strPropVal)

Either way, the comment needs to be much more thorough and clear.

While we're here, I have a convention for variable names that are used to return values from out params like this. I think prefixing them with out makes the intention much clearer. I've been bitten by this before..

swCustPropMgr.Get2 strProp, strPropStatement, outPropVal

Some would call this Hungarian notation done right.


I actually did just look up the docs and it seems that Get2 does return void (in VBA terms, it's a Sub), so my advice isn't very practical. However, it's also been marked as Obsolete and the newest version of that method, Get4 returns a boolean. So, I'd give that a shot. They're also saying that Get4 uses caching to speed up the call, so you could see a significant performance boost as well.

\$\endgroup\$
  • \$\begingroup\$ Splitting out some of the statements into a function is something I should probably be doing more. SW changes their functions with every release, I'll look into Get4! But, I don't understand why DoEvents is "hacky". Can you recommend a non-hacky method to replace? What I found in testing was I a problem where occasionally properties weren't being captured. This was tough to troubleshoot because sometimes .Get2 executes immediately - but other times it takes upwards of 2 minutes to return. VBA does not wait for this return, which is why I added a DoEvents loop to make it wait. \$\endgroup\$ – CBRF23 May 19 '15 at 17:22
  • \$\begingroup\$ Give my answer another careful read. I explained most of that already. As for the DoEvents, it simply frees up the OS to do other things and can cause other issues. So, it is a "hack" to make VBA "wait" on the return. Which, like I said, shouldn't be happening anyway. I explained a method you can try to solve this in a cleaner way. \$\endgroup\$ – RubberDuck May 19 '15 at 17:26
  • 1
    \$\begingroup\$ @CBRF23 VBA runs on a single thread, it cannot possibly be awaiting (as in async/await) an API call. \$\endgroup\$ – Mathieu Guindon May 19 '15 at 17:26
  • 1
    \$\begingroup\$ OTOH it could be that the API itself runs async, and yields a reference to the unpopulated result. That would explain the behavior, but not why it doesn't return immediately systematically. I'd look for a way to get the results synchronously. \$\endgroup\$ – Mathieu Guindon May 19 '15 at 17:56
  • \$\begingroup\$ @RubberDuck - I plan to upgrade to the newer Get4 function, but I would still like an example of how you would handle this without using DoEvents. At the time I wrote the code, Get2 was the latest procedure available - I'd like to know how else I could have handled the wait? @Mat'sMug - I would imagine something like you describe was happening. I don't have any way to tell. All I know is that I was able observe the behavior I described first hand of VBA executing the next line before getting the return. This would happen seemingly randomly, maybe 5% of the total executions. \$\endgroup\$ – CBRF23 May 19 '15 at 22:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.