Skip to content

Jarvis March Implementation in Java #354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Oct 10, 2018
Merged

Jarvis March Implementation in Java #354

merged 5 commits into from
Oct 10, 2018

Conversation

crafter312
Copy link
Contributor

@crafter312 crafter312 commented Aug 13, 2018 •
edited
Loading

It works, but I could use some tips on how to improve it!
@Gathros Gathros added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Aug 13, 2018
Trashtalk217
Trashtalk217 reviewed Sep 11, 2018
View reviewed changes
Copy link
Contributor

@Trashtalk217 Trashtalk217 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't take this criticism too hard and please call me out on bullshit if I'm just wrong.
contents/jarvis_march/code/java/JarvisMarch.java Outdated
}
}

public static ArrayList<Integer[]> jarvisMarch(ArrayList<Integer[]> arr) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda feel like this should be encapsulated in a private method named getLeftestPoint, but you really only need it once so it's not like that will safe you much space. Discussion is open on this one...
Copy link
Contributor Author

@crafter312 crafter312 Oct 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The part of my code that finds the leftmost point is small, so I'll add a comment labeling it. In fact, I'll just go through and add more comments.
@leios
Copy link
Member

leios commented Sep 21, 2018

So it looks like this one was mostly reviewed by @Trashtalk217 , what is the current state of this PR @crafter312 ?
@crafter312
Copy link
Contributor Author

I’m slowly doing work on it. Haven’t had much time though because i’m currently working on my college applications, but i’ll try finish my edits within the next couple weeks. I’ve been reading about the Vector2D library that @Trashtalk217 mentioned. Do you know if there is a good alternative using the default java libraries?
@leios
Copy link
Member

leios commented Sep 22, 2018

No rush here! I was just checking in.

Unfortunately, I don't know Java and using an external library might be useful in this case. I am not honestly sure the best way to progress there.
@Trashtalk217
Copy link
Contributor

Trashtalk217 commented Sep 23, 2018 •
edited
Loading

@crafter312
I've looked into the library issue and there are a couple of things I found:
The javax library is maintained by Oracle and those are also the guys maintaining Java itself, making it a pretty valid argument for using the library.
However Vector2D and the whole of javax is not part of the core, meaning it requires some extra setup for a theoretical project where someone has to download and setup javax. Another option could be making a small Vector2D class for yourself like this:

Class Vector2D { float x, y public float angle(Vector2D) { //etc }

I'm kinda on the fence on which to choose, which one do you prefer?
@crafter312
Copy link
Contributor Author

I'm leaning towards creating my own smaller Vector2D class. If I did, would the best way be to make it a nested class so people can see it as part of the code example?
@Wesley-Arrington
Copy link
Contributor

I support the idea of using a nested class. I think it would be the best option to keep the code approachable and concise
@crafter312 crafter312 changed the title Jarvis March Implementation in Java WIP Jarvis March Implementation in Java Oct 1, 2018
@crafter312 crafter312 changed the title WIP Jarvis March Implementation in Java Jarvis March Implementation in Java Oct 1, 2018
Yordrar
Yordrar reviewed Oct 4, 2018
View reviewed changes
Copy link
Contributor

@Yordrar Yordrar left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the raw arrays is confusing and is not very readable, you should encapsulate the points in their own class as stated before, take a look at other implementations such as C++ or python
@crafter312 crafter312 changed the title Jarvis March Implementation in Java WIP Jarvis March Implementation in Java Oct 5, 2018
@crafter312 crafter312 changed the title WIP Jarvis March Implementation in Java Jarvis March Implementation in Java Oct 10, 2018
@PudottaPommin PudottaPommin self-assigned this Oct 10, 2018
PudottaPommin
PudottaPommin requested changes Oct 10, 2018
View reviewed changes
Copy link
Member

@PudottaPommin PudottaPommin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello and thank you for your 1st PR. I'll try to review this as good as I can.

Here are some general points:

  • Indentation for Java files should be 4 spaces. You have 2.
  • if(p.getX()==x&&p.getY()== y) should be if (p.getX() == x && p.getY() == y)
  • braces on all conditions/loops
contents/jarvis_march/code/java/JarvisMarch.java Outdated
public double getY() { return y; }

public boolean equals(Point p) {
if(p.getX()==x&&p.getY()==y)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have recently wrote Code style guide. In general is usage of spaces. So all places like if(p.getX()==x&&p.getY()==y) should be if (p.getX() == x && p.getY() == y) and also I have to ask you to add braces to this and all conditions.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed my usage of spaces. Also, the editor I use for Java was configured to use two spaces for indentation, so I changed that to four.
contents/jarvis_march/code/java/JarvisMarch.java Outdated
{7, -7}, {-2, -9}, {6, -5}, {0, 14}, {2, 8}};
ArrayList<Integer[]> lst2 = new ArrayList<Integer[]>(Arrays.asList(lst));
ArrayList<Point> gift = new ArrayList<Point>();
for(Integer[] i: lst2) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole part would be much more readable if you define just new ArrayList<Point>() and add all points directly into that, instead of this transformation.
When you change it to creating points, you can remove constructor Point(Integer[] p) .
PudottaPommin
PudottaPommin requested changes Oct 10, 2018
View reviewed changes
Copy link
Member

@PudottaPommin PudottaPommin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, it looks much better.

When you change these 2 things, I think it can be merged afterwards.

} else {

to

} else {

and add new line on the end java file.
@PudottaPommin PudottaPommin merged commit 9b2da4e into algorithm-archivists:master Oct 10, 2018
@PudottaPommin
Copy link
Member

Great job and thank you for improving AAA! :)
@crafter312 crafter312 deleted the jarvis_march_in_java branch October 10, 2018 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.)

7 participants

@crafter312 @leios @Trashtalk217 @Wesley-Arrington @PudottaPommin @Yordrar @Gathros