Add Jarvis March in C#. #85

june128 · 2018-04-19T23:50:14Z

ToDo

zsparal · 2018-04-21T22:16:51Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+
+namespace JarvisMarch
+{
+    public class Vector


Make this a struct and drop the properties. It has better performance and the properties are completely superfluous for a Vector type:

public struct Vector { public int X; public int Y; public Vector(int x, int y) { this.X = x; this.Y = y; } }

zsparal · 2018-04-21T22:23:07Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+                    var secondVector = new Vector(points[i].X - currentPoint.X, points[i].Y - currentPoint.Y);
+
+                    // Calculate the current scalar product.
+                    var tempScalarProduct = (firstVector.X * secondVector.X) + (firstVector.Y * secondVector.Y);


I'd make a dot product method on the vector. Similarly for the used operators (minus) in this case. That way this could look like:

var secondVector = points[i] - currentPoint; var tempScalarProduct = firstVector.Dot(secondVector);

zsparal · 2018-04-21T22:26:15Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+            {
+                // Search for next Point.
+                // Set the first vector, which is currentPoint -> previousPoint.
+                var firstVector = new Vector(previousPoint.X - currentPoint.X, previousPoint.Y - currentPoint.Y);


I'm not the biggest fan of this naming. First, the Point and Vector suffix is pretty redundant. Second, firstVector means nothing. I'll have to read the Jarvis march algorithm again to come up with a better name.

The name is not the best, I agree. But I don't know, if getting rid of Point and Vector is the best. I think they provide quite some clarity. If better names were used tho, the suffix might be redundant. I have to think about the names.

zsparal · 2018-04-21T22:28:45Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+
+    public static class JarvisMarch
+    {
+        public static List<Vector> RunJarvisMarch(List<Vector> points)


Redundant naming:

JarvisMarch.Run(...);

is pretty self-explanatory. If you want to follow best practices then don't make these static classes. That way you could potentially implement an interface IGiftWrapping and have multiple algorithms that you can swap out at runtime (static classes/methods make this impossible)

So just use normal classes in this case?

zsparal · 2018-04-21T22:30:31Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+        public static List<Vector> RunJarvisMarch(List<Vector> points)
+        {
+            // Set the intial point to the first point of the list.
+            var initialPoint = points[0];


I think we should have a shared library somewhere in the AAA for example with List/IEnumeable extensions methods (similarly for other languages that need it). These lines just take up a bunch of space in the implementation when by all rights it should just be:

var iniitalPoint = points.MinBy(p => p.X);

We should talk about @leios about it

Yeah, this is somewhat related to #69, but I am not sure about the best way to do this in practice.

This is actually why the language choice chapter exists. The hope was to introduce any extra libraries and such people would need to use the language for the purpose of this book, while also benchmarking the languages against each other so people could see their strengths and weaknesses.

Yeah, that's something to discuss.

june128 · 2018-04-22T02:37:11Z

Thanks for the review @Gustorn ! I will target your reviews in the following days as well :)

june128 · 2018-04-22T20:31:22Z

I added a ToDoList for this PR.

…dd ScalarProduct to Vector.

…Vector. Add mention for gustorn's help.

zsparal · 2018-04-29T21:03:28Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+{
+    public struct Vector
+    {
+        public readonly int x;


Public fields should be Pascal case

zsparal · 2018-04-29T21:05:00Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+                // Set the first vector, which is currentPoint -> previousPoint.
+                var firstVector = previousPoint - currentPoint;
+
+                Vector? nextPoint = null;


This implementation feels a bit unelegant

june128 · 2018-05-06T02:18:00Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+        }
+
+        // Returns true, if p is more left than b, if viewed from a.
+        private bool IsLeftOf(Vector a, Vector b, Vector p)


Does someone know better names for this parameters? And should I provide a more in-depth explanation of what the method does?

The comment is incorrect. It returns true if p is left of the line defined by a and b.

I mean it's somewhat correct, but far from perfect. I'll change it.

Ok, I see it. It's incorrect.

No, you were right, it's technically correct. I still think it's better to use more rigorous terms when defining operations like this.

june128 · 2018-05-06T02:18:41Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+        public static bool operator==(Vector a, Vector b) => a.Equals(b);
+        public static bool operator!=(Vector a, Vector b) => !(a == b);
+        public static Vector operator+(Vector a, Vector b) => new Vector(a.x + b.x, a.y + b.y);
+        public static Vector operator-(Vector a, Vector b) => new Vector(a.x - b.x, a.y - b.y);


Should we leave those operators in here (- and +). They are nice to have, but not necessary.

If they're not used I say remove them

zsparal · 2018-05-06T14:50:25Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+            // Set the intial point to the point of the list, where the x-position is the lowest.
+            var initialPoint = points.Aggregate((leftmost, current) => leftmost.x < current.x ? leftmost : current);
+
+            convexHull.Add(initialPoint);


Just move this inside the do-while loop (at the start), it doesn't need to be special cased

zsparal · 2018-05-06T14:53:59Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+                });
+
+                convexHull.Add(nextPoint);
+                points.Remove(nextPoint);


You don't need to remove the point if we don't care about collinear points (and the consensus was that we don't). There are some other reasons why you shouldn't have it here:

It kind of messes with the complexity of the algorithm. It won't affect big O notation but you're adding another linear search.

It's very bad manners to modify an incoming parameter

If you do want to remove the point add something like:

var remainingPoints = new HashSet<Vector>(points);

And change stuff to use that one. But again, removing the point isn't necessary.

Well, I removed the point from the points we search, since that point isn't needed anymore and won't be used anyway. But you're right with the linear search, it creates a bigger disadvantage than advantage... The question is tho, if I should go for the HashSet.

I would just not remove the points

Ok, I just exclude that now.

zsparal · 2018-05-06T14:54:33Z

chapters/computational_geometry/gift_wrapping/jarvis_march/code/cs/JarvisMarch.cs

+                    return potentialNextPoint;
+                });
+
+                convexHull.Add(nextPoint);


As said in the other comment, you can just move this to the beginning and then you don't need the Add outside of the do...while loop

This Add and the other one are in there, since the output should include the start/end-point at the beginning and at the end. If we don't want that, we can just move one Add at the beginning and remove the other one like you said.

I don't think it's very useful to include the starting point in the output. It doesn't really give you anything and it makes the algorithm a bit harder to read.

zsparal

Looks good to me, I think it's ready to be merged

leios · 2018-05-06T20:18:12Z

Thanks for all the work on this guys!

Add Jarvis March in C#.

1d9419b

zsparal reviewed Apr 21, 2018

View reviewed changes

june128 added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Apr 28, 2018

june128 assigned june128 and unassigned june128 Apr 28, 2018

june128 added 2 commits April 29, 2018 01:10

Make JarvisMarch non-static. Simplify RunJarvisMarch's name to Run. A…

66a44a9

…dd ScalarProduct to Vector.

Make Vector a struct and make fields readonly. Overload operator for …

570e493

…Vector. Add mention for gustorn's help.

zsparal reviewed Apr 29, 2018

View reviewed changes

june128 added Implementation Edit This provides an edit to an algorithm implementation. (Code and maybe md files are edited.) and removed Implementation Edit This provides an edit to an algorithm implementation. (Code and maybe md files are edited.) labels Apr 29, 2018

Refactor and simplify code. Update mention. Resolve compiler warnings.

b91141c

june128 commented May 6, 2018

View reviewed changes

Fix mention.

8f0d3af

zsparal reviewed May 6, 2018

View reviewed changes

Simplify code. Improve comments. Remove redundant operators.

7e57903

zsparal approved these changes May 6, 2018

View reviewed changes

Merge branch 'master' into addJarvisMarchCSharpPR

2919001

leios merged commit d27b044 into algorithm-archivists:master May 6, 2018

june128 deleted the addJarvisMarchCSharpPR branch May 6, 2018 23:47

Uh oh!

Add Jarvis March in C#. #85

Add Jarvis March in C#. #85

Uh oh!

Conversation

june128 commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ToDo

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!

zsparal Apr 21, 2018 • 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!

june128 commented Apr 22, 2018

Uh oh!

june128 commented Apr 22, 2018

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!

zsparal May 6, 2018 • 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!

zsparal May 6, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

zsparal left a comment

Choose a reason for hiding this comment

Uh oh!

leios commented May 6, 2018

Uh oh!

Uh oh!

june128 commented Apr 19, 2018 •

edited

Loading

zsparal Apr 21, 2018 •

edited

Loading

zsparal May 6, 2018 •

edited

Loading

zsparal May 6, 2018 •

edited

Loading