# Solidworks EPDM add-in

I need to expand the functionality of this project to encompass more commands (you may notice in GetSelectedFilePaths() I have a case that is not currently used, this is to support additional commands I plan to add in my next sprint). So, before going further with this project I thought I'd post it up here for review.

General Overview:

Solidworks EPDM is a data management tool. It is essentially a shell extension which uses a database to store file data/metadata and act as a file vault. Here is the API documentation.

EPDM add-ins are mostly used to extend functionality by adding additional menu commands, toolbar buttons, buttons on data cards, etc. which can run procedures from the add-in.

My add-in so far encompasses just a couple menu commands. I chose these commands to do first as they are similar and related, but I plan to extend this add-in with more commands, menu buttons, etc. in the next development sprint.

Project Info:

What I hope to get from this review:

I'm particularly interested in how/where I can refactor this for improved readability and/or performance before I continue to extend this add-in. My main concern is readability for future maintenance (I'm not a programmer by trade, so once this is finished it might be a year before I jump back into it or even do any coding), but I don't want to sacrifice major performance gains for the sake of readability.

I've done about 3 refactorings now myself, and it's coming along well I think, but I'm sure it could be better as I'm no expert in programming.

Particularly, I think there is room for improvement in:

• Field/property use

I suspect my field/property structure could be better. E.g. I have a lot of private properties which maybe would be better as fields? I tried to follow the guidelines here, but I suspect I may be better off using more fields and less private properties.

• VerifiedPaths property

I considered making RunCommand (currently a local variable within the OnCmd outer Try block) a Private Property, then making the Get procedure for VerifiedPaths call the GetValidSelectionPaths procedure using RunCommand.ValidatorIndex but I'm not sure if that would be better or worse for readability and efficiency.

• Use of properties and the ResetMyProperties procedure

I'm using properties of this class to hold data used by the given command, and then resetting it afterwards so each property is ready for the next command received. I'm not sure if this is good practice, or if I'd be better off splitting this into another class to act as a variable container.

• General "chunking" of procedures

Not really sure how to describe this, but basically am I using enough small procedures? Too many? Could I make this simpler by doing more/less chunking off of the work done?

• Exception capturing/handling

I'm still new to this, so I could probably be doing better here. I'm not entirely sure what exceptions to expect in most cases, so I'm basically just finding them as I debug and then adding ways to eliminate or handle them as I find them. As a result, my exception trapping and handling could probably be setup better or be more comprehensive.

Imports System.Windows.Forms
Imports EPDM.Interop.EPDMResultCode
Imports EdmLib
Imports System.Reflection
Imports System.Runtime.CompilerServices

Module MyExtensions
<Extension()>
Public Function IsAllocated(ByVal input As System.Array) As Boolean
Return Not ((input Is Nothing) OrElse (input.Length = 0) OrElse (input.GetValue(0) Is Nothing))
End Function
End Module

Public Class EngChangeProcess

Dim Index As Long
Dim CommandName As String
Dim MenuFlags As Integer
Dim ExecutingTemplate As String
Dim ValidatorIndex As Long
Dim CreateReferences As Boolean
End Structure

Private Enum ValidatorIndex
NewDRR
NewNPR
NewECN
End Enum

Private Const pathDRR As String = "\_DRR\"
Private Const pathNPR As String = "\_NPR\"
Private Const pathECRN As String = "\_ECRN\"
Private Const extDRR As String = ".LokDRR"
Private Const extNPR As String = ".LokNPR"
Private Const extECRN As String = ".LokECRN"

Private NewDRR As MenuCommand
Private NewNPR As MenuCommand
Private NewECN As MenuCommand
Private Commands As Dictionary(Of Long, MenuCommand)

Private mVault As EdmVault5
Private mCommand As EdmCmd
Private mSelected As System.Array
Private mTemplate As System.String
Private mPaths As List(Of String)

Private Property SourceVault As EdmVault5
Get
Return mVault
End Get
Set(value As EdmVault5)
mVault = value
End Set

