4
\$\begingroup\$

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:

Project settings

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
    Implements IEdmAddIn5

    Private Structure MenuCommand
        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

    Private Sub DefineMenuCommands()
        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
            .MenuFlags = EdmMenuFlags.EdmMenu_MustHaveSelection + EdmMenuFlags.EdmMenu_OnlyFiles + EdmMenuFlags.EdmMenu_OnlyInContextMenu
            .CreateReferences = True
            Commands.Add(.Index, NewDRR)
        End With

        NewNPR = New MenuCommand
        With NewNPR
            .Index = 1
            .CommandName = "Create new New Product Request"
            .ExecutingTemplate = "New New Product Request (NPR)"
            .ValidatorIndex = ValidatorIndex.NewNPR
            .MenuFlags = EdmMenuFlags.EdmMenu_MustHaveSelection + EdmMenuFlags.EdmMenu_OnlyFiles + EdmMenuFlags.EdmMenu_OnlyInContextMenu
            .CreateReferences = True
            Commands.Add(.Index, NewNPR)
        End With

        NewECN = New MenuCommand
        With NewECN
            .Index = 2
            .CommandName = "Issue new Engineering Change Notice"
            .ExecutingTemplate = "New Engineering Change Notice (ECN)"
            .ValidatorIndex = ValidatorIndex.NewECN
            .MenuFlags = EdmMenuFlags.EdmMenu_MustHaveSelection + EdmMenuFlags.EdmMenu_OnlyFiles + EdmMenuFlags.EdmMenu_OnlyInContextMenu
            .CreateReferences = True
            Commands.Add(.Index, NewECN)
        End With
    End Sub

    Public Sub GetAddInInfo(ByRef poInfo As EdmAddInInfo,
                            ByVal poVault As IEdmVault5,
                            ByVal poCmdMgr As IEdmCmdMgr5) Implements IEdmAddIn5.GetAddInInfo

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

            '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."
                .mlAddInVersion = GetVersionAsInt()

                '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
                    poCmdMgr.AddCmd(lCmdID:=.Index,
                                    bsMenuString:=.CommandName,
                                    lEdmMenuFlags:=.MenuFlags)
                End With
            Next

        Catch ex As Runtime.InteropServices.COMException
            Select Case ex.ErrorCode
                Case EPDM.Interop.EPDMResultCode.EdmResultSuccessCodes_e.S_EDM_32BIT_ADDIN
                    MessageBox.Show("Succesfully registered 32bit addin.")
                Case EPDM.Interop.EPDMResultCode.EdmResultSuccessCodes_e.S_EDM_64BIT_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)) _
                                    Then ValidPaths.Add(Path)
                        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
                SourceVault.LoginAuto(SourceVault.Name, SourceCommand.mlParentWnd)
            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 " &
                                "" & SourceVault.Name & "" & ".  Please contact the EPDM administrator.")

        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
                    Case EdmCmdType.EdmCmd_Menu
                        If SelectedItem.mlObjectID1 > 0 Then
                            Dim SelectedFile As IEdmFile6 = SourceVault.GetObject(EdmObjectType.EdmObject_File, SelectedItem.mlObjectID1)
                            FilePaths.Add(SelectedFile.GetLocalPath(SelectedItem.mlObjectID3))
                            SelectedFile = Nothing
                        End If
                    Case EdmCmdType.EdmCmd_CardButton
                        If SelectedItem.mlObjectID1 > 0 Then
                            FilePaths.Add(SelectedItem.mbsStrData2)
                        End If
                End Select

            Catch ex As Exception
                ExceptionBuffer.Add(ex)
            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
            Dim ReferenceBuilder As IEdmAddCustomRefs = SourceVault.CreateUtility(EdmUtility.EdmUtil_AddCustomRefs)

            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
                    ReferenceBuilder.AddReferencesPath(ParentFile.ID, ReferenceFiles.ToArray)

                    '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
                    ExceptionBuffer.Add(ex)
                End Try
            Next
        Catch ex As Exception
            MessageBox.Show(ex.ToString)
        End Try

    End Sub

End Class
\$\endgroup\$
  • \$\begingroup\$ 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 :) \$\endgroup\$ – CBRF23 Jul 29 '15 at 23:24
  • \$\begingroup\$ 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. \$\endgroup\$ – RubberDuck Aug 19 '15 at 10:49
  • \$\begingroup\$ 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 :) \$\endgroup\$ – CBRF23 Aug 19 '15 at 15:38
3
\$\begingroup\$

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)) _
                    Then ValidPaths.Add(Path)

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 
        ValidPaths.Add(Path)
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

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – CBRF23 Aug 18 '15 at 21:30
  • \$\begingroup\$ Typed my comment before your edit - the info on .Length is good to have. Thanks! \$\endgroup\$ – CBRF23 Aug 18 '15 at 21:31
  • \$\begingroup\$ 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. \$\endgroup\$ – Malachi Aug 18 '15 at 21:34
  • \$\begingroup\$ 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 :) \$\endgroup\$ – CBRF23 Aug 18 '15 at 21:36
1
\$\begingroup\$

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
                ExceptionBuffer.Add(ex)
            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.

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – CBRF23 Jul 29 '15 at 12:20
  • \$\begingroup\$ 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? \$\endgroup\$ – CBRF23 Jul 29 '15 at 12:23
  • \$\begingroup\$ 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. \$\endgroup\$ – RubberDuck Jul 29 '15 at 14:22
  • \$\begingroup\$ 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. \$\endgroup\$ – CBRF23 Jul 29 '15 at 17:45
  • \$\begingroup\$ And you're repeating yourself every step of the way. DRY it out. \$\endgroup\$ – RubberDuck Jul 29 '15 at 21:55
1
\$\begingroup\$
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
        .MenuFlags = EdmMenuFlags.EdmMenu_MustHaveSelection + EdmMenuFlags.EdmMenu_OnlyFiles + EdmMenuFlags.EdmMenu_OnlyInContextMenu
        .CreateReferences = True
        Commands.Add(.Index, NewDRR)
    End With

    NewNPR = New MenuCommand
    With NewNPR
        .Index = 1
        .CommandName = "Create new New Product Request"
        .ExecutingTemplate = "New New Product Request (NPR)"
        .ValidatorIndex = ValidatorIndex.NewNPR
        .MenuFlags = EdmMenuFlags.EdmMenu_MustHaveSelection + EdmMenuFlags.EdmMenu_OnlyFiles + EdmMenuFlags.EdmMenu_OnlyInContextMenu
        .CreateReferences = True

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
Commands.Add(NewDDR.Index, NewDDR)

NewNPR = New NPRCommand
Commands.Add(NewNPR.Index, NewNPR)

' ...
\$\endgroup\$

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.