I need help solving a common architectural problem with multiple concrete classes implementing an interface

Question

I have a common issue I encounter which I've solved with a Factory class in the past but it always felt a little "off".

Let's say I have multiple Exporter classes which implement IExporter, which has a single method:

void Export(string outputPath);

...each Exporter class knows how to export different types of data (for example, data from a database, or from an Array, etc.). But because of this, each Exporter class needs this information injected into it. I approach this using the concrete class's constructor:

DatabaseExporter(DbContext dbContext, string tableName) {...}
ArrayExporter(string[] arrayData) {...}

Depending on the situation, I need to instantiate either a DatabaseExporter or an ArrayExporter. But if I just instantiate one of these classes on the fly, it hurts the testability of my code (i.e. I can't use a mock Exporter). And I can't just inject an IExporter because I need to fill out its parameters (DbContext or Array) somehow.

So the solution I always end up with is to create an injectable ExporterFactory which has methods to create each of the different types of exporters, with parameters. For example:

IExporter CreateDatabaseExporter(DbContext dbContext, string tableName);
IExporter CreateArrayExporter(string[] arrayData);

Is this a sane approach to this problem? It feels weird to have all of the different construction implementations in a single class, but maybe that's the point of the factory. Or maybe I'm misunderstanding the pattern. Could someone please help me clear this up?

Answer 1

Breaking down the problem

What you've done right is identified that all exporters work the same. As a basic example (which I will expand on), this means something along the lines of:

IExporter exporter = GetWhateverExporterYouWant();

exporter.Export("C:\\output.txt");

But you've also noticed that the input value (or rather its type) changes based on the exporter type.

IExporter exporter = GetWhateverExporterYouWant();

exporter.Export("C:\\output.txt", myArrayData); // If it's ArrayExporter
exporter.Export("C:\\output.txt", myContext, myTable); // If it's DatabaseExporter

This is where it breaks down, because a common interface (the Export method) cannot have different signatures in it's different implementations.

You've already noticed that subclasses are still able to define their own personal contructor arguments. That is a very valid point, but you might not want to go down that route.

Where this makes sense is in cases where you use a global dependency injection, where the constructor doesn't really take in "input values", but rather "injected dependencies".
On a technical level, they are the same, but on a functional level they are not. Injected dependencies are usually data-agnostic (the decision on which dependency to use is usually decided based on external factors), whereas input values are contextual to the current request/execution and are expected to change all the time.

As I see it, there are three options here. I'm favoring the third option, but I want to discuss the others first so you know why they are(n't) applicable to you.

1. Explicit instantiation

You can still use the contructor argument approach, but you can't rely on some blind preset framework to do it for you because you need your input values to be easily and clearly changeable.

When explicit instantiation becomes more than just a one-or-two-liner; good practice tends to push you into the realm of factories, which is what you're currently doing.

IExporter CreateDatabaseExporter(DbContext dbContext, string tableName);
IExporter CreateArrayExporter(string[] arrayData);

Is this a sane approach to this problem? It feels weird to have all of the different construction implementations in a single class, but maybe that's the point of the factory.

On a technical level, there is nothing wrong with this. However, if your factory methods are nothing more than a pass-through, then the factory has no purpose. I'm talking about something like:

public IExporter CreateDatabaseExporter(DbContext dbContext, string tableName)
{
    return new DatabaseExporter(dbContext, tableName);
}

public IExporter CreateArrayExporter(string[] arrayData)
{
    return new ArrayExporter(arrayData);
}

The factory methods here are nothing but an empty shell. To the caller, there is nothing to gain by using the factory. With the factory:

IExporter exporter = ExporterFactory.CreateArrayExporter(arrayData);

Without the factory:

IExporter exporter = new ArrayExporter(arrayData);

The purpose of a factory is deciding which subclass to use. The bonus to the caller is that the caller doesn't need to know or decide between the different options. It just tells the factory "give me whatever suits my purpose please". By making passthrough factory methods, you do not actually give the factory a purpose.

Think of it this way: when you go to a restaurant and ask for a spaghetti arrabiata, you trust the chef to know what spices to use in your meal. You can't be bothered figuring it all out, you tell the chef "just make it tasty".
If instead the chef required you to list every spice you want him to use, then there is no purpose to the chef (you might as well cook your own arrabiata then).