End Property
Private Property SourceCommand As EdmCmd
Get
Return mCommand
End Get
Set(value As EdmCmd)
mCommand = value
End Set
End Property
Private Property SourceSelection As System.Array
Get
Return mSelected
End Get
Set(value As System.Array)
mSelected = value
End Set
End Property
Private Property TargetTemplate As System.String
Get
Return mTemplate
End Get
Set(value As System.String)
mTemplate = value
End Set
End Property
Private Property VerifiedPaths As List(Of String)
Get
Return mPaths
End Get
Set(value As List(Of String))
mPaths = value
End Set
End Property

Private Sub ResetMyProperties()
SourceVault = Nothing
SourceCommand = Nothing
SourceSelection = Nothing
TargetTemplate = String.Empty
VerifiedPaths = Nothing
End Sub

If Commands Is Nothing Then Commands = New Dictionary(Of Long, MenuCommand)
If Commands.ToArray.IsAllocated Then Exit Sub

NewDRR = New MenuCommand
With NewDRR
.Index = 0
.CommandName = "Create new Document Revision Request"
.ExecutingTemplate = "New Document Revision Request (DRR)"
.ValidatorIndex = ValidatorIndex.NewDRR
.CreateReferences = True
End With

NewNPR = New MenuCommand
With NewNPR
.Index = 1
.CommandName = "Create new New Product Request"
.ExecutingTemplate = "New New Product Request (NPR)"
.ValidatorIndex = ValidatorIndex.NewNPR
.CreateReferences = True
End With

NewECN = New MenuCommand
With NewECN
.Index = 2
.CommandName = "Issue new Engineering Change Notice"
.ExecutingTemplate = "New Engineering Change Notice (ECN)"
.ValidatorIndex = ValidatorIndex.NewECN
.CreateReferences = True
End With
End Sub

ByVal poVault As IEdmVault5,

Try
'Make sure menu command structures are ready to use:

'Specify add-in information for EPDM to display
With poInfo
.mbsAddInName = "Engineering Document Change Management Add-in by Ssoeder"
.mbsCompany = "Lokring Technology LLC"
.mbsDescription = "This add-in provides the functionality for the Engineering document change process."

'Specify minimum version of SolidWorks Enterprise PDM
.mlRequiredVersionMajor = 15
.mlRequiredVersionMinor = 1
End With

'Register menu commands with the EPDM shell extension.
'The command ID (lCmdID) is provided to OnCmd when the command is executed
For Each kvp In Commands
With kvp.Value
End With
Next

Catch ex As Runtime.InteropServices.COMException
Select Case ex.ErrorCode
MessageBox.Show("Succesfully registered 32bit addin.")
MessageBox.Show("Successfully registered 64bit addin")
Case Else
MessageBox.Show("An unhandled COMException has ocurred.  Details: " & vbNewLine & _
"HRESULT = 0x" & ex.ErrorCode.ToString("X") & ": " & ex.Message)
End Select
Catch ex As Exception
MessageBox.Show(ex.ToString)
End Try
End Sub

Public Sub OnCmd(ByRef poCmd As EdmCmd, ByRef ppoData As System.Array) Implements IEdmAddIn5.OnCmd
Try
'Set private properties to hold the data passed with the command
SourceVault = poCmd.mpoVault
SourceCommand = poCmd
SourceSelection = ppoData

'Get the command structure for the command ID
Dim RunCommand As MenuCommand = Commands(poCmd.mlCmdID)

'Verify the selection meets requirements of the command
VerifiedPaths = GetValidSelectionPaths(RunCommand.ValidatorIndex)
If Not VerifiedPaths.ToArray.IsAllocated Then Throw New Exception("No valid files selected.  Cannot execute command.")

'Set the template to execute
TargetTemplate = RunCommand.ExecutingTemplate

'Execute the template
TryRunTemplate(CreateReferences:=RunCommand.CreateReferences)

Catch ex As Exception
MessageBox.Show(ex.ToString)
Finally
ResetMyProperties()
End Try

MessageBox.Show("Complete")
End Sub

Private Function GetValidSelectionPaths(ByRef Index As ValidatorIndex) As List(Of String)
Try
Dim Exceptions As New List(Of Exception)
Dim RawPaths As List(Of String) = GetSelectedFilePaths(Exceptions)

