6

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?

  • Which do you know first? arrayData or outputPath? – candied_orange Jun 5 at 18:15
  • That's a good question, as I haven't implemented the UI yet which will populate the output path. So I suppose I'll know the arrayData first. – vargonian Jun 5 at 20:40
  • Does your DatabaseExporter convert data into an array-like object before exporting it? – Jared Goguen Jun 7 at 4:18
2

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.

  • The data classes could also be responsible for their own parsing. – Adam B Jun 7 at 13:50
  • @Flater Thank you very much for this explanation; it really clarified the "smell" that I kept detecting with my implementation. The entire point of the factory was because I have a limitation: I can't use my IoC container to bind an IExporter to more than one concrete class, but I do need multiple concrete implementations to exist. I want to be able to mock the exporter/file-writer for unit testing. Separating these "writers" (i.e. classes which take an input type and write it to a shared file format) into their own classes may be best choice, though what about Doc Brown's Stream option? – vargonian Jun 9 at 6:43
  • @vargonian: You're right that IoC isn't right for you here. IoC is for compiletime (or runtime startup) fixed dependencies and that's not what you want here. Based on your comment though, I do want to stress that just because you need an interface for mocking does not automatically mean you need a factory for instantiating the concrete classes. If the choice of concrete class is known (per consumer), there's nothing wrong with having the consumer (or whoever instantiates the consumer) instantiate the concrete exporter. – Flater Jun 9 at 12:00
  • 1
    @vargonian: In essence, without a factory, a consumer needs to make their own decision between FooExporter and BarExporter. But with a "passthrough factory", consumers are still forced to explicitly choose between CreateFooExporter() and CreateBarExporter(), which still means the consumer is making the same choice, so nothing really changes. A factory is only useful if you can do things like CreatePersonExporter() which can choose to return varying concrete IExporter implementations. If the method choice reveals the concrete type choice, there's little purpose to it. – Flater Jun 9 at 12:04
6

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.

  • Thanks for the solution. The problem with this approach is that it requires a single class to know how to export from an Array, from a Database, or any other data source in the future. That violates the single responsibility principle, unless I'm fundamentally misunderstanding something. – vargonian Jun 5 at 19:14
  • @vargonian I've added stream adapters that isolate knowledge of the array from the database and from the outputPath. These should be just as injectable and mockable as the rest. – candied_orange Jun 6 at 13:11
  • Thank you, I'll have to read up on stream adapters to see how those work. Looks promising, and something I may use in other ways in the future! – vargonian Jun 6 at 17:39
  • @vargonian well "stream adapter" isn't a google-able thing on it's own. It's simply a class that takes whatever it needs, array, database creds & query, rest endpoint, etc and turns whatever in finds into a stream that could be sent to a file, standard out, a printer, whatever. Start here – candied_orange Jun 6 at 17:56
  • My interpretation about your suggestion of new ArrayAsStream(arrayData) / new DatabaseAsStream(dbContext, tableName) is that this is what constructor overloading is made for. A constructor is named after what it creates, not the parameters that it is created from. Much more sensical, imo, would be new ExportStream(arrayData) / new ExportStream(dbContext, tableName) because it makes it clear that the result of either is the same (typewise and functionally). – Flater Jun 7 at 9:58
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.

  • 1
    @vargonian: see my last edit why your "weird feelings" may have a real, hard technical justification. – Doc Brown Jun 6 at 6:28
  • Thank you, yeah it's a challenging problem trying to abstract things away to the point at which I become uncomfortable at touching anything concrete, and it becomes a sort of "factories all the way down" situation. I think your first advice is probably appropriate, i.e. that I shoudn't overthink it because I'm probably not going to need it. But I'd love to have the tools for the instances when I do. – vargonian Jun 6 at 18:21
1

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.

0

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.

0

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.

  • I find it highly contradictory that the IDataReader reads from a different location than the IDataWriter writes to. That can of course happen, but the signatures you're using force IDataReader to read from a database and IDataWriter to write to a file path, which means it's not a matter of "it could be different", but rather "it must be different". Either these interfaces should be renamed (IDatabaseReader, IDatabaseToFileWriter), or you're going to have to seriously reevaluate the abstraction you're trying to enforce. – Flater Jun 7 at 9:51
  • Yeah sure, rename them. I originally had them named similarly to what you suggested. Don't think it's terribly relevant to the overall point. – Jared Goguen Jun 7 at 13:29

protected by gnat Jun 7 at 12:03

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.