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

  </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


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

		needToCheck.Add(step);

Uh oh!

Add equals method for workplans #316

Are you sure you want to change the base?

Add equals method for workplans #316

Uh oh!

Conversation

marierin commented Aug 3, 2023

Uh oh!

CLAassistant commented Aug 3, 2023

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants