Is my base class constructed correctly?

Question

I have tables that use BulkManager to save data to database. I moved common functionality to BulkTableBase. Because creation of BulkManager depends on params of each Table I create the BulkManager in .ctor of the Table. The Add method of each Table is also different.

But I feel that calling CreateBulkManager method in .ctor is wrong! What if someone who derive from BulkTableBase would forget to call CreateBulkManager.

Is my BulkTableBase class constructed well?

public class Table1 : BulkTableBase 
{             
     public Table1(int param1, int param2)
     {
        _dt = new DataTable();
        _dt.Columns.Add(new DataColumn("Id", typeof(int)));
        _dt.Columns.Add(new DataColumn("Name", typeof(int)));

        _config = ConfigurationManager.GetSection("Table1_Config");
        CreateBulkManager(_dt, new Dictionary<string, object> { { "param1", param1 }, { "param2", param2 } }, _config);                            
     }

     public void Add(string name, int id)
     { 
          var row = _dt.NewRow();
          row["Name"] = name;
          row["Name"] = id;
          BulkManager.Add(row);
     }     
}

public class Table2 : BulkTableBase 
{        
     public Table2(int param3, string param4, byte param5)
     {
         _dt = new DataTable();
         _dt.Columns.Add(new DataColumn("CustomerId", typeof(int)));
         _dt.Columns.Add(new DataColumn("Address", typeof(string)));
         _dt.Columns.Add(new DataColumn("Tel", typeof(string)));

        _config = ConfigurationManager.GetSection("Table2_Config");
        CreateBulkManager(_dt, new Dictionary<string, object> { { "param3", param3 }, { "param4", param4 }, { "param5", param5 } }, _config);                              
     }

     public void Add(int customerId, string address, string tel)
     { 
           var row = _dt.NewRow();
           row["CustomerId"] = customerId;
           row["Address"] = address;
           row["Tel"] = tel;
          BulkManager.Add(row);
     }     
}

public abstract class BulkTableBase : IDisposable
{
     private SqlBulkManager _bulkManager;
     protected SqlBulkManager BulkManager { get { return _bulkManager; } }

     protected SqlBulkManagerBase CreateBulkManager(DataTable tableScheme, IDictionary<string, object> parameters, SqlBulkManagerConfig config)
     {
         var sqlParams = new List<SqlParameter>();

         foreach (var param in parameters)
         {
             sqlParams.Add(new SqlParameter(param.Key, param.Value));
         }

         _bulkManager = new SPSqlBulkManager(tableScheme, config, sqlParams);   
         _bulkManager.Start();           
     }

     // wait until all data is saved
     public void SaveAll()
     {
        BulkManager.Flush();
     }

     public void Dispose()
     {
         BulkManager.Stop();
     }
}

public class SqlBulkManager
{
    //...
    public SqlBulkManager(DataTable scheme, BulkManagerConfig config, IEnumerable<SqlParameter> extraParams)
    {
    }

    public void Add(DataRow item)
    {
        // add item to table and when number of rows reaches N the data is saved to DB
    }

    //...
}

UPDATED

I understand that I violate the Liskov substitution principle, but I wanted that eventually all bulks to be saved either exception occurred or not. Therefore, I needed BulkTableBase. So, I moved the code to FileParser->HandleWork method, creates BulkTable table there and pass to Parse method of the implemented parser.

public abstract class FileParser : MyThreadPool
{
    protected sealed override void HandleWork()
    {
        // some common checking
        //...
        BulkTableBase bulkTable = CreateBulkTable();

        try
        {   
            OnBeforeParse();
            Parse(bulkTable);
            OnAfterParse();
        }
        catch(Exception ex)
        { ... }
        finally
        {
           //...
           if (bulkTable != null)
           {
               // The BulkManager may have many bulks that hasn't saved yet, so by 
               //    calling "SaveAll()" it waits untill all threads accomplished the saving
               bulkTable.SaveAll();
               bulkTable.Dispose();
           }
        }       
    }

    protected abstract void Parse(BulkTableBase bulkTable, ...);
    protected abstract BulkTableBase CreateBulkTable();
}

public class Parser1 : FileParser
{
    protected override void Parse(BulkTableBase bulkTable, ...)
    {
       var table = (Table1)bulkTable;
        //...
    }

    protected override BulkTableBase CreateBulkTable()
    {
          return new Table1(...);
    }
}

Answer 1

Calling that function from sub-classes is not that bad, I think the problem is your classes are trying to do too much and are stepping on each others toes, so to speak. I'll touch on this as I go through this.

You are doing too much in the constructors for the Table#. The constructor should be to setup the class only. I think adding a record right in the constructor is awkward, especially since you have an .Add() method.

I also don't like that you are using ConfigurationManager within your class. That class / value should be injected.

This would also require a change to the BulkManager class to remove the parameters variable:

public SqlBulkManager(DataTable scheme, BulkManagerConfig config)
{
    // Assign as required
}

// -----

public Table1(SqlBulkManagerConfig config)
{
    var dataTable = new DataTable();
    dataTable.Columns.Add(new DataColumn("Id", typeof(int)));
    dataTable.Columns.Add(new DataColumn("Name", typeof(int)));

    AssignBulkManager(new SqlBulkManager(dataTable, config));
}

// -----

public Table2(SqlBulkManagerConfig config)
{   

    var dataTable = new DataTable();
    dataTable.Columns.Add(new DataColumn("CustomerId", typeof(int)));
    dataTable.Columns.Add(new DataColumn("Address", typeof(string)));
    dataTable.Columns.Add(new DataColumn("Tel", typeof(string)));

    AssignBulkManager(new SqlBulkManager(dataTable, config));
}

