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

c# tree traversal: fixed index out of range issue #894

Merged

Conversation

henrikac
Copy link
Contributor

This PR fixes an issue in the DFSRecursiveInorderBinary method where

if (tree._children.Count > 0)
{
    DFSRecursiveInorderBinary(tree._children[0]);
    Console.WriteLine(tree.Id);
    DFSRecursiveInorderBinary(tree._children[1]);
}

would throw an IndexOutOfRange exception if tree._children.Count == 1.

This PR also removes a couple of redundant this. in the constructors.

Note: The first line in Tree.cs says "// submitted by Julian Schacher (jspp)" and I am not sure if I should make any changes to this line or add something like "modified by ..." because this is not the exact code submitted by Julian?

@Amaras Amaras added hacktoberfest-accepted Implementation Edit This provides an edit to an algorithm implementation. (Code and maybe md files are edited.) labels Oct 22, 2021
@leios
Copy link
Member

leios commented Oct 22, 2021

Yeah, we kinda changed things since tree traversal was added and are really inconsistent with the "submitted by" notes. I think either removing the note or adding a "modified by..." would both be ok solutions to the note

@henrikac
Copy link
Contributor Author

I removed the submitted line to make it more consistent with the examples in other languages.

contents/tree_traversal/code/csharp/Tree.cs Outdated Show resolved Hide resolved
contents/tree_traversal/code/csharp/Tree.cs Outdated Show resolved Hide resolved
contents/tree_traversal/code/csharp/Tree.cs Outdated Show resolved Hide resolved
@Amaras
Copy link
Member

Amaras commented Oct 25, 2021

@lazyprop That basically is resolving the merge.

Copy link
Member

@leios leios left a comment

Choose a reason for hiding this comment

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

I think this is a straightforward change and am happy to merge it. The line numbers seem fine.

I'll wait a second to see if anyone who knows more C# has anything to say before merging. If anyone else wants to click the button, feel free!

@Amaras
Copy link
Member

Amaras commented Oct 26, 2021

Looks good to me as well (I don't know C Sharp that well, but it's readable enough)

@Amaras Amaras merged commit 4a81890 into algorithm-archivists:master Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted 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.

4 participants