Composite and Visitor patterns for tree-based survey functionality in C#

Question

I have written some survey functionality for a project. Basically, a generic survey form that can be composed of sections and questions.

I have a Survey class, Questions and Sections. The Survey is basically a tree, where each node can be a Question or a Section. Nodes have children -- so essentially Question can have a collection of subsections and subquestions, and a Section can have a collection of subsections and subquestions.

The nodes in my Survey have derive from the abstract class SurveyPart.

namespace Surveys
{
    public abstract class SurveyPart
    {
        public abstract List<SurveyPart> Children { get; set; }
    }

    public class Survey
    {
        public List<SurveyPart> Children { get; set; }

        public Survey()
        {
            Children = new List<SurveyPart>();
        }
    }

    public class Question : SurveyPart
    {
        public override List<SurveyPart> Children { get; set; }
        public string QuestionText { get; set; }

        public Question()
        {
            Children = new List<SurveyPart>();
        }
    }

    public class Section : SurveyPart
    {
        public override List<SurveyPart> Children { get; set; }
        public string Header { get; set; }

        public Section()
        {
            Children = new List<SurveyPart>();
        }
    }
}

As far as I understand this is the Composite pattern? Not sure I've got it entirely right.

So with that I can build a survey (at present with the sections and questions coming from a DB.) Next thing is to render it. For that I'm attempting to use the Visitor pattern implemented with extension methods.

namespace ExtensionMethods
{
    using Surveys;

    public static class SurveyTextRenderer
    {
        public static int Depth;

        public static void Write(this Survey survey)
        {
            Depth = 0;

            Console.WriteLine("Survey");
            Console.WriteLine(new string('-', "Survey".Length));

            foreach (SurveyPart child in survey.Children)
            {
                Depth++;
                child.Write();
                Depth--;
            }
        }


        public static void Write(this SurveyPart part)
        {
            if (part is Section)
                (part as Section).Write();
            if (part is Question)
                (part as Question).Write();
        }


        public static void Write(this Section section)
        {
            Console.Write(new String('\t', Depth));
            Console.WriteLine("S:" + section.Header);

            foreach (SurveyPart child in section.Children)
            {
                Depth++;
                child.Write();
                Depth--;
            }
        }


        public static void Write(this Question question)
        {
            Console.Write(new String('\t', Depth));
            Console.WriteLine("Q: " + question.QuestionText);

            foreach (SurveyPart child in question.Children)
            {
                Depth++;
                child.Write();
                Depth--;
            }
        }
    }
}

It all works OK -- if I set up the following mock survey:

Survey survey = new Survey
            {
                Children = new List<SurveyPart>
                {
                    new Section 
                    { 
                        Header = "Section 1", 
                        Children = new List<SurveyPart>
                        {
                            new Question { QuestionText = "Foo?" },
                            new Question { QuestionText = "Bar?" },
                            new Question { QuestionText = "Barry?" }
                        }
                    },
                    new Section 
                    { 
                        Header = "Section 2", 
                        Children = new List<SurveyPart>
                        {
                            new Question 
                            { 
                                QuestionText = "Did you like it?",
                                Children = new List<SurveyPart>
                                {
                                    new Section 
                                    { 
                                        Header = "If you answered yes, please answer the following",
                                        Children = new List<SurveyPart>
                                        {
                                            new Question { QuestionText = "How come?" },
                                            new Question { QuestionText = "How much did you like it?" }
                                        }
                                    }
                                }
                            },
                            new Question { QuestionText = "Please leave a comment" },
                        }
                    }
                }
            };

and call

survey.Write();

I get:

Survey
------
S:Section 1
    Q: Foo?
    Q: Bar?
    Q: Barry?
S:Section 2
    Q: Did you like it?
        S:If you answered yes, please answer the following
            Q: How come?
            Q: How much did you like it?
    Q: Please leave a comment

So basically to sum up, I'm trying to use the Composite pattern to allow for a tree of sections and questions. Then to navigate and render this tree I'm trying to use the Visitor pattern.

A few questions occurring to me:

• I'm using the static member Depth to keep track of how deep the visitor has gone. Could it be problematic having state on my extension methods static class?
• Would the SurveyPart abstract class make more sense as an interface?
• Am I using Visitor and Composite in the right way, or am I borking them up?

