HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Solidworks EPDM add-in

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
solidworksaddepdm

Problem

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

Solution

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

Code Snippets

If (Path.Contains(pathNPR) AndAlso Path.Contains(extNPR)) _
                OrElse (Path.Contains(pathDRR) AndAlso Path.Contains(extDRR)) _
                    Then ValidPaths.Add(Path)
If (Path.Contains(pathNPR) AndAlso Path.Contains(extNPR)) _
    OrElse (Path.Contains(pathDRR) AndAlso Path.Contains(extDRR)) Then 
        ValidPaths.Add(Path)
End If
Return (Not(input Is Nothing) AndAlso (input.Length > 0) AndAlso Not(input.GetValue(0) Is Nothing))
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

Context

StackExchange Code Review Q#98376, answer score: 3

Revisions (0)

No revisions yet.