Add equals method for workplans #316

marierin · 2023-08-03T12:08:15Z

override the equals-method to compare two workplans
compare the types of the steps
compare the connectors

still missing:

compare the parameters
compare not connected elements

feature for the ticket #242

CLAassistant · 2023-08-03T12:08:27Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Toxantron · 2023-08-04T05:58:33Z

src/Moryx/Workflows/Implementation/Workplan.cs

+            }
+            Workplan newPlan = (Workplan)obj;
+
+            var start = this.Connectors.First(x => x.Name.Equals("Start"));


You can not do this based on names. This comparison needs to be more generic.

Toxantron · 2023-08-04T06:00:54Z

src/Moryx/Workflows/Implementation/Workplan.cs

+        /// </summary>
+        /// <param name="obj"></param>
+        /// <returns></returns>
+        public override bool Equals(object obj)


Please move this into a static method on Workplan.Equals(workplanA, workplanB) instead of overwriting the object method.

Toxantron · 2023-08-04T06:01:12Z

src/Moryx/Workflows/Implementation/Workplan.cs

+
+                        if (!(isAlreadyChecked))
+                        {
+                            needToCheck.Add(follower);


Use the workplanstep id instead of collection index here.

- compare the types of steps and connectors - create tests

MarisaGoergen · 2023-08-04T05:45:33Z

src/Moryx/Workflows/Implementation/Workplan.cs

+            List<IWorkplanStep> needToCheck = new List<IWorkplanStep>();
+            List<IWorkplanStep> newNeedToCheck = new List<IWorkplanStep>();
+
+            List<IWorkplanStep> check = new List<IWorkplanStep>();


Maybe checkED would be a better name

MarisaGoergen · 2023-08-04T05:46:17Z

src/Moryx/Workflows/Implementation/Workplan.cs

+
+            List<IWorkplanStep> check = new List<IWorkplanStep>();
+
+            needToCheck.Add(step);


Can be done together with the initialization `new List() {step};

MarisaGoergen · 2023-08-04T05:47:24Z

src/Moryx/Workflows/Implementation/Workplan.cs

+            {
+
+
+                for (int a = 0; a < step.Outputs.Length; a++)


This could be put in a separate method. You method is too long and unclear

MarisaGoergen · 2023-08-04T07:00:25Z

src/Moryx/Workflows/Implementation/Workplan.cs

+                        var follower = this.Steps.FirstOrDefault(x => x.Inputs.Any(y => y.Equals(connector)));
+                        var newFollower = newPlan.Steps.FirstOrDefault(x => x.Inputs.Any(y => y.Equals(newConnector)));
+
+                        bool isAlreadyChecked = (check.Contains(follower) || check.Contains(newFollower));


Why are you checking if newFollower is in check, when you only add steps of your current workplan to check?

MarisaGoergen · 2023-08-04T07:18:31Z

src/Moryx/Workflows/Implementation/Workplan.cs

+                    {
+                        var follower = this.Steps.FirstOrDefault(x => x.Inputs.Any(y => y.Equals(connector)));
+                        var newFollower = newPlan.Steps.FirstOrDefault(x => x.Inputs.Any(y => y.Equals(newConnector)));
+


Why don't you check, if the outputs are the same?

MarisaGoergen · 2023-08-04T07:19:44Z

src/Tests/Moryx.Tests/Moryx.Tests.csproj

@@ -14,6 +14,7 @@
  </ItemGroup>

  <ItemGroup>
+    <ProjectReference Include="..\..\Moryx.AbstractionLayer\Moryx.AbstractionLayer.csproj" />


Is that fine or should the test move to Moryx.AbstractionLayer.Tests @1nf0rmagician ?

Either that or alternatively the test could be limited to IWorkplanStep and reflection.

MarisaGoergen · 2023-08-04T07:19:59Z

src/Tests/Moryx.Tests/Workplans/Dummies/AssemblingActivity.cs

+    [ActivityResults(typeof(DefaultActivityResult))]
+    public class AssemblingActivity : Activity<AssemblingParameters>
+    {
+


Remove Empty lines

MarisaGoergen · 2023-08-04T07:20:21Z

src/Tests/Moryx.Tests/Workplans/Dummies/AssemblingActivity.cs

+using Moryx.AbstractionLayer;
+using Moryx.AbstractionLayer.Capabilities;
+using System;
+using System.Collections.Generic;


Check if all usings are needed

MarisaGoergen · 2023-08-04T07:21:17Z

src/Tests/Moryx.Tests/Workplans/EqualTest.cs

+        private Workplan fifthWorkplan;
+        private Workplan sixthWorkplan;
+
+        public Workplan CreateFirstWorkplan()


Quite a lot of duplicated code in the workplan creation

MarisaGoergen · 2023-08-04T07:21:44Z

src/Tests/Moryx.Tests/Workplans/EqualTest.cs

+    [TestFixture]
+    public class EqualTest
+    {
+        private Workplan firstWorkplan;


Maybe better names. What are the differences between each workplan?

MarisaGoergen · 2023-08-07T12:47:30Z

src/Moryx/Workflows/Implementation/Workplan.cs

-        /// <param name="obj"></param>
-        /// <returns></returns>
-        public override bool Equals(object obj)
+        public static bool noteSameFollowringSteps(Workplan workplan, Workplan newWorkplan ,IWorkplanStep step, IWorkplanStep newStep, List<IWorkplanStep> needToCheck, List<IWorkplanStep> newNeedToCheck, List<IWorkplanStep> isChecked)


Name is confusing. Methodnames are always written in PascalCase

MarisaGoergen · 2023-08-07T12:47:41Z

src/Moryx/Workflows/Implementation/Workplan.cs

-using System.Linq;
-using System.ComponentModel;
+using System.Linq;
+using System.Runtime.InteropServices.ComTypes;


What do you need this for?

- create an additional method to check connactions and following steps - reduce unnecessary variables - changing the sequence - rename methods in the EqualTest class - switch from a override method to a static method

marierin requested a review from MarisaGoergen August 3, 2023 12:08

marierin self-assigned this Aug 3, 2023

marierin marked this pull request as draft August 3, 2023 12:09

Toxantron requested changes Aug 4, 2023

View reviewed changes

Override the equals method of workplan including tests

28eaa67

- compare the types of steps and connectors - create tests

marierin force-pushed the feature/addEqualsMethodForWorkplans branch from 98e2667 to 28eaa67 Compare August 4, 2023 06:56

MarisaGoergen suggested changes Aug 4, 2023

View reviewed changes

MarisaGoergen reviewed Aug 7, 2023

View reviewed changes

marierin added 2 commits August 8, 2023 07:58

Update after merge request feedback

59fd6d1

- create an additional method to check connactions and following steps - reduce unnecessary variables - changing the sequence - rename methods in the EqualTest class - switch from a override method to a static method

Create class for parameters and optimize if-conditions

2742aa4

marierin force-pushed the feature/addEqualsMethodForWorkplans branch from 8aadc89 to 2742aa4 Compare August 8, 2023 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add equals method for workplans #316

Add equals method for workplans #316

marierin commented Aug 3, 2023

CLAassistant commented Aug 3, 2023

Toxantron Aug 4, 2023

Toxantron Aug 4, 2023

Toxantron Aug 4, 2023

MarisaGoergen Aug 4, 2023

MarisaGoergen Aug 4, 2023

MarisaGoergen Aug 4, 2023

MarisaGoergen Aug 4, 2023

MarisaGoergen Aug 4, 2023

MarisaGoergen Aug 4, 2023

Toxantron Aug 4, 2023

MarisaGoergen Aug 4, 2023

MarisaGoergen Aug 4, 2023

MarisaGoergen Aug 4, 2023

MarisaGoergen Aug 4, 2023

MarisaGoergen Aug 7, 2023

MarisaGoergen Aug 7, 2023


		List<IWorkplanStep> check = new List<IWorkplanStep>();

		needToCheck.Add(step);

Add equals method for workplans #316

Are you sure you want to change the base?

Add equals method for workplans #316

Conversation

marierin commented Aug 3, 2023

CLAassistant commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment