views:

97

answers:

4

I'm creating a CSV reader (yes I know about Fast CSV Reader and FileHelpers). The CsvReader class uses the CsvParser class to parse the CSV file. I want to make the CsvReader class unit testable, so I want to be able to externally set the CsvParser class that is used (also, so you can create your own implementation). I also, don't want to have to create a parser and pass it in on normal use.

This is how I would like to use it.

var reader = new CsvReader( "path/to/file.csv" );

When doing this, I could create the CsvParser in the constructor of the CsvReader and have a property to change the parser.

public ICsvParser Parser { get; set; }

public CsvReader( filePath )
{
    Parser = new CsvParser( filepath );
}

But then when unit testing, the default parser is always created and I only want to test the CsvReader.

The parser could be passed into the constructor, but I don't want to have to create a parser separately on normal use. This seems like a good place for a factory.

This seems like it would be a common problem when using IOC. What is a good solution for this?

+5  A: 

The solution is to rewrite your constructor of CsvReader to accept an implementation of ICsvParser and your concrete implementation of ICsvParser should have a constructor taking in its dependencies (a path to a file to parse) and an already constructed ICsvParser should be injected into the constructor for CsvReader:

public CsvReader(ICsvParser parser) {
    this.Parser = parser;
}

The ICsvParser should already be constructed to accept its dependency (a path to the file to be parsed).

Thusly:

// path is string containing path to file to parse
ICsvParser parser = new SomeCsvParser(path);
ICsvReader reader = new CsvReader(parser);

The point is that CsvReader does not need a path, it just needs a CsvParser. Further, CsvReader should not need to be aware of the dependencies of CsvParser (that it needs a path to a file to parse) lest it becomes dependent on those dependencies too.

new inside of constructors is a smell.

Jason
I agree with this, but I don't want consumers of the library to have to create a parser when creating the reader. Would it be preferable to have another constructor that I just pass the path in and it creates the parser and uses the given path for it? Or would it be better to use a factory that takes in the file path, and creates the reader and parser and returns the reader?
Josh Close
If you don't want the user to have to create a parser, then yes, your best option is to have a factory that eats paths and returns a constructed `ICsvReader`. You should still set up `CsvParser` and `CsvReader` as I've described. I said `new` inside of constructors is a small. A second smell is object graph construction inside a constructor. That is, the constructor for `CsvReader` should not be taking in dependencies for other objects (`CsvParser`) and using that to construct itself. That job should be delegated to a factory. SRP and all that.
Jason
Great. I originally had it setup this way, but I didn't like requiring the parser to be passed into the constructor for normal use. I'll just use a factory for easy/default creation and everything should be good.
Josh Close
What are your thoughts on putting a static Create method on CsvReader that returns an ICsvReader VS creating a separate CsvReaderFactory class to do the creation? There is no type being passed in and no decision making happening, so a full factory class seems a little much.
Josh Close
@Josh: that solution has the same quality as the one I suggested to you - it follows the YAGNI rule ("You Aint Gonna Need It (yet)"). If you don't need a complete factory class, better write a simple factory method first, and when you later think a factory class is better, refactor.
Doc Brown
As an aside this pattern is technically a builder rather than a factory, but semantics schemantics :)
Paolo
@Paolo: Sorry, but as described it is a factory pattern. In the factory pattern, the factory will output an instance of a concrete type. The builder pattern abstracts the building process (so `BuildEngine`, `BuildChassis`, `BuildAxle`) and there is another object (a `Director`) that coordinates the aggregation of these parts into the whole (`Civic` or `Odyssey`, for example).
Jason
@Paolo: as Wikipedia says (http://en.wikipedia.org/wiki/Factory_method_pattern), the term "factory method" is often just used to refer to any method whose main purpose is creation of objects. This is slightly more general than the use of the term "factory" in the GoF book.
Doc Brown
A: 

Create two constructors, one with the parser interface as a parameter, and one just with filePath. Let the the second one create CsvParser and let it call the first one with this object. Your test code then can use the first constructor and pass a mock CsvParser.

This solution has one drawback: the assembly containing CsvReader has to reference the one containing CsvParser. You have to decide by yourself if this ok or not in your situation.

Doc Brown
A: 

Typically you would have the parser passed in via the constructor and create your object using the IoC container, this would then inject a parser instance into the constructor for you.

In unit testing you would pass in a mock parser either directly, e.g. new CsvReader(new MockParser()) or by configuring your IoC with a test configuration that injected the mock for you.

Paolo
+3  A: 

If this were IoC you'd be passing in the CsvParser and not the path.

chillitom