For Each ex As Exception In Exceptions
MessageBox.Show(ex.ToString)
Next

Select Case Index
Case ValidatorIndex.NewDRR, ValidatorIndex.NewNPR
'Just make sure we have at least one file
If RawPaths.Count > 0 Then Return RawPaths
Case ValidatorIndex.NewECN
'We want to verify we have at least one NPR or DRR file to be the target of the ECN
Try
Dim ValidPaths As New List(Of String)
For Each Path As String In RawPaths
If (Path.Contains(pathNPR) AndAlso Path.Contains(extNPR)) _
OrElse (Path.Contains(pathDRR) AndAlso Path.Contains(extDRR)) _
Next
If ValidPaths.Count > 0 Then Return ValidPaths
Catch ex As Exception
MessageBox.Show(ex.ToString)
End Try
End Select
Catch ex As Exception
MessageBox.Show(ex.ToString)
End Try

Return Nothing

End Function

Private Function GetVersionAsInt() As Integer
Dim v As Version = Assembly.GetExecutingAssembly().GetName().Version()

Debug.Assert(v.Major >= 0 And v.Major < 100)
Debug.Assert(v.Minor >= 0 And v.Minor < 100)
Debug.Assert(v.Build >= 0 And v.Build < 10000)
Debug.Assert(v.Revision >= 0 And v.Revision < 100)

Dim longVersion As Long = v.Major * 100000000L + _
v.Minor * 1000000L + _
v.Build * 100L + _
v.Revision

Return CInt(longVersion)
End Function

Private Sub TryRunTemplate(Optional ByVal CreateReferences As Boolean = True)
'Must be logged into the vault to run the template
Try
If Not SourceVault.IsLoggedIn Then
End If
Catch ex As Exception
MessageBox.Show(ex.ToString, Application.ProductName, MessageBoxButtons.OK, MessageBoxIcon.Exclamation)
End Try

