The problem
Let’s say I have a class called DataSource which provides a ReadData method (and maybe others, but let’s keep things simple) to read data from an .mdb file:
var source = new DataSource("myFile.mdb");
var data = source.ReadData();
A few years later, I decide that I want to be able to support .xml files in addition to .mdb files as data sources. The implementation for “reading data” is quite different for .xml and .mdb files; thus, if I were to design the system from scratch, I’d define it like this:
abstract class DataSource {
abstract Data ReadData();
static DataSource OpenDataSource(string fileName) {
// return MdbDataSource or XmlDataSource, as appropriate
}
}
class MdbDataSource : DataSource {
override Data ReadData() { /* implementation 1 */ }
}
class XmlDataSource : DataSource {
override Data ReadData() { /* implementation 2 */ }
}
Great, a perfect implementation of the Factory method pattern. Unfortunately, DataSource is located in a library and refactoring the code like this would break all existing calls of
var source = new DataSource("myFile.mdb");
in the various clients using the library. Woe is me, why didn’t I use a factory method in the first place?
Solutions
These are the solutions I could come up with:
-
Make the DataSource constructor return a subtype (
MdbDataSourceorXmlDataSource). That would solve all my problems. Unfortunately, C# does not support that. -
Use different names:
abstract class DataSourceBase { ... } // corresponds to DataSource in the example above class DataSource : DataSourceBase { // corresponds to MdbDataSource in the example above [Obsolete("New code should use DataSourceBase.OpenDataSource instead")] DataSource(string fileName) { ... } ... } class XmlDataSource : DataSourceBase { ... }That’s what I ended up using since it keeps the code backwards-compatible (i.e. calls to
new DataSource("myFile.mdb")still work). Drawback: The names are not as descriptive as they should be. -
Make
DataSourcea “wrapper” for the real implementation:class DataSource { private DataSourceImpl impl; DataSource(string fileName) { impl = ... ? new MdbDataSourceImpl(fileName) : new XmlDataSourceImpl(fileName); } Data ReadData() { return impl.ReadData(); } abstract private class DataSourceImpl { ... } private class MdbDataSourceImpl : DataSourceImpl { ... } private class XmlDataSourceImpl : DataSourceImpl { ... } }Drawback: Every data source method (such as
ReadData) must be routed by boilerplate code. I don’t like boilerplate code. It’s redundant and clutters the code.
Is there any elegant solution that I have missed?
6
I would go for a variant to your second option that allows you to phase-out the old, too generic, name DataSource:
abstract class AbstractDataSource { ... } // corresponds to the abstract DataSource in the ideal solution
class XmlDataSource : AbstractDataSource { ... }
class MdbDataSource : AbstractDataSource { ... } // contains all the code of the existing DataSource class
[Obsolete("New code should use AbstractDataSource instead")]
class DataSource : MdbDataSource { // an 'empty shell' to keep old code working.
DataSource(string fileName) { ... }
}
The only drawback here is that the new base-class can’t have the most obvious name, because that name was already claimed for the original class and needs to remain like that for backwards compatibility. All the other classes have their descriptive names.
3
The best solution will be something close to your option #3. Keep the DataSource mostly as it is now, and extract just the reader part into its own class.
class DataSource {
private Reader reader;
DataSource(string fileName) {
reader = ... ? new MdbReader(fileName) : new XmlReader(fileName);
}
Data ReadData() {
return reader.next();
}
abstract private class Reader { ... }
private class MdbReader : Reader { ... }
private class XmlReader : Reader { ... }
}
This way, you avoid duplicate code, and are open to further extensions.
1