When it comes to "reader" classes in C#, there's a few patterns they generally abide to. A few things they have in common. I would recommend repeating those patterns.
1 - Encapsulate Another Reader
As you've seen,
CsvReader encapsulates, depends on a
TextReader. The reader classes in the
System.IO namespace do similar.
StreamReader takes a Stream. So does
StringReader takes a string.
This dependency is injected via the constructor and represents everything the class needs to do its work (no more, no less).
So ask yourself two things:
- What is your classes responsibility? Try to think of the one thing that it is responsible for.
- What is the minimum your class needs to fulfill that responsibility?
In the case of your
CsvCountFile class, what's it trying to do? It looks to me like you're trying to extend Csv reading for a specialised case. Now, is the class representing a reader, or is it representing a file? My earlier comment asked if the
AbsolutePath property was strictly necessary. If the class is a reader, it doesn't need to know the filename, it just needs a less-specific reader to read from.
So following the reader patterns, you would:
- Expose a constructor that accepts another reader class. The choice of
CsvReader is up to you. If it accepts a
CsvReader you're being more specific, which is good, but it exposes the tight-coupling your class has to it, which means the dependency for that library will leak into other parts of your system. Accepting
TextReader means you more effectively encapsulate the inner workings, which is probably what you're wanting here. Don't expose a constructor with no arguments. The class is useless without another reader, so make that explicit.
- Rename the class with a suffix of "Reader", to clearly identify the responsibility of this class.
2 - Read the Next Part vs Read to End
CsvReader and other readers from the
System.IO namespace have (more or less) two sorts of read methods.
- Read the next part of the source and set a
CurrentRecord property on the reader, returning a bool (or some other indication) to the caller to indicate a successful read.
- Read to the end, or end of the next significant segment, and return the record to the caller.
In the case of
CsvReader, look at its
.Read() method for an example of the first type. It returns a
bool indicating whether a record was read, and sets the
System.IO.TextReader for an example of the second type of read operation. It returns a "string that contains all characters from the current position to the end of the text reader".
In the case of the first type of read method, the state of the reader after the method call is important. A consumer of the class will call
.Read(), then look at the
CurrentRecord property, perform some logic, repeat.
The second type of read method doesn't depend on the state of the reader once the read is complete. A consumer will create the reader, call
.ReadToEnd(), then dispose of the reader because they've got what they need.
.ReadCountData() method doesn't really fit into either of these categories. It takes a bit from both techniques by reading all the way to the end, but then storing the result on the reader itself. I would recommend going one way or the other. Either have a method that:
- Advances the reader by one full record and exposes that record as a
CurrentRecord property, or;
- Reads to the end and returns the result instead of storing it as a property on the reader.
For the first method, because you're returning two different types of records (
CountDefinition), things get a little awkward for a single
CurrentRecord property. Without knowing more about those classes it's hard to say, but would it be reasonable to introduce a common base class for them? The readers
CurrentRecord property could be of type
Definition, which will either be an instance of
CountDefinition, depending on the last record type read.
For the second method, you could encapsulate the two types into a single "result" class.
Whether you want one, both, or the other type of read method depends on your needs. If you want to be able to perform logic part-way through reading, then you'll want the first type of read method. For example, maybe you abandon the read operation because of some data found in the
LocationDefinition? If not, then I'd lean towards the second type of read method to keep things a little simpler.
One of the biggest challenges for testable code is dependencies. If you have code that depends on zero infrastructure, it's much easier to test. By infrastructure I mean things like the file system, database, network, etc.
Because you're relying on
CsvReader, you're stuck with its dependency (
TextReader) at a minimum. If you keep your implementation to the bare minimum though (as I suggested above on point 1), then you at least won't need any more dependencies.
TextReader has an inheritor that makes testing a bit simpler.
StringReader you can rely on in-memory strings for your testing, which will make it easy to write self-contained tests. Something to remain wary of though is character encoding. It's easy to dodge the issue of character encoding when you're using a
StringReader, whereas a
StreamReader, which you'll presumably expect to be using "in production" can be affected by file character encoding. It's your call as to whether this is likely to be an issue.