Having to create the DataRow in the add method on your tables is a code smell. You should just modify the parameters and pass off to the SqlBulkManager. This will allow the DataTable to be reference only in one class, SqlBulkManager.

Add an AddToBulkManager method to BulkTableBase which accepts an IEnumerable>. The parameter should just be passed off to the SqlBulkManager.

protected void AddToBulkManager(IEnumerable<KeyValuePair<string, object>> item)
{
    BulkManager.Add(item);
}

You would have to change the signature of the Add function in SqlBulkManager to accept an IEnumerable>.

public void Add(IEnumerable<KeyValuePair<string, object>> item)
{
    var row = _scheme.NewRow();

    // Do some error checking to make sure item is compatible

    foreach (var field in item)
    {
        row[field.Key] = field.Value;
    }

    // More Processing
}

now your Add() method in Table1, and Table2 would be

// Table1
public void Add(string name, int id)
{
    var parameters = new Dictionary<string, object> { { "Id", param1 }, { "Name", param2 } };

    AddToBulkManager(parameters);
}

// Table2
public void Add(int customerId, string address, string tel)
{
    var parameters = new Dictionary<string, object> { { "CustomerId", customerId }, { "Address", address }, { "Tel", tel } };

    AddToBulkManager(parameters);
}

Important

Your Table# classes are a major violation of the Liskov Substitution Principle. Having the Add() method in each one accept different parameters means that Table1 could not be substituted for Table2, and vice versa.

It would be better to remove the super class BulkTableBase from each one. Rename BulkTableBase to TableProcessor(or something that makes sense in your app). You will also have to remove the abstract from the declaration

public class TableProcessor : IDisposable

You could then either inject that class, or instantiate the class in your Table# classes

public class Table1
{
    private readonly TableProcessor _tableProcessor;

    public Table1(SqlBulkManagerConfig config)
    {
        _tableProcessor = new TableProcessor();

        var dataTable = new DataTable();
        dataTable.Columns.Add(new DataColumn("Id", typeof(int)));
        dataTable.Columns.Add(new DataColumn("Name", typeof(int)));

        _tableProcessor.AssignBulkManager(new SqlBulkManager(dataTable, config));
    }

Or injected

public class Table2 : BulkTableBase
{
    private readonly TableProcessor _tableProcessor;

    public Table2(TableProcessor tableProcessor, SqlBulkManagerConfig config)
    {   

        _tableProcessor = tableProcessor;

        var dataTable = new DataTable();
        dataTable.Columns.Add(new DataColumn("CustomerId", typeof(int)));
        dataTable.Columns.Add(new DataColumn("Address", typeof(string)));
        dataTable.Columns.Add(new DataColumn("Tel", typeof(string)));

        _tableProcessor.AssignBulkManager(new SqlBulkManager(dataTable, config));
    }

To wrap this up, here is the entire code base, I think it looks a lot cleaner and is much easier to follow:

You might have to implement IDisposable in the Table# classes, without a full code base to test, I'm not sure how that will work.

public class Table1
{
    private readonly TableProcessor _tableProcessor;

    public Table1(SqlBulkManagerConfig config)
    {
        _tableProcessor = new TableProcessor();

        var dataTable = new DataTable();
        dataTable.Columns.Add(new DataColumn("Id", typeof(int)));
        dataTable.Columns.Add(new DataColumn("Name", typeof(int)));

        _tableProcessor.AssignBulkManager(new SqlBulkManager(dataTable, config));
    }

    // wait until all data is saved
    public void SaveAll()
    {
        _tableProcessor.SaveAll();
    }

    public void Add(string name, int id)
    {
        var parameters = new Dictionary<string, object> { { "Id", param1 }, { "Name", param2 } };

        _tableProcessor.Add(parameters);
    }
}

public class Table2
{
    private readonly TableProcessor _tableProcessor;

    public Table2(TableProcessor tableProcessor, SqlBulkManagerConfig config)
    {
        _tableProcessor = tableProcessor;

        var dataTable = new DataTable();
        dataTable.Columns.Add(new DataColumn("CustomerId", typeof(int)));
        dataTable.Columns.Add(new DataColumn("Address", typeof(string)));
        dataTable.Columns.Add(new DataColumn("Tel", typeof(string)));

        _tableProcessor.AssignBulkManager(new SqlBulkManager(dataTable, config));
    }

    // wait until all data is saved
    public void SaveAll()
    {
        _tableProcessor.SaveAll();
    }

    public void Add(int customerId, string address, string tel)
    {
        var parameters = new Dictionary<string, object> { { "CustomerId", customerId }, { "Address", address }, { "Tel", tel } };

        _tableProcessor.Add(parameters);
    }
}

public class TableProcessor : IDisposable
{
    private SqlBulkManager BulkManager { get; set; }

    public void AssignBulkManager(SqlBulkManager manager)
    {
        if (BulkManager != null && BulkManager.IsStarted)
        {
            // Cleanup somehow
        }

        BulkManager = manager;
        BulkManager.Start();
    }

    public void Add(IEnumerable<KeyValuePair<string, object>> item)
    {
        BulkManager.Add(item);
    }

    // wait until all data is saved
    public void SaveAll()
    {
        BulkManager.Flush();
    }

    public void Dispose()
    {
        BulkManager.Stop();
    }
}

public class SqlBulkManager
{
    //...
    public SqlBulkManager(DataTable scheme, BulkManagerConfig config)
    {
    }

    public void Add(IEnumerable<KeyValuePair<string, object>> item)
    {
        var row = _scheme.NewRow();

        // Do some error checking to make sure item is compatible

        foreach (var field in item)
        {
            row[field.Key] = field.Value;
        }

        // More Processing
    }

    //...
}