To be fair, there is one minor benefit. Even with a passthrough method, you ensure that the constructor is only called in one location (the factory method), rather than all over the place.
However, it's unclear whether that actually yields an advantage. It may yield an advantage if you need to change all the constructor calls to a new class, but in reality that's easily fixed by doing a refactoring of the class name (which changes all references in one go).

Additionally, this is a straw on the camel's back scenario. The additional layer is not necessarily bad or counterproductive even if it has no functional purpose; but if you keep applying that same logic to every part of your application and keep putting more and more (pointless) straws on the camel's back, eventually it will break.

I would reevaluate the purpose of the factory, and whether it's really adding something compared to having the callers simply instantiate their own exporter.

2. Generic input parameters

This might not be applicable because it comes with its own baggage. Doing this requires you to know the type of the input parameters (there is a contrived way to even avoid that but its complexity is IMO much too high and it massively detracts from maintainability and readability).

public interface IExporter<TSource>
{
    void Export(string outputPath, TSource data);
}

Then, when you implement the interface, you use concrete types:

public class PersonExporter : IExporter<Person>
{
    void Export(string outputPath, Person data) { }
}

If there is more than one input value, make a DTO which contains all values:

public class DatabaseValues
{
    public Context MyContext { get; set; }
    public string TableName { get; set; }
}

public class DatabaseExporter : IExporter<DatabaseValues>
{
    void Export(string outputPath, DatabaseValues data) { }
}

You can also use a different generic type as the concrete class.

public class ArrayExporter : IExporter<IEnumerable<T>>
{
    void Export(string outputPath, IEnumerable<T> data) { }
}

However, you might want to subclass this further until you get to a concrete class, or at least add some for of constraint on the generic parameter:

public interface IExportableData
{
    IEnumerable<string> GetExportValues();
}

public class ArrayExporter : IExporter<IEnumerable<T>> where T : IExportableData
{
    void Export(string outputPath, IEnumerable<T> data) { }
}

But the generic approach is only valuable when there is sufficient shared logic in the generic IExporter<T> class. Otherwise it still a passthrough, a hollow shell, and it usually is not worth the effort of creating a pointless hollow shell. It (slightly) bloats the code, doesn't add any functional value, and actually detracts from your polymorphic approach at it requires you to know the type of the input values at all times (you can no longer refer to something as an IExporter, you always have to refer to it as a IExporter<Person>, IExporter<DatabaseValues, etc...

3. No common base type (and/or splitting the responsibilities)

Just because two classes have the same method name/signature does not necessarily mean that they must share a base class. E.g. just because the classes Person, Disease and Album all have a Name property, does not mean that you're required to make them derive from a shared BaseNamedObject or IObjectWithAName class.

The same might be happening here for your exporters. It all hinges on one question: what benefit are you getting from the common base type? I don't quite see a big benefit based on your example code. It feels like you're trying to achieve something like:

public class FooExporter : IExporter
{
    public void Export(string outputPath, Foo foo)
    {
        // A lot of code that parses your Foo as a string

        File.WriteAllText(outputPath, "foo as a string");
    }
}

This is an oversimplification, but I'm struggling to see what logical steps your exporters are going to contain that are not intrinsically related to the input data and its type. Where is the export logic, the logic that is shared between all IExporter objects? Is there that much logic (more than a one-or-two-liner) that it's really necessary to create an inheritance/implementation structure for it?

An export, in my opinion, is innately defined by the data it's intended to parse/export. 95% of the effort goes to parsing the data. Writing the data is usually trivially simple to a point where it just uses a .NET construct (StreamWriter or any of the File.Write...() or File.Append...() variants) Different data leads to a wholly different approach, most likely with little to no reusable pieces. Which means that a common base type offers little purpose.

It almost feels like your IExporter (and implementations) are more intended as a IFileWriter type, where they mostly just define how to write the data to disk (which is something that can be generalized, but is often already a one-liner that hardly needs an additional wrapper), rather than how to parse the data (which, as you've discovered, can't really be generalized).

It may be better to separate the data generation and data storage responsibilities, because then your data storage can be a single class (which takes in string data).

For the data generation, i.e. the taking in of a certain type of input parameter and parsing it down to an exportable format, it may be better to avoid trying to foist a base class on them just because you can say that "they both export things". Exporting is incredibly vague and such a varied topic that it's hard to find any strong commonalities.

Answer 2

One of the most powerful design techniques is to pay attention to what you know when. Following this can help keep your design simple.

