Is there a design pattern to handle when code depends on the subtype of two objects

Question

I'll try to be as explicit as possible, in case there is a better solution for my problem than answering my question.

I'm working in C#.

I have a report template that can include any number of 'features' turned on. A feature might be a table of information, a pie/bar chart, a list, etc. I am generating the report as a text file, or a PDF (possibly other options in the future).

So far I have an IFeature interface, and some feature types implementing it: ChartFeature, ListFeature, etc. I read the list of features enabled from the database and pass each one to a method along with the data id and the method returns a populated IFeature of the proper type.

I also have an IReportWriter interface that TextReportWriter and PdfReportWriter implement. That interface has a method: AddFeature(IFeature).

The problem is that AddFeature in each writer ends up looking like:

public void AddFeature(IFeature)
{
    InsertSectionBreakIfNeeded();

    if(IFeature is TableFeature)
    {
        TableFeature tf = (TableFeature)feature;
        streamWriter.WriteLine(tf.Title);
        for(int row=0; row < tf.Data.First.Length; row++)
        {
            for(int column=0; i < tf.Data.Length; i++)
            {
                if(i != 0)
                {
                    streamWriter.Write("|");
                }
                streamWriter.Write(feature.Data[column][row]);
            }
        }
    }
    else if(IFeature is ListFeature)
    {
        ListFeature lf = (ListFeature)feature;
        streamWriter.Write(lf.Title + ": ");
        bool first = true;
        foreach(var v in lf.Data)
        {
            if(!first)
            {
                streamWriter.Write(", ");
            }
            else
            {
                first = false;
            }
            streamWriter.Write(v);
        }
    }
    ...
    else
    {
        throw new NotImplementedException();
    }
    sectionBreakNeeded = true;
}

In the PDF writer the above would be modified to generate PDF table cells, text boxes, and so forth.

This feels ugly. I like it somewhat better as AddFeature(ListFeature){...}, AddFeature(ChartFeature) because at least then it's compile time checked, but in practice it just moves the problem so now outside if the IReportWriter I'm calling if(feature is ...).

Moving the display code into the feature just reverses the problem because it would need to know whether it should be writing plain text or a PDF.

Any suggestions, or am I best just using what I have and ignoring my feelings?

Edit: Filled in some of the conditions to give people a better idea of what is happening. Don't worry too much about the exact code in those examples, I just wrote it off the top of my head.

Answer 1

I'd avoid Visitor for two reasons: 1) it's complicated and 2) it seems your IFeature and IReportWriter hierarchies are both open to extension. Visitor is only good if the visited Element hierarchy is stable. See @Will's comment in https://stackoverflow.com/a/32256469/1168342. Simple is also a good design.

Here's what your code looks like in a UML class diagram:

AddFeature seems to be an inconsistent name. What this method is doing is formatting output, so I'd name it appropriately.

If you follow the Replace conditional with polymorphism refactoring, you can add an IFeature.WriteOutput() method that each concrete Feature will implement. Then your call in IReportWriter looks like

public void AddFeature(IFeature feature)
{
    InsertSectionBreakIfNeeded();
    feature.WriteOutput();
    sectionBreakNeeded = true;
}

In a sense, you've applied only the Strategy pattern to your code, where IFeature plays the role of Strategy and IReportWriter plays the role of Context:

---

Edit for Abstract Factory

It's looking less simple, but your example code didn't really consider all the permutations of [PDF, Text] and [Chart, List].

I suggest the Abstract classes/interfaces of PdfReportFeature and ListReportFeature in case there are some functions like creating a format preamble that would be there. You could possibly apply the Template Method pattern, if needed.

The idea is that each concrete class, e.g., PdfListFeature, will have its own WriteOutput method that does what it needs to do. The concrete ReportWriter just calls feature.WriteOutput() for whatever feature is injected (aggregated) into the report.

There's no double-dispatch since you won't be mixing PDF and Text reports together (Visitor really doesn't make sense to me). When you create a report, it's one or the other type. Your Abstract Factory pattern will help you create and pass the proper class for chart or list into the writer.

I updated the Strategy part above to be consistent with the Abstract Factory approach. I hope this makes sense.

Answer 2

The general case of your problem is called double-dispatch - you need to dispatch to a method based on the runtime type of two parameters, not just one (the "this" pointer).

One standard pattern to deal with this is called the Visitor pattern. It's description traces back to the original Design Patterns book, so there's lots of example and analysis of it out there.

The basic idea is that you have two general things - you have the Elements (which are the things that you're processing) and Visitors, which process over the Elements. You need to do dynamic dispatch over both of them - so the actual method called varies depending on both the concrete type of the element and of the visitor.

In C#, and kinda sorta following your example, you'd define an IFeatureVisitor interface like this:

public interface IFeatureVisitor {
    void Visit(ChartFeature feature);
    void Visit(ListFeature feature);
    // ... etc one per type of feature
}

Then, in your IFeature interface, add an "Accept" method.

public interface IFeature {
    public void Accept(IFeatureVisitor visitor);
}

Your feature implementations would implement the Accept method like so:

public class ChartFeature : IFeature {
    public void Accept(IFeatureVisitor visitor) {
        visitor.Visit(this);
    }
}

And then your report writers would implement the IVisitor interface and do whatever it's supposed to do in each type.

To use this, it's look something like this:

var writer = new HtmlReportWriter();
foreach(IFeature feature in document) {
    feature.Accept(writer);
}
writer.FinishUp();

The way this works is that the first virtual call to Accept resolves back to the concrete type of the feature. The call to the Visit method is NOT virtual - the call to visitor.Visit(this) calls the correct overload since at that point it knows the exact static type of the thing that's being visited. No casts and type safety is preserved.

This pattern is great when new visitor types get added. It's much more painful when the elements (features in your case) change - every time you add a new element, you need to update the IVisitor interface and all the implementations. So consider carefully.

As I mentioned, there's been almost 20 years since the book was published, so you can find lots of analysis and improvements on Visitor pattern out there. Helpfully this gives you enough of a start to continue your analysis.

Answer 3

I would structure this in a slightly different way:

I wod have a IReport object that composes all the features in the report. This object would have methods AddFeature(IFeature) and GenerateReport(IReportWriter)

I would then have IFeature implement WriteFeature(IReport, IReportWriter) and this way delegate how the Feature is actually processed to the Feature itself.

The way you've structured the code makes me think that there is no way to write a Feature in a format agnostic way that can be processed by any given writer, so let the object itself deal with the issue.

Answer 4

I suppose you're trying to draw something (i.e. output it to pdf or text or whatever ...).

My guess would be to go with something like this:

interface IReportWriter {
    void AddFeature(IFeature feature);
    // Some other method to generate the output.
    IOutput Render();
    // Drawing primitives that every report writer implements
    void PrintChar(char c);
    void DrawLine(Point begin, Point end);
    ...
}

// Default implementation for ReportWriters
abstract class AbstractReportWriter {
    private IList<IFeature> features = new List<IFeature>();

    ...

    public void AddFeature(IFeature feature) {
        this.features.Add(feature);
    }

    public IOutput Render() {
        foreach(var feature in this.features) {
            feature.RenderOn(this);
        }
    }

    // Leave the primitives abstract
    public abstract void PrintChar(char c);
    public abstract void DrawLine(Point begin, Point end);
}

And on the feature side:

interface IFeature {
    void RenderOn(IReportWriter);
}

And here is an example implementation of ChartFeature:

public class ChartFeature : IFeature {
    ...
    public void RenderOn(IReportWriter report) {
       // Draw the chart based on the primitives.
       report.DrawLine(..., ...);
       ...
    }
}