Code smells Part II #10668

QilongTang · 2020-05-17T23:18:33Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

Fix some new code smells brought in last week..

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.

Reviewers

@DynamoDS/dynamo

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

QilongTang · 2020-05-17T23:19:28Z

src/DynamoCoreWpf/ViewModels/Watch3D/DynamoEffectsManager.cs

            }

-            public static ShaderDescription  VertexShaderDynamoMeshDescription = new ShaderDescription(nameof(VertexShaderDynamoMeshDescription), ShaderStage.Vertex,
+            public static readonly ShaderDescription  VertexShaderDynamoMeshDescription = new ShaderDescription(nameof(VertexShaderDynamoMeshDescription), ShaderStage.Vertex,


API breaking, but team agreed on Helix related work

QilongTang · 2020-05-17T23:19:36Z

src/DynamoCoreWpf/ViewModels/Watch3D/DynamoEffectsManager.cs

            }

-            public static ShaderDescription PixelShaderDynamoMeshDescription = new ShaderDescription(nameof(PixelShaderDynamoMeshDescription), ShaderStage.Pixel,
+            public static readonly ShaderDescription PixelShaderDynamoMeshDescription = new ShaderDescription(nameof(PixelShaderDynamoMeshDescription), ShaderStage.Pixel,


QilongTang · 2020-05-17T23:20:06Z

src/DynamoCoreWpf/ViewModels/Watch3D/HelixWatch3DViewModel.cs

            {
                geometryModels = Element3DDictionary
-                        .Where(x => x.Key.Contains(node.AstIdentifierGuid) && x.Value as Element3D != null).ToArray();
+                        .Where(x => x.Key.Contains(node.AstIdentifierGuid) && x.Value is Element3D).ToArray();


SQ has been back and forth about this, which made me think we may not need this type check at all

QilongTang · 2020-05-17T23:20:52Z

src/DynamoPackages/PackageManagerClient.cs

            if (!pkgResponse.success)
            {
-                throw new Exception(pkgResponse.message);
+                throw new ApplicationException(pkgResponse.message);


Please, no more System Exception thrown by user code :)

@QilongTang I know this has been merged already, but as a future reference, ApplicationException is not recommended to be used anymore. From https://docs.microsoft.com/en-us/dotnet/api/system.applicationexception?view=netcore-3.1:

You should derive custom exceptions from the Exception class rather than the ApplicationException class. You should not throw an ApplicationException exception in your code, and you should not catch an ApplicationException exception unless you intend to re-throw the original exception.

I think in this particular case, the most appropriate fix would be to throw a PackageManagerException which derives from Exception.

QilongTang · 2020-05-17T23:21:11Z

src/Engine/ProtoCore/Utils/ArrayUtils.cs

        /// <returns> true if the element was found </returns>
        public static bool GetFirstNonArrayStackValue(StackValue svArray, ref StackValue sv, RuntimeCore runtimeCore)
        {
-            RuntimeMemory rmem = runtimeCore.RuntimeMemory;


Unused local var

QilongTang · 2020-05-17T23:21:35Z

src/Libraries/PythonNodeModels/PythonNode.cs

-        protected PythonNodeBase(IEnumerable<PortModel> inPorts, IEnumerable<PortModel> outPorts) : base(inPorts, outPorts)
-        {
-            ArgumentLacing = LacingStrategy.Disabled;
-        }


Constructors should be grouped near

QilongTang · 2020-05-17T23:22:22Z

test/DynamoCoreWpfTests/NodeViewTests.cs

        public void GettingNodeNameDoesNotTriggerPropertyChangeCycle()
        {
            //add a node
-            var numNode = new CoreNodeModels.Input.DoubleInput { X = 100, Y = 100 };


ModeBase.X is obsolete, seems here we do not really need positions to be set

reddyashish · 2020-05-18T20:47:18Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

+        /// </summary>
+        /// <param name="packageDownloadHandle">package download handle</param>
+        /// <param name="downloadPath">package download path</param>
+        internal void InstallStateCheck(PackageDownloadHandle packageDownloadHandle, string downloadPath)


Minor thing: As we are assigning the package state in this method, may be we can call it something like SetPackageState.

@reddyashish Make sense. Let me update!

reddyashish · 2020-05-18T20:47:39Z

Looks good to me @QilongTang.

QilongTang · 2020-05-19T00:17:01Z

@reddyashish Addressed your comments. Merging.

QilongTang added 10 commits May 13, 2020 17:12

Initial Commit

958bf7c

Use string builder!!

2f60494

CleanUps

8da286b

Some more cleanups

43353e7

Some more smells

d4fc1b3

spelling

9c8dc45

Remove unnecessary public exposure

2d310cc

LeftOvers

3ee619f

Merge branch 'master' into CodeSmells

30aafb2

Code Smells

9608386

QilongTang commented May 17, 2020

View reviewed changes

QilongTang added 2 commits May 17, 2020 23:25

Fix unit tests

cc0f313

left overs

2a5dad8

reddyashish reviewed May 18, 2020

View reviewed changes

QilongTang added 2 commits May 18, 2020 20:08

Update function name

c38dbf1

clean up

6eec55a

QilongTang merged commit 358cd39 into master May 19, 2020

QilongTang deleted the CodeSmells branch May 19, 2020 00:17

mmisol mentioned this pull request Aug 10, 2020

Update to Dynamo Core 2.8 DynamoDS/DynamoRevit#2608

Merged

5 tasks

Code smells Part II #10668

Code smells Part II #10668

Uh oh!

Conversation

QilongTang commented May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

FYIs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmisol May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reddyashish commented May 18, 2020

Uh oh!

QilongTang commented May 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

QilongTang commented May 17, 2020 •

edited

Loading

mmisol May 19, 2020 •

edited

Loading