(Note I've left a lot a parts out... e.g. different types of questions, scores/responses to questions, etc... just focused on the tree/Composite/Visitor parts of the code for now.)

Answer 1

That design will work well for multiple renderers - when you stated in the comment that you would it made much more sense. You should remove the static Depth variable though - it makes your code not thread-safe. Also, you can reduce the number of overloads. Here's a way to refactor your renderer:

public class SurveyTextRenderer
{
   public Write(Survey survey)
   {
      Console.WriteLine(survey.Name);
      Console.WriteLine(new string('-', survey.Name.Length);

      for (SurveyPart part in survey.Children)
      {
         processNode(part, 0);
      }
   }

   protected void ProcessNode(SurveyPart part, int depth)
   {
      if (part is Section)
         WriteSection(part as Section, depth);
      else if (part is Question)
         WriteQuestion(part as Question, depth);
      else
         // Error handling or default case

      for (SurveyPart part in survey.Children)
      {
         ProcessNode(part, depth + 1);
      }
   }
}

Note the error handling - what happens if you add a new SurveyPart and don't update the renderer? Also, I changed the name to use a Survey name, which you should see how to implement easily.

I did not implement WriteSection and WriteQuestion; they'll be pretty close to what you have already except that the recursion is removed. I don't think you really need statics in this case, but you can make them static if you want. However, you could make, say,

public abstract class Renderer
{
   public abstract void Write(Survey survey);
}

and extend that. It may or may not be useful to you. It depends on your calling code whether it would be worth adding that abstraction. If you have a method that is called like PrintSurvey(new TextRenderer(), datasource) where datasource is one of multiple places the survey could be stored (XML, database, file, etc.) it might be useful. You don't want to repeat yourself. In fact, you could have Survey extend SurveyPart (maybe rename it SurveyElement?) and remove the sort-of-redundant Write()->ProcessNode() calls.

Hopefully at least this gives you a few ideas!

Answer 2

I think trying to implement a visitor pattern with static members is the wrong way to go, you should use an actual instance of the visitor and pass it around the data structure.

Also, I generally don't like the idea of one class making decisions based on the type of another, just feels wrong to me.

In all the descriptions of the visitor pattern I have seen, it was the responsibility of the objects in the hierarchy to pass the visitor object to their children, though I have always found it more convenient for the visitor to do its own propagation (different visitors may want different traversals - prefix, postfix, infix, etc).

public interface IVisitor
{
    void Visit(Survey survey);
    void Visit(Section section);
    void Visit(Question question);
}

public abstract class SurveyPart
{
    // ...

    public abstract void Apply(IVisitor visitor);
}

public class Survey
{
    // ...

    public abstract void Apply(IVisitor visitor);
}

public class Question : SurveyPart
{
    // ...

    public override void Apply(IVisitor visitor)
    {
        visitor.Visit(this);
    }
}

public class Section : SurveyPart
{
    // ...

    public override void Apply(IVisitor visitor)
    {
        visitor.Visit(this);
    }
}

The render visitor can then be implemented as follows.

public class RenderVisitor : IVisitor
{
    public RenderVisitor(TextWriter writer)
    {
        this.writer = writer;
    }

    void Visit(Survey survey)
    {
        writer.Write(...);
        VisitChildren(servey.Children);
    }

    void Visit(Section section);
    {
        writer.Write(...);
        VisitChildren(servey.Children);
    }

    void Visit(Question question);
    {
        writer.Write(...);
        VisitChildren(servey.Children);
    }

    void VisitChildren(List<SurveyPart> children)
    {
        depth++;

        foreach(SurveyPart child in children)
        {
            child.Apply(this);
        }

        depth--;
    }

    int depth;
    readonly TextWriter writer;
}

And used as:

servey.Apply(new RenderVisitor(Console.Out));

Answer 3

Prelude

I'm using the static member Depth to keep track of how deep the visitor has gone. Could it be problematic having state on my extension methods static class?

It would be uncommon. In fact, I wouldn't use extension methods to begin with. Tree walking should be performed by a dedicated tree walker.

Would the SurveyPart abstract class make more sense as an interface?

I would definately keep the abstract class, but perhaps it could implement an interface. I don't think it would hurt the design to have an interface as well.

Am I using Visitor and Composite in the right way, or am I borking them up?

• You have implemented the Composite pattern correctly.
• This is a good example of the Visitor pattern. I don't think you have implemented this pattern correctly. What you did do is implement in the spirit of the pattern: "Visitor lets you define a new operation without changing the classes of the elements on which it operates."

Tree Walking

Your survey project is a perfect example of how to walk a tree of objects. There are two common patterns for walking a tree. They are both used heavely in compiler generators. A good source for understanding the difference is given here .

1. Visitor Pattern
2. Listener Pattern

The main difference is that a visitor acts upon the caller whenever it gets accepted by that caller, while a listener gets notified about the steps in a tree walk over the caller. The listener requires a tree walker to get called. All will become clear in my proposed solution further in the review.

The text rendering of your survey fits better as a listener pattern.

pseudo-code:

- tree walker: walks survey
- listener: writes 'hello survey'
- tree walker: walks section
- listener: pushes indent
- listener: writes 'hello section'
- ..
- tree walker: exits section
- listener: pops indent
- tree walker: exits survey
- listener: pops indent

---

Model Design

I would keep the abstract class, accomodate it with an interface, and see no reason why Children have to be abstract. Since the class is abstract, it could do with a protected constructor.

 public abstract class SurveyPart
 {
     public abstract List<SurveyPart> Children { get; set; }
 }

public interface ISurveyPart
{
    IList<ISurveyPart> Children { get; set; }
}

public abstract class SurveyPart : ISurveyPart
{
    public IList<ISurveyPart> Children { get; set; }

    protected SurveyPart() {
        Children = new List<ISurveyPart>();
    }
}

The derived classes can then be simplified. You no longer have to create Children for each of them. I have also made a design decision to include Survey as derived type of SurveyPart. Many tree API's allow for the container to be part of the nodes.

public class Survey : SurveyPart
{
    public Survey() { }
}

public class Question : SurveyPart
{
    public string QuestionText { get; set; }
    public Question() {}
}

public class Section : SurveyPart
{
    public string Header { get; set; }
    public Section() { }
}

---

Pattern Design

As suggested in the foreword, I would opt for a listener pattern. However, you could combine it neatly with the visitor pattern. So let's do that :-)

First, we'll have to create some interfaces.

public interface ISurveyListener
{
    void Enter();
    void Enter(ISurveyPart surveyPart);
    void Exit(ISurveyPart surveyPart);
    void Exit();
}

public interface ISurveyVisitor
{
    void Visit(ISurveyPart surveyPart);
}

It is very important to see how the listener and visitor each have their own way of dealing with ISurveyPart. ISurveyListener gets called by SurveyTreeWalker and ISurveyVisitor gets called by SurveyPart.

public interface ISurveyPart
{
    IList<ISurveyPart> Children { get; set; }
    void Accept(ISurveyVisitor visitor);
}

public abstract class SurveyPart : ISurveyPart
{
    public IList<ISurveyPart> Children { get; set; }

    protected SurveyPart() {
        Children = new List<ISurveyPart>();
    }

    public virtual void Accept(ISurveyVisitor visitor) {
        if (visitor == null) return;
        visitor.Visit(this);
        if (Children == null) return;
        foreach (var child in Children) {
            child.Accept(visitor);
        }
    }
}

public class SurveyTreeWalker
{
    public static void Walk(ISurveyPart surveyPart, ISurveyListener listener) {
        listener.Enter();
        WalkNode(surveyPart, listener);
        listener.Exit();
    }

    public static void WalkNode(ISurveyPart surveyPart, ISurveyListener listener) {
        if (surveyPart == null) return;
        listener.Enter(surveyPart);
        if (surveyPart.Children != null) {
            foreach (var child in surveyPart.Children) {
                WalkNode(child, listener);
            }
        }
        listener.Exit(surveyPart);
    }
}

---

Text Rendering

Now that we have our model and patterns ready, we can implement SurveyTextRenderer. It is both a ISurveyVisitor and ISurveyListener. For rendering the entire tree, we will use it as listener.

We first make a base class that adheres to our patterns. 3 abstract Render methods are available for derived classes to render the survey parts to text. There is functionality for writing with indentations: PushIndent, PopIndent, Write, WriteLine. The visitor pattern is implemented as to render the accepting survey part. The listener pattern is implemented as to visit the survey parts and deal with indentations.

public abstract class SurveyTextRendererBase : ISurveyVisitor, ISurveyListener
{
    public const string DefaultIndentToken = "\t";
    public TextWriter Writer { get; set; }
    public string IndentToken { get; set; }
    protected string IndentText { get { return string.Join(string.Empty, indent); } }

    private Stack<string> indent;
    private ISurveyPart rootPart;

    protected SurveyTextRendererBase(TextWriter writer) {
        Writer = writer;
        IndentToken = DefaultIndentToken;
    }

    protected abstract void Render(Survey survey);
    protected abstract void Render(Question question);
    protected abstract void Render(Section section);

    protected virtual void Render(ISurveyPart surveyPart) {
        if (surveyPart == null) return;
        if (surveyPart is Survey) Render(surveyPart as Survey);
        if (surveyPart is Question) Render(surveyPart as Question);
        if (surveyPart is Section) Render(surveyPart as Section);
    }

    public void Visit(ISurveyPart surveyPart) {
        Render(surveyPart);
    }

    public void Enter() {
        indent = new Stack<string>();
    }

    public void Enter(ISurveyPart surveyPart) {
        if (rootPart == null) {
            rootPart = surveyPart;
        } else {
            PushIndent();
        }
        Visit(surveyPart);
    }

    public void Exit(ISurveyPart surveyPart) {
        if (surveyPart != rootPart) {
            PopIndent();
        }
    }

    public void Exit() {
        indent = null;
        rootPart = null;
    }

    protected void Write(string text) {
        Writer.Write(string.Format("{0}{1}", IndentText, text));
    }

    protected void WriteLine(string text) {
        Writer.WriteLine(string.Format("{0}{1}", IndentText, text));
    }

    protected void PushIndent() {
        indent.Push(IndentToken);
    }

    protected bool PopIndent() {
        if (indent.Any()) {
            indent.Pop();
            return true;
        }
        return false;
    }
}

The specific class SurveyTextRenderer performs the rendering as specifed in the OP's situation.

public class SurveyTextRenderer : SurveyTextRendererBase
{
    public SurveyTextRenderer(TextWriter writer) 
        : base (writer) {
    }

    protected override void Render(Survey survey) {
        WriteLine("Survey");
        WriteLine(new string('-', "Survey".Length));
    }

    protected override void Render(Question question) {
        WriteLine("Q: " + question.QuestionText);
    }

    protected override void Render(Section section) {
        WriteLine("S:" + section.Header);
    }
}

---

Usage Scenario

Let's put the example data in a method CreateTestSurvey. Now we can perform the following test and get the output the OP requested.

public static void Main()
{
    var survey = CreateTestSurvey();
    var canvas = new StringBuilder();

    using (var writer = new StringWriter(canvas)) 
    {
        SurveyTreeWalker.Walk(survey, new SurveyTextRenderer(writer));
    }

    Console.Write(canvas.ToString());
    Console.ReadKey();
}

Answer 4

I'm using the static member Depth to keep track of how deep the visitor has gone. Could it be problematic having state on my extension methods static class?

Absolutely. The reason for this is simply that static methods should always be thread-safe, they are expected to be self-contained units-of-work, so to speak - currently your variable is not exclusively accessible to the current calling thread. This means that your method/s could have side-effects on execution, dependant on things once-removed from such. Stress testing might make issues become apparent, but without that it is simply a matter of usage and time before things get eerie.

Would the SurveyPart abstract class make more sense as an interface?

That depends, do you intend to add any base functionality to this type? Expose any helpful reusable elements that could be contained there and utilised in the same fashion by all inheritors? If so, then yes, otherwise, if the only reason for this type is to constrain other types to a certain model (or adhere to patterns), then no.

Am I using Visitor and Composite in the right way, or am I borking them up?

To be honest, I'm at work and don't really have time to analyse your patterns right now. Though, the composite pattern might dictate the use of interfaces to de-mark your known types, with levels of abstraction starting form this level.