StackOverflowException caused by Recursion

Question

I am currently writing a program to help write Lore. Every Book Object can be a parent and have children. Which means that every child can have children etc. into infinity. I was working on a ToString() method that could account for this using recursion, but I keep getting a StackOverflowException.

I know what it means, but I am in doubt as to how I fix it. I am fairly new to C# but have quite a lot of Java experience, so if you know a trick or something that I've missed then please let me know!

So my question is: How do I avoid a StackOverflow Exception? The problem is in GetAllChildren()

EDIT:

After running a test, I should get something like this:

Name: a
Children:
b
c
    d
e

With the code from @lc. I get the following output:

Name: a
Children: No Children   b
c
e
    b
c
e
    b
c
e

Here is the class:

class Book
{
    private String name;
    private Book[] children;
    private StringBuilder text;
    private Boolean isParent;

    public Book(String name, Book[] children, StringBuilder text, Boolean isParent)
    {
        this.name = name;
        this.children = children;
        this.text = text;
        this.isParent = isParent;
    }

    /**
     * Most likely all possible Constructors
     * */
    public Book(String name, Book[] children) : this(name, children, new StringBuilder("No Text"), true) { }
    public Book(String name, String text) : this(name, new Book[0], new StringBuilder(text), false) { }
    public Book(String name, StringBuilder text) : this(name, new Book[0], text, false) { }
    public Book(String name) : this(name, new Book[0], new StringBuilder("No Text"), false) { }
    public Book(Book[] children, String text) : this("Unnamed Book", children, new StringBuilder(text), true) { }
    public Book(Book[] children, StringBuilder text) : this("Unnamed Book", children, text, true) { }
    public Book(Book[] children) : this("Unnamed Book", children, new StringBuilder("No Text"), true) { }
    public Book(StringBuilder text) : this("Unnamed Book", new Book[0], text, false) { }
    public Book() : this("Unnamed Book", new Book[0], new StringBuilder("No Text"), false) { }

    public String Name
    {
        get { return name; }
        set { name = value; }
    }

    public Book[] Children
    {
        get { return children; }
        set { children = value; }
    }

    /**
     * Will Return the StringBuilder Object of this Text
     * */

    public StringBuilder Text
    {
        get { return text; }
        set { text = value; }
    }

    public Boolean IsParent
    {
        get { return isParent; }
        set { isParent = value; }
    }

    private void GetAllChildren(Book book, StringBuilder sb)
    {
        if (book.isParent)
        {
            GetAllChildren(book, sb);
        }
        else
        {
            sb.Append("\t");
            foreach (Book b in children)
            {
                sb.Append(b.Name + "\n");
            }
        }
    }

    public override String ToString()
    {
        StringBuilder sChildren = new StringBuilder("No Children");
        if (children.Length != 0)
        {
            GetAllChildren(this, sChildren);
        }

        return "Name: " + name + "\n" +
            "Children: " + sChildren.ToString();
    }
}

@SiGanteng I was considering making the joke, but I chose not too ;P — OmniOwl
– OmniOwl, Commented Mar 14, 2013 at 9:16
What was the thinking behind calling GetAllChildren(book, sb); inside the isParent branch? Note that neither book nor sb have been altered. — Damien_The_Unbeliever
– Damien_The_Unbeliever, Commented Mar 14, 2013 at 9:19

lc. · Accepted Answer · 2013-03-14 09:40:56Z

8

I think you meant:

if (book.isParent)
{
    foreach (var child in book.Children)
        GetAllChildren(child, sb);
}

Otherwise you're just calling the GetAllChildren method with the same parameters (book, sb) over and over again.

Side note - you still have some problems because the stop condition in the GetAllChildren is iterating through children, when it should not (if it is not a parent, it should not have children). It should instead return its own name. Furthermore, each child should also append its name in the foreach loop above (or actually, each book should append its own name).

Side note 2 - the method as written (with these changes) should really be static, as it is not related to any given instance (which brings me to the suggestion below).

Suggestion - I would recommend something like the following instead (untested and will need some work on the formatting):

//name changed to reflect what it really does
//also changed to be an instance method (we no longer pass in a Book)
//added listThisBooksName parameter to allow supressing the topmost book's output
private void AppendAllChildren(StringBuilder sb, int level = 0, 
    bool listThisBooksName = false)
{
    if (listThisBooksName)
    {
        //append ourself here

        //first indent however far we need to
        sb.Append(new String('\t', level));

        //now add our name
        sb.Append(this.Name);

        //and a newline (you can strip the last one later if you want)
        sb.Append('\n');
    }

    //forget the "isParent" property, just check if it has any children
    //we don't need Children.Any() because the foreach will just iterate 0 times
    //you might also consider using a List<Book> instead of an array for Children
    if (this.Children != null)
        foreach (var child in this.Children)
            child.AppendAllChildren(sb, level+1, true);
}

edited Mar 14, 2013 at 9:40

answered Mar 14, 2013 at 9:16

lc.

117k21 gold badges163 silver badges190 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

OmniOwl Over a year ago

I tried your code from the first note, not your static one yet and stopped getting the exception, but please look at my edit. The method is still off.

OmniOwl Over a year ago

Almost perfect. The parent list itself as child though.

lc. Over a year ago

@Vipar Didn't know you wanted to list the topmost parent separate from the child. Check my edit. I assume you understand what this code is doing?

OmniOwl Over a year ago

There we go! Perfect! Thanks a lot man :) For any future readers the Static Method works!

Brian Agnew · Accepted Answer · 2013-03-14 09:17:18Z

2

Isn't this the issue:

    if (book.isParent)
    {
        GetAllChildren(book, sb);
    }

and you call the same method again ? I think the above should iterate through the children and call GetAllChildren for each child Book. Only output the name if your Book doesn't have any children.

answered Mar 14, 2013 at 9:17

Brian Agnew

273k38 gold badges342 silver badges443 bronze badges

Comments

nvoigt · Accepted Answer · 2013-03-14 09:17:33Z

2

You recursion is recursing on the same book while that book's IsParent is true. You probably want to recurse through all children if the book is a parent.

answered Mar 14, 2013 at 9:17

nvoigt

77.8k28 gold badges103 silver badges156 bronze badges

Collectives™ on Stack Overflow

StackOverflowException caused by Recursion

3 Answers 3

4 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

4 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related