Using nested private class to hold data from csv file

Question

I have a class that only has one method. This method has to read csv file, do some work concerning internal logic (check if item already exists, do some transformations, etc.) and finally write all data to a database. I am using a nested class for storing data from csv file as a list of objects. It looks something like this:

public class Importer
{
    public void Import(string path)
    {
        using(var reader = File.OpenText(path))
        {
            var csv = new CsvReader(reader);
            var items = csv.GetRecords<ItemCsvModel>().ToList();
            //do some work and insert items to db
        }
    }

    private class ItemCsvModel
    {
        public int Id {get; set;}   
        public string Name {get; set;}
        public string Description {get; set;}
    }
}

Code reviewer didn't like using nested classes told me that it would be better if this class would be moved to another file. I think that this class is just an internal implementation detail and it has no reason to be visible outside the Importer class. I could've just as easily read that csv file without using any class by simply doing something like this:

var id = csv.GetField<int>("Id");
var name = csv.GetField<string>("Name");
var description = csv.GetField<string>("Description");

But I chose another approach. At the moment there are no other places where csv files are read and I doubt it will change any time soon so I am not repeating any class declarations over and over again. What are your thoughts?

Answer 1

I don't see any issue with it.

Maybe down the road if you want to implement a different type of parser (xml, let's say), but in that case you pull out the existing csv implementation, give it a common interface and inject it (or the xml parser) as an IParser or whatever.

That's one reason it'd make sense to move it out, but I'm also a firm believer in not over-designing for code that a) isn't presently needed, and b) can be easily refactored out if a need arises.

To me, this fits both a and b criteria above. I don't follow XP much at all really, but the YAGNI mindset is one of the best nuggets of wisdom I've gained in OOP (YAGNI: You Aren't Gonna Need It).

(once I read a great article about this type of over-engineering in the context of Java... the author started out with a simple class and method that would have worked just fine, gotten then job done, then went through a "typical" thought process of how everything was "supposed" to be, and by the end the class was such a mess you couldn't even tell what it was doing... would love if someone has the link, I seemed to have lost it).

Answer 2

I'd say that your class is doing too many things. It's does file operations, CSV parsing, calculations, and database storage. That's the real problem. This all violates the Single-Responsibility Principle (S in SOLID). If you split this up into at least four classes, you might see a better solution. The other commenter makes a good point about what if the file changes from CSV to XML or JSON or anything else. Think about that and ask yourself why the class that handles database access should change if your file format changes. It shouldn't.

Answer 3

I have the very same solution for CSV exporters. Every exporter has its own transformer (entity-object[]).

Transformer is a private inner class . My solution implements my hierarchy of interfaces ICSVExporter, ICSVEntityTransformer. Its my way to proupose the way to do for further developments (and the way to avoid juniors making bolied-plate code)

Project's architect gave me his OK. It was just fine.

Your reviwer is giving his opinion. Ask him about his reasons. I bet you will have an answer for his doubts and fears.

Your code is clear, simple and FUNCTIONAL.

To think to much in further upgrades is over-reenginery.

I will only suggest to have Reader and Processor in different components. Who reads may or may not to be who process the input data.

Think in differents way to load CSV files:

• FileSystemCsvReader
• FtpCsvReader
• WebserviceCsvReader
• QueueCsvReader
• WhateverITComeFromCsvReader

All of them do the same task but in diferent ways. But the way to parse data into Java objects could be all the same.

Do you have different CSVs? Then you could have 1 Processor and N Transformers. Then yes, get that inner class out from the Processor.

...if you do a flexible solution. Even with inner classes it can be done.