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

Enhance the code of TreeTraversal and EuclideanAlgorithm for C# #120

Merged
merged 5 commits into from
Jun 28, 2018
Merged

Enhance the code of TreeTraversal and EuclideanAlgorithm for C# #120

merged 5 commits into from
Jun 28, 2018

Conversation

june128
Copy link
Member

@june128 june128 commented Jun 4, 2018

Enhance and refactor the code of TreeTraversal.
Move all additional methods from TreeTraversalMdAdditional to TreeTraversal and delete the leftover files.
Delete EuclideanAlgorithmMdAdditional as well.

@june128 june128 requested a review from zsparal June 4, 2018 00:46
@june128 june128 added the Implementation Edit This provides an edit to an algorithm implementation. (Code and maybe md files are edited.) label Jun 4, 2018
@Gathros Gathros added the Chapter Edit This changes the archive's chapters. (md files are edited.) label Jun 6, 2018
@june128 june128 removed the Chapter Edit This changes the archive's chapters. (md files are edited.) label Jun 11, 2018
june128 added 4 commits June 28, 2018 00:48
…ersal. Fix incorrect ConsoleWriteLine. Use local functions. Create constructor for Node, which takes an id. Remove unnecessary curly braces. Remove redundant CreateTree method and just put the code into the constructor. Remove unnecessary comment. Remove the now redundant TreeTraversalMdAdditional. Sort the methods so that they have the same order as in the text.
Copy link
Contributor

@Butt4cak3 Butt4cak3 left a comment

Choose a reason for hiding this comment

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

Very few things I can say about C# code, but I tried my best.

tree.DFSRecursivePostorder();

// Console.WriteLine("DFSRecursiveInorder (fail)");
// tree.DFSRecursiveInorderBinary();
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these left over from a debugging session?

Copy link
Member Author

Choose a reason for hiding this comment

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

People can uncomment that, to see, that the algorithm will throw an exception then (because the node has more than 2 children). After that a new Tree is created with only 2 children per node and the method can be used without problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case: What about a comment above these 2 lines that explains what you just explained here? Just something like // Uncomment these to see what happens if ....

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I'll do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Console.WriteLine(node.Id);

foreach (var c in node.Children)
DFSRecursive(c);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a C# expert, but I'm pretty sure that omitting brackets for loops and conditions is dangerous no matter which language you're using.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Gustorn What do you think about that?

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be dangerous, sure, but C#'s bracket style kinda encourages it. Having a single-line statement stretch into 3 eats up a lot of vertical space. I don't really mind either way but as long as it's a short, single line I think omitting the parens is fine

Copy link
Contributor

Choose a reason for hiding this comment

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

I shall allow it, then.

@Butt4cak3 Butt4cak3 removed the request for review from zsparal June 27, 2018 23:31
@Butt4cak3 Butt4cak3 self-assigned this Jun 28, 2018
Copy link
Contributor

@lulucca12 lulucca12 left a comment

Choose a reason for hiding this comment

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

Shouldn't you put the comments in multiple lines

@june128 june128 mentioned this pull request Jun 28, 2018
@leios
Copy link
Member

leios commented Jun 28, 2018

As a note, I think putting the comments in multiple lines looks a little cleaner, but the lines will soft-wrap in the actual book.

@Butt4cak3
Copy link
Contributor

I'm not a fan of soft-wrapping, but I have to agree that it will look even worse when you hard-wrap the lines and then the soft-wrapping in the gitbook kicks in, so I don't mind in this case.

@Butt4cak3
Copy link
Contributor

I think it's good. I hope I didn't miss anything.

@Butt4cak3 Butt4cak3 removed their assignment Jun 28, 2018
@june128 june128 merged commit 71ff692 into algorithm-archivists:master Jun 28, 2018
@june128 june128 deleted the enhanceExamplesPR branch June 28, 2018 21:54
@june128
Copy link
Member Author

june128 commented Jun 28, 2018

I'll open a new PR applying the restructuring suggested by @Gustorn in #137

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implementation Edit This provides an edit to an algorithm implementation. (Code and maybe md files are edited.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants