#200 CachedDataSet has redundant code.

[Hans Deragon]

CachedDataSet does not make use of member fields and functions inherited from AbstractDataSet. If it did, the code would be simpler, opening the way for easier maintenance and extension.

[Jeff Jensen]

Hi Hans, any further thoughts or progress on this?

[Jeff Jensen]

Completed with merge request 30

[Anghel Botos]

This commit actually breaks CachedDataSet...just look at the constructor:

    /**
     * Creates a copy of the specified dataset.
     */
    public CachedDataSet(IDataSet dataSet) throws DataSetException
    {
        super(dataSet.isCaseSensitiveTableNames());
        initialize();
    }

How can you create a (in-memory) copy of a dataset if the only thing you're using from the original dataset is its case sensitivity flag???

Please put back the original code in there which does the actual copying of the input dataset, i.e.:

        ITableIterator iterator = dataSet.iterator();
        while (iterator.next())
        {
            ITable table = iterator.getTable();
            _tables.add(table.getTableMetaData().getTableName(), new CachedTable(table));
        }

One more remark...the default constructor, i.e.:

    /**
     * Default constructor.
     */
    public CachedDataSet() throws DataSetException {
        super();
        initialize();
    }

makes no sense whatsoever in a class whose purpose (according to its own JavaDoc) is to: "Hold copy of another dataset or a consumed provider content."...what is the "other" dataset when this constructor is used?...nothing??? Please remove the default constructor in order to make the class consistent with its intended purpose.

BR,
Anghel

[Hans Deragon]

Greetings Anghel,

What technical error are you getting? All unit tests pass. Instead of using a duplicate OrderedTableNameMap the fix make use of the one found in the parent class, like all other classes. It is possible that the test suite missed something, but I am curious what the bugs are. Can you please describe them?

Best regards,
Hans Deragon

[Anghel Botos]

The problem is that this CachedDataSet does not copy any more the original data set (as it states in the JavaDoc). It basically does nothing with the original data set in the constructor...hence it's just an empty dataset.

[Anghel Botos]

The tests pass because ALL of them use the constructor based on a producer...but there is NO test using the constructor based on another data set.

[Hans Deragon]

Wow, you are right! How could I have miss this?!? I was too confident with the tests. The change was part of a refactoring and I am trully sorry that I missed this serious bug I introduced.

I do not believe that a full revert is necessary. I have changed the current constructor for this one and added a test for it. Up to now, this seams to fix the problem. I have not finished the work, but in the next few days I will submit a new branch for the fix. I also removed the constructor that is taking no arguments. Your opinion is mostly welcomed.

/**
 * Creates a copy of the specified dataset.
 */
public CachedDataSet(IDataSet dataSet) throws DataSetException
{
    super(dataSet.isCaseSensitiveTableNames());
    initialize();

    ITableIterator iterator = dataSet.iterator();
    while (iterator.next())
    {
        ITable table = iterator.getTable();
        _orderedTableNameMap.add(table.getTableMetaData().getTableName(), new CachedTable(table));
    }
}

[Hans Deragon]

I believe that the fix I proposed to the CachedDataSet(IDataSet dataSet) constructor does the job. As for the default constructor without arguments, I found the following code in one of the DbUnit test cases. There is a scenario where it can be useful.

private IDataSet produceToMemory(String source) throws DataSetException {
    CsvProducer producer = new CsvProducer(source);
    CachedDataSet cached = new CachedDataSet();
    producer.setConsumer(cached);
    producer.produce();
    return cached;
}

[Jeff Jensen]

Please also refer to bug [bugs:#424] for this.