0
\$\begingroup\$

Context

I have a project which has an importation module to import content of Excel files.

Originally, importation was possible only for .xls file (< 2003) through Interop assemblies through a pretty ugly non-modular architecture. But there was a need to support importation of .xlsx files.

I have refactored the module to build a system of data readers. Depending on the extension of the files that is imported, a data reader class is instantiated accordingly to be able to read the data from either an Excel <2003 file or an Excell >2003 file transparently.

So I have two classes Excel2003DataReader and OpenXmlDataReader both based on the following interface:

public interface IImporterDataReader: IDisposable
{
    object DataFile { get; }

    bool HasValue(string workSheetName, int rowIdx, int columnIdx);

    string GetValue(string workSheetName, int rowIdx, int columnIdx);

    List < RangeItem > GetRangeValues(string workSheetName, int rowIdx, int columnIdx, ExtractDirection direction = ExtractDirection.Horizontal, int amountExtraValues = 0);
}​

The IImporterDataReader instance is then passed a parameter to the Import method:

void Import(XmlDocument calculator, IImporterDataReader dataSource, Dictionary<string, string> parameters = null)

So dataSource.GetValue() can be used inside the method without worrying what kind of Excel file is actually imported.


The problem

The problem is that this system is too tightly adressed to Excel files. What I need to be able to reader xml files or json files as well ?

I was thinking of abstracting the way the parameters are passed to the methods HasValue()and GetValue() so they could be different per data reader implementations: have workSheetName, row and column properties for excel files, have an xpath property for xml files, etc.

Could be something like this:

public interface IImporterDataReader: IDisposable
{
    object DataFile { get; }

    bool HasValue(IDataAccessor accessor);

    string GetValue(IDataAccessor accessor);

    // Let's forget about this one right now
    //List < RangeItem > GetRangeValues(string workSheetName, int rowIdx, int columnIdx, ExtractDirection direction = ExtractDirection.Horizontal, int amountExtraValues = 0);
}​


Question

I haven't honestly tried implementing such a mechanism, this is still in my mind.

  • Is this an interesting path to go for to implement such a mechanism ?
  • Also what could the accessor look like ?

I have the feeling this is a bit too complicated for the purpose, but maybe not.

\$\endgroup\$
  • 1
    \$\begingroup\$ does implementing data importer for Json and XML files are really necessary ? Please be cautioned not to over refactor your code to achieve YAGNI. \$\endgroup\$ – Beatles1692 Feb 26 '12 at 10:45
  • \$\begingroup\$ I hear you indeed. No this is not required, i was more trying to think out of the box. I am still in the process of refactoring though and I have came up with this first solution, which works. The goal was to achieve a solution as flexible as possible without being overly complicated though and even if not implemented yet, that would be modular enough to be extended in the future. \$\endgroup\$ – Didier G. Feb 26 '12 at 11:04
  • \$\begingroup\$ You can also think of a generic output format (a DataTable for example) that you program will use to access the data that it needs and have different TableMakers for different input files.This approah will be more abstract I think :) \$\endgroup\$ – Beatles1692 Feb 26 '12 at 11:31
  • \$\begingroup\$ Please don't give me such ideas ^^ \$\endgroup\$ – Didier G. Feb 26 '12 at 11:41
  • 1
    \$\begingroup\$ @Didier G, Excel, XML and JSON formats are vastly different from one another: table, structured dictionary, unstructured dictionary. If I were you, I would not spend a lot of effort crafting perfect OO code that works around a crappy interface. It is a red herring - when I see well-written code that creates 0 StyleCop warnings, I mentally treat it as "complete". When I see funny-looking code, I slow down and start reading it more carefully. Instead of smacking everything with an OO hammer, think about the consumers of your code. What is the simplest way to use this "black box"? Make it happen. \$\endgroup\$ – Leonid Feb 26 '12 at 16:49
1
\$\begingroup\$

Since there should be a relation between your importer data reader and data accessor ( importer should know how to use data accessor to access data.) I suggest that you use generics to achieve such coherence.(Since each importer needs its unique data then IDataAccessors can't be replaced by each other thus I don't think that's a good idea to introduce an abstraction for them)

public abstract ImporterDataReader<TAccessor>:IImporterDataReader
{
public abstract bool HasValue(TAccessor accessor);

public abstract string GetValue(TAccessor accessor);
}

Then you can have concrete classes for each type of Accessor:

public class JsonDataReader:ImporterDataReader<JsonDataAccessor>
{
....
}
\$\endgroup\$
  • \$\begingroup\$ I will give this a try, seems interesting. Thanks for the contribution. \$\endgroup\$ – Didier G. Feb 26 '12 at 11:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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