'Run the template and get the files created
Dim NewFiles As List(Of String) = GetFilesFromTemplate()
Try
If Not NewFiles.ToArray.IsAllocated Then Throw New Exception("No files returned by" & "" & TargetTemplate & """." & vbNewLine & "Please contact the EPDM administrator.")
Catch ex As Exception
MessageBox.Show(ex.ToString)
End Try

'Create the references if required
If CreateReferences Then
Try
Dim Exceptions As New List(Of Exception)
CreateTheReferences(NewFiles:=NewFiles,
ReferenceFiles:=VerifiedPaths,
ExceptionBuffer:=Exceptions)

If Exceptions.ToArray.IsAllocated Then
For Each ex As Exception In Exceptions
MessageBox.Show(ex.ToString)
Next
End If
Catch ex As Exception
MessageBox.Show(ex.ToString)
End Try
End If
End Sub

Private Function GetFilesFromTemplate() As List(Of String)

Try
Dim TheTemplate As IEdmTemplate53 = GetTemplate()
Dim DataReturn As System.Array

'Execute the template
Dim RefreshFlag As Long = TheTemplate.RunEx(hParentWnd:=SourceCommand.mlParentWnd,
lCurrentFolderID:=SourceVault.RootFolderID,
ppoRetData:=DataReturn)

'Refresh the folder view if required
If RefreshFlag = EdmRefreshFlag.EdmRefresh_FileList Then SourceVault.RefreshFolder(SourceVault.RootFolderPath)

'Check that the execution returned some data
If Not DataReturn.IsAllocated Then Throw New Exception("No data was returned from template " &
"" & TargetTemplate & "" & "." & vbNewLine &
"Contact the EPDM administrator if you continue to have problems.")

'Return the path(s) of the newly created file(s)
Dim NewFilePaths As New List(Of String)
Dim path As String = String.Empty
For Each data As EdmData In DataReturn
Select Case data.Type
Case EdmDataType.EdmData_File
path = data.Get(EdmDataPropertyType.EdmProp_Path)
If path.Length > 0 Then NewFilePaths.Add(path)
path = String.Empty
End Select
Next

Return NewFilePaths

Catch ex As Exception
MessageBox.Show(ex.ToString)
Return Nothing
End Try

End Function

Private Function GetTemplate() As IEdmTemplate53
Try
Dim TheTemplate As IEdmTemplate53 = Nothing
Dim TemplateMgr As IEdmTemplateMgr5 = SourceVault.CreateUtility(EdmUtility.EdmUtil_TemplateMgr)
Dim Position As IEdmPos5 = TemplateMgr.GetFirstTemplatePosition
Dim Match As Boolean = False

While (Not Position.IsNull) And (Not Match)
TheTemplate = TemplateMgr.GetNextTemplate(Position)
If TheTemplate.GetMenuString.ToLower = TargetTemplate.ToLower Then Match = True
End While

If Match Then Return TheTemplate

Throw New Exception("Template " & "" & TargetTemplate & "" & " was not found on the vault " &

Catch ex As Exception
MessageBox.Show(ex.ToString)
End Try

Return Nothing
End Function

Private Function GetSelectedFilePaths(ByRef ExceptionBuffer As List(Of Exception)) As List(Of String)
Dim FilePaths As New List(Of String)
For Each SelectedItem As EdmCmdData In SourceSelection

'Reference: http://help.solidworks.com/2015/english/api/epdmapi/epdm.interop.epdm~epdm.interop.epdm.edmcmddata.html
Try
Select Case SourceCommand.meCmdType
If SelectedItem.mlObjectID1 > 0 Then
Dim SelectedFile As IEdmFile6 = SourceVault.GetObject(EdmObjectType.EdmObject_File, SelectedItem.mlObjectID1)
SelectedFile = Nothing
End If
Case EdmCmdType.EdmCmd_CardButton
If SelectedItem.mlObjectID1 > 0 Then
End If
End Select

Catch ex As Exception
End Try
Next
Return FilePaths
End Function

Private Sub CreateTheReferences(ByVal NewFiles As List(Of String),
ByVal ReferenceFiles As List(Of String),
ByRef ExceptionBuffer As List(Of Exception))

Try

For Each NewPath As String In NewFiles
Try
Dim ParentFile As IEdmFile6 = SourceVault.GetFileFromPath(NewPath)
Dim ParentFolder As IEdmFolder6 = SourceVault.GetFolderFromPath(Left(NewPath, InStrRev(NewPath, "\")))
Dim UnlockLater As Boolean = False

With ParentFile
If Not .IsLocked Then
UnlockLater = True
.LockFile(ParentFolder.ID, SourceCommand.mlParentWnd, CInt(EdmLockFlag.EdmLock_Simple))
End If
End With

'Add the file references - they are not committed to the database yet at this step

'Compute the reference tree.  Returns True or False to indicate if the method executed successfully
Dim Success As Boolean = ReferenceBuilder.CreateTree(0)

#If CONFIG = "Debug" Then
'When debugging, it can be helpful to show the reference dialog before comitting the references:
Success = ReferenceBuilder.ShowDlg(SourceCommand.mlParentWnd)
#End If

'Commit the references to the database.  Returns True or False to indicate if the method executed successfully
If Success Then Success = ReferenceBuilder.CreateReferences()

'Throw a new exception explaining that references couldn't be added to the file.
If Not Success Then Throw New Exception("ERROR: Could not create the file reference(s) on " &
"" & ParentFile.Name & "" & "." & vbNewLine &
"Check the template to ensure it is NOT set to check-in newly created files." &
vbNewLine & "If still having problems after ensuring files are not being checked-in " &
"then contact the EPDM administrator.")

'Refreshes the shell folder view.
SourceVault.RefreshFolder(ParentFolder.LocalPath)

If UnlockLater Then ParentFile.UnlockFile(lParentWnd:=SourceCommand.mlParentWnd,
bsComment:="System: Added reference(s) for " & String.Join(";", ReferenceFiles.ToArray) & " created by " & TargetTemplate,
lEdmUnlockFlags:=0)

Catch ex As Exception
End Try
Next
Catch ex As Exception
MessageBox.Show(ex.ToString)
End Try

End Sub

End Class

• I don't know why RubberDuck deleted his answer - there was good advice in there which led me down the path of looking more closely at generic error handling and I found this MSDN entry on How to Log Exceptions which I think I will implement in my refactor :) – CBRF23 Jul 29 '15 at 23:24
• I deleted it in a moment of frustration, feeling like you hadn't gotten anything from it. I've undeleted now that I see that wasn't the case. My apologies. – RubberDuck Aug 19 '15 at 10:49
• No worries - lots good from your comments - I started a dialog on programmers about passing errors back the EPDM program, figured out how to add error logging, and implemented all of this in my refactor :) – CBRF23 Aug 19 '15 at 15:38

You shouldn't write your if blocks like this

            If (Path.Contains(pathNPR) AndAlso Path.Contains(extNPR)) _
OrElse (Path.Contains(pathDRR) AndAlso Path.Contains(extDRR)) _


this is cheating to make it a one lined statement to get around having to put an End If afterwards.

It should look like this

If (Path.Contains(pathNPR) AndAlso Path.Contains(extNPR)) _
OrElse (Path.Contains(pathDRR) AndAlso Path.Contains(extDRR)) Then
End If


If you have a negative length that is obviously not right, so Length should be greater than zero.

Important Note

Unfortunately, with an extension method it is legal to pass no parameter (or nothing) so you still need to check to see if "nothing" was passed into the function.

I do, however, recommend that you form the return condition like this

Return (Not(input Is Nothing) AndAlso (input.Length > 0) AndAlso Not(input.GetValue(0) Is Nothing))


instead of what you had, this way you can clearly see that one piece can dump out a false and return that to the function call. something to make this more readable would be to do it like this

Dim inputIsNothing As boolean : Set inputIsNothing = input Is Nothing
Dim inputIsArray As boolean : Set inputIsMoreThanNothing = input.Length > 0
Dim inputArrayIsEmpty As boolean : Set inputArrayIsEmpty = input.GetValue(0) Is Nothing

Return Not inputIsNothing AndAlso inputIsArray AndAlso Not inputArrayIsEmpty


it is a little bit longer than your version, but it is a little easier to read in my opinion

• What's the reason why it's not acceptable to do the first way? I find it easier to read the first way because through the use of line continuations I can place the operands at the beginning of the line, which I think makes them easier to find and in turn easier to understand what the code is doing. In your example I can see the last line before the End If statement and then I have to look up and figure out if that line is the beginning of the executing code statement, or a continuation of the conditional requirements. – CBRF23 Aug 18 '15 at 21:30
• Typed my comment before your edit - the info on .Length is good to have. Thanks! – CBRF23 Aug 18 '15 at 21:31
• Then isn't an operand though. it's like the opening brace of the if statement (if we were in C#). I couldn't see where the end of the if statement was, and that could be a problem to future developers or even to you in the future when you come back to the code in a couple of months. – Malachi Aug 18 '15 at 21:34
• Sorry, my bad on the terminology! I can see your point; I think the best solution then is a little vertical white space between Then and End If :) – CBRF23 Aug 18 '15 at 21:36

I have a lot of private properties which maybe would be better as fields?

Well, here is one of your private properties. (They all look like this though.)

Private Property SourceCommand As EdmCmd
Get
Return mCommand
End Get
Set(value As EdmCmd)
mCommand = value
End Set
End Property


You're not really getting any benefit from them. It would be much simpler to just use a private field instead. If you instead had some logic that was attached to the field that must be called each time you accessed mCommand, then it would be worth it. For example:

Private Property SourceCommand As EdmCmd
Get
Return If(mCommand Is Nothing, new EdmCmd, mCommand)
End Get
Set(value As EdmCmd)
mCommand = value
End Set
End Property


Always use properties for Public access to instance variables though. All in all though, it doesn't hurt to use Private properties. It does insulate you from changes to their internals down the road. It's a judgment call really.

I'm not entirely sure what exceptions to expect in most cases, so I'm basically just finding them as I debug and then adding ways to eliminate or handle them as I find them. As a result, my exception trapping and handling could probably be setup better or be more comprehensive.

This has been my exact approach to exception handling in .Net and it's served me well so far. There are times when you just know that you may get a NullReferenceException or an IndexOutOfBounds, but generally speaking, you really don't know what exceptions are going to happen until they do.

What you shouldn't be doing is this.

            Catch ex As Exception
End Try
Next
Catch ex As Exception
MessageBox.Show(ex.ToString)
End Try


First off, you shouldn't really be catching Exception if you can help it. That is literally swallowing every possible exception and showing the exception text to the end user. Also, you've repeated the "Catch Ex, show messagebox" code all over.

I understand the desire (need?) to have one place in the code to handle any exception that could happen in your addin so you don't crash the host. I've done it myself. Ideally, your add-in would create an instance of an "app" or "plugin" class.This class would have a Main routine and in that routine should be the only place "Catch Ex, Show Messagebox" is at. All other code underneath it should either handle a specific exception, or none at all. Then, if an unexpected exception does happen, it bubbles all the way up until it hits the catch in Main.

I didn't get a chance to do a deep dive into your code. I'm sure there much else to say, but I'll have to leave it to another reviewer.

• RubberDuck - thank you for your time and feedback! With regards to the Catch statements you quoted: I want to allow the code to try to process all the files in the list, I don't want it to break on exceptions, but I also don't want to sweep exceptions under the rug. E.g. if there are 99 files selected, and I get an exception on file number 50, I don't want to end up with half files processed, and half not - which is why I added that list of exceptions, which can then be dealt with from the calling procedure. How could I better handle exceptions in the loop, without breaking the loop? – CBRF23 Jul 29 '15 at 12:20
• I wanted to add, I do understand what you are saying about the Main() procedure having the only Catch...Exception statement and handling only specific exceptions in sub-procedures. My question is specifically about how then would I prevent the loop from breaking on un-expected exception? – CBRF23 Jul 29 '15 at 12:23
• If an actual Exception happens, you want the loop to break. You don't want it to continue on thinking everything is great when, potentially, there are catastrophic exceptions happening. This SO Q&A would be a great read for you. – RubberDuck Jul 29 '15 at 14:22
• I read the questionyou linked. I wouldn't say I'm swallowing exceptions, and I think that what I'm doing would fall under the realm of catching and "failing gracefully" - I'm allowing for the possibility of the loop to fail, I let it fail, catch the exception, and try to process the rest of the files through more loop iterations. I'm not removing or hiding any information, and from what I can see in testing - ex.ToString shows all the information you would get if you let the exception "bubble up" naturally, I've just chosen to control when/how failures affect my program. – CBRF23 Jul 29 '15 at 17:45
• And you're repeating yourself every step of the way. DRY it out. – RubberDuck Jul 29 '15 at 21:55
Private Enum ValidatorIndex
NewDRR
NewNPR
NewECN
End Enum

Private Const pathDRR As String = "\_DRR\"
Private Const pathNPR As String = "\_NPR\"
Private Const pathECRN As String = "\_ECRN\"
Private Const extDRR As String = ".LokDRR"
Private Const extNPR As String = ".LokNPR"
Private Const extECRN As String = ".LokECRN"

Private NewDRR As MenuCommand
Private NewNPR As MenuCommand
Private NewECN As MenuCommand

' ...

NewDRR = New MenuCommand
With NewDRR
.Index = 0
.CommandName = "Create new Document Revision Request"
.ExecutingTemplate = "New Document Revision Request (DRR)"
.ValidatorIndex = ValidatorIndex.NewDRR
.CreateReferences = True
End With

NewNPR = New MenuCommand
With NewNPR
.Index = 1
.CommandName = "Create new New Product Request"
.ExecutingTemplate = "New New Product Request (NPR)"
.ValidatorIndex = ValidatorIndex.NewNPR

The enum here isn't a bad way to go, but it could be better if you introduced some OOP. Create a base class that inherits from MenuCommand. In this base class, you'll set the shared properties (like MenuFlags) in the ctor. Then, create a new subclass for each of your menu commands. In the end, you should get some code that looks something like this.
NewDDR = New DDRCommand