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();
    }
}

Answer 1

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);
}

Answer 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.

Answer 3

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.