You might be setting your dependencies in the wrong place.

ArrayExporter.Export() depends on both string[] arrayData and string outputPath. But which do you know when?

---

If you don't know outputPath until you know the rest:

Exporter exp = new Exporter();

Inject that into something that calls either of these:

exp.ArrayExport(arrayData, outputPath);

exp.DBExport(dbContext, tableName, outputPath);

---

If you know the outputPath before string[] arrayData consider:

Exporter exp = new Exporter(outputPath);

Inject that into something that calls either of these:

exp.ArrayExport(arrayData);

exp.DBExport(dbContext, tableName);

---

If you don't like the same class knowing about the database and arrays then consider a stream adapter:

Exporter exp = new Exporter(outputPath);

Inject that into something that calls either of these:

exp.Export(new ArrayAsStream(arrayData));

exp.Export(new DatabaseAsStream(dbContext, tableName));

Now the Exporter class only knows about output files and streams. Every data source can have their own stream adapter class. All without changing the order in which you learn things.

---

If you know all this stuff well before you know when you want to export:

Exporter exp = new ArrayExporter(arrayData, outputPath);

Exporter exp = new DatabaseExporter(dbContext, tableName, outputPath);

Inject either of those into something that calls:

exp.Export();

You can reuse the stream idea here if you like.

---

According to your comment you know arrayData first

Exporter exp = new ArrayExporter(arrayData);

Exporter exp = new DatabaseExporter(dbContext, tableName);

Inject either of those into something that calls:

exp.Export(outputPath);

---

Any of these ways will let you use as much pure dependency injection as you like and mock to your hearts content.

Answer 3

Is this a sane approach to this problem?

It depends. For many situations, this is sufficient and perfectly ok. If you don't create a new type of exporter every two weeks, or don't want to provide an extensible black-box library for dealing with exporter classes, it is probably better to keep things simple and live with a little bit of "duct tape".

It feels weird to have all of the different construction implementations in a single class

If that bothers you, or if you want the factories follow the Open-Closed principle, you can make use the abstract factory pattern. Make a DatabaseExporterFactory and an ArrayExporterFactory, implementing an IExporterFactory. Now the construction implementations "live" in different classes, and you can add new construction mechanisms just by adding new factory types.

The pattern allows you to separate the decision of which type of exporter will be created from point in the code where the actual creation will happen. This is useful if you want to make it possible to add new exporter types and new exporter factories outside of some reusable library code which will make use of these interfaces.

Let me add separating the different construction processes may be indeed mandatory if a "single, combined" factory would introduce some undesirable coupling of dependencies. If the creation code for a DatabaseExporter has a dependency on some huge database component, putting that code in the same class as the ArrayExporter creation code, forces any code which tries to reuse the latter to depend on the database component. If that's the case, using "abstract factory" maybe the appropriate solution.

Answer 4

If we use your method, this is actually very easy to test.

You have an interface that exports database and arrays. Now add a test export class that has a simple mocking implementation that implements the interface so you could use it for tests.

Also, could you explain a little more why this makes it hard to test?

Also, maybe the export interface is not a good abstraction?

Seems like you are violating the single responsibility principle and your 2 classes are doing a lot.

You should have a class that reads from the db. Then a class that takes that db's output and writes it to a file. Might make it easier to test. I am just throwing out ideas and I don't know enough about what you are doing.

Answer 5

My first impulse is that you're really trying to extend IEnumerable, so you should just write an IEnumerable extension similar to LINQ. Not only is it simpler, it's also easy to mock and test!

To expand on my suggestion, whether you have a DbSet, an Array, a Dictionary, a List, etc, it should not matter since they are IEnumerable. If your data source is IEnumerable, it makes sense that you should be able to Export it. So if you extend IEnumerable with IExportable (or something similar), then you can use LINQ syntax to export any IEnumerable, even ones you haven't thought about yet. On the other hand, if you want to do something sightly different for each kind of data source, then extending IEnumerable may not work for you.

Answer 6

My initial take on this is that your classes already violate the SRP and this is making it difficult for you to construct orthogonal interfaces.

interface IDataReader
{
    IEnumerable<TSchema> Read<TSchema>(DbContext dbContext, string tableName)
}

interface IDataWriter 
{
    bool Write<TSchema>(IEnumerable<TSchema> data, string outputPath);
}

If you already have data, then you can write it. If you need data, you can pull it from the DB and then write it.