Skip to content
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.

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

Implementation of Tree Traversal in java #149

Merged
merged 10 commits into from
Jun 29, 2018
Merged

Implementation of Tree Traversal in java #149

merged 10 commits into from
Jun 29, 2018

Conversation

xam4lor
Copy link
Contributor

@xam4lor xam4lor commented Jun 28, 2018

No description provided.

@Gathros Gathros added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Jun 28, 2018
@zsparal
Copy link
Contributor

zsparal commented Jun 28, 2018

The same argument as in the C# case. I don't think we need separate implementations just to include in the chapters. The final code, especially with the explicit this makes it incredibly clear what the reader should expect. In Java you would normally write the parameterless version, so just stick to that one.

Additionally, I'd prefer to keep merging stuff into the archive that actually compiles.

@Butt4cak3
Copy link
Contributor

Butt4cak3 commented Jun 28, 2018

Yup. What @Gustorn already said. Show the entire code at the bottom of the chapter and import parts of it for the smaller snippets. No need to have two versions of the same code.

@xam4lor
Copy link
Contributor Author

xam4lor commented Jun 28, 2018

Allright !

@xam4lor
Copy link
Contributor Author

xam4lor commented Jun 28, 2018

The problem is that if I do that, I have to options :

  • Create useless methods in the Tree class who are only use on the website to explain part by part of the function (like DFS_recursive_postorder())
  • Don't add this methods and let a 'hole' which says 'This has not been implemented in your chosen language, so here is the Julia code'

That's why I made two different codes.

@Butt4cak3
Copy link
Contributor

Butt4cak3 commented Jun 28, 2018

I'd just go for unused methods. They don't hurt and people can take the code and play around with them if they want.

@june128
Copy link
Member

june128 commented Jun 28, 2018

Currently the C# code is split into MdAdditional (for in-text code) and normal example code as well. Both compile, but one is the actual example.
With the most recent PR (since things and how to do them changed a bit) they now get merged into one file.
So I support and strongly recommend one implementation instead of two and of course ones that actually compile. After all people should be able to run the code and be able use it for learning.

@xam4lor xam4lor mentioned this pull request Jun 28, 2018



public void DFSRecursive(Node node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should really just remove these versions. They are not any more clear and they take up a bunch of space for no reason. People who'll want to use the Java implementation will know what this.root means in the other implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we need this method to have a parameter of type Node.
I have two solutions : one is to create a function with a same name but no parameters like DFSRecursive() who just call DFSRecursive(this.root).
Or I call DFSRecursive(null) and check if the node is null, the node equals this.root.
In java we cannot create functions within other functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could create a private function under the public one which does the recursive part and then you can include both in the code example

Copy link
Contributor

Choose a reason for hiding this comment

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

I was about to suggest method overloading as well. Is there a reason not to?

Copy link
Contributor Author

@xam4lor xam4lor Jun 29, 2018

Choose a reason for hiding this comment

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

It was one of my solutions, I think It's the better thing to do. (overloading)

@zsparal
Copy link
Contributor

zsparal commented Jun 29, 2018

One more thing, the methods do not follow Java's naming convention: they should be camelCase, e.g.: dfsStack.

@zsparal
Copy link
Contributor

zsparal commented Jun 29, 2018

I'm a bit unsure about the excessive whitespace use to separate the different functions. I see why you did it but maybe change it to 2 newlines.

Copy link
Contributor

@zsparal zsparal left a comment

Choose a reason for hiding this comment

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

My last nitpicks, sorry for doing them piecemeal, I did the earlier ones during the day when I had a little free time.


private void createAllChildren(Node node, int rowCount, int childrenCount) {
if(rowCount <= 1)
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

This indentation is not quite right

Queue<Node> queue = new PriorityQueue<Node>();
queue.add(this.root);

Node temp;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can move this inside the loop

Copy link
Contributor

Choose a reason for hiding this comment

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

Any opinion on this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by 'inside a loop' ?

Copy link
Contributor

Choose a reason for hiding this comment

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

You only use tmp inside the following while loop, so you could write this instead:

while(stack.size() != 0) {
    System.out.println(stack.peek().id);
    Node tmp = stack.pop();

    for (Node c : tmp.children) {
        stack.push(c);
    }
}

dfsRecursiveInOrderBinary(node.children.get(0));
System.out.println(node.id);
dfsRecursiveInOrderBinary(node.children.get(1));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The standard Java coding style would place the else on the same line as the closing brace:

if (...) {

} else {

}


// This assumes only 2 children
private void dfsRecursiveInOrderBinary(Node node) {
if(node.children.size() > 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the de-facto standard is to have spaces around brackets in Java, so

if (condition) {

}
// Instead of
if(condition) {

}

Similarly for while, for, etc. loops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right ! That's a bad practice that I have :/

@zsparal
Copy link
Contributor

zsparal commented Jun 29, 2018

With the newest whitespace/formatting changes the line ranges in the chapter are out of date

@xam4lor
Copy link
Contributor Author

xam4lor commented Jun 29, 2018

And I think that's done !

Copy link
Contributor

@zsparal zsparal left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, sorry again for giving the feedback piece-by-piece, I know it's more annoying to fix it this way :)

@zsparal zsparal merged commit 01c6fb8 into algorithm-archivists:master Jun 29, 2018
@xam4lor
Copy link
Contributor Author

xam4lor commented Jun 29, 2018

You do what you can ! No problem ! :)

@xam4lor xam4lor deleted the treetraversal-java branch June 29, 2018 20:12
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.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants