patternMinor
Solidworks EPDM add-in
Viewed 0 times
solidworksaddepdm
Problem
I need to expand the functionality of this project to encompass more commands (you may notice in
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
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 outerSolution
You shouldn't write your if blocks like this
this is cheating to make it a one lined statement to get around having to put an
It should look like this
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
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
it is a little bit longer than your version, but it is a little easier to read in my opinion
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 IfIf 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 inputArrayIsEmptyit 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 IfReturn (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 inputArrayIsEmptyContext
StackExchange Code Review Q#98376, answer score: 3
Revisions (0)
No revisions yet.