5
\$\begingroup\$

I'm writing adapters for some MS Access database because I really didn't like that automatically generated code that Visual Studio was providing. Right now I just need to get data, not updating anything over there, but that will come later.

Here's some code that I'm using (all the adapters look the same, so that is what bothers me if I can fix them up somehow). I'm wondering about moving most of the methods to the base class and just passing kind of Filter class as an argument to form an actual query, but I'd like to see some different approaches on that problem.

public abstract class AccessDatabaseAdapter
{
    private readonly OleDbConnection _connection;
    private OleDbDataAdapter _adapter;

    public OleDbDataAdapter Adapter
    {
        get
        {
            if (_adapter == null)
            {
                _adapter= new OleDbDataAdapter(String.Empty, _connection);
            }
            return _adapter;
        }
    }


    protected AccessDatabaseAdapter()
    {
        var parser = new MSAccessConnectionStringParser();
        _connection = new OleDbConnection(parser.GetConnectionString());
    }


    private void SetSelectCommand(string command)
    {
        Adapter.SelectCommand = new OleDbCommand(command);
        Adapter.SelectCommand.Connection = _connection;
    }

    protected DataTable GetTable(string command)
    {
        var dataTable = new DataTable();
        SetSelectCommand(command);
        Adapter.Fill(dataTable);

        return dataTable;
    }

    protected abstract bool RowIsInvalid(DataRow row);
}

public interface ITableRetrieveAdapter<T>
{
    T GetWithId(int id);
    IList<T> GetAll();
}

public class MsAccessComponentRetrieveAdapter : AccessDatabaseAdapter, ITableRetrieveAdapter<Component>
{
    protected override bool RowIsInvalid(DataRow row)
    {
        return row == null || row[0] is DBNull || row[1] is DBNull || row[2] is DBNull;
    }

    public Component GetWithId(int id)
    {
        var selectCommmand = string.Format("SELECT * from tblComponent WHERE ComponentID = {0}", id);

        return RowToComponent(GetTable(selectCommmand).Rows[0]);
    }

    public IList<Component> GetAll()
    {
        var selectCommmand = "SELECT * from tblComponent";

        return TableToComponents(GetTable(selectCommmand));
    }

    private IList<Component> TableToComponents(DataTable table)
    {
        return (from DataRow row in table.Rows 
                select RowToComponent(row)).ToList();
    }

    private Component RowToComponent(DataRow row)
    {
        if (RowIsInvalid(row))
        {
            return null;
        }

        return new Component((int)row[0], (string)row[1], (string)row[2]);
    }
}

public class MsAccessEmployeeRetrieveAdapter : AccessDatabaseAdapter, ITableRetrieveAdapter<Employee>
{
    protected override bool RowIsInvalid(DataRow row)
    {
        return row == null || row[0] is DBNull || row[1] is DBNull || row[2] is DBNull;
    }

    public Employee GetWithId(int id)
    {
        var selectCommand = string.Format("SELECT * from tblOperator WHERE Operatornumber = {0}", id);

        return RowToEmployee(GetTable(selectCommand).Rows[0]);
    }

    public IList<Employee> GetAll()
    {
        var selectCommand = "SELECT * from tblOperator";

        return TableToEmployees(GetTable(selectCommand));
    }

    private IList<Employee> TableToEmployees(DataTable table)
    {
        return (from DataRow row in table.Rows
            select RowToEmployee(row)).ToList();
    }

    private Employee RowToEmployee(DataRow row)
    {
        if (RowIsInvalid(row))
        {
            return null;
        }

        return new Employee((int)row[2], (string)row[0], (string)row[1]);
    }
}
\$\endgroup\$

4 Answers 4

7
\$\begingroup\$

Don't do SELECT *, especially not when you later rely on specific fields to be returned in a specific order: (int)row[0], (string)row[1], (string)row[2].

There are really three major reasons:

  • Inefficiency in moving data to the consumer.
  • Indexing issues.
  • Binding Problems.

I never see you close your OleDbConnection anywhere in your code. That worries me. IMHO you should look at something like this to fill a DataTable:

DataSet ds = new DataSet();

using (OleDbConnection connection = new OleDbConnection(connectionString))
using (OleDbCommand command = new OleDbCommand(query, connection))
using (OleDbDataAdapter adapter = new OleDbDataAdapter(command))
{
    adapter.Fill(ds);
}

return ds;

Granted, that example is for a DataSet, but judging from your code it should be much the same for a DataTable.


But I would really urge you to use an ORM like Entity Framework or NHibernate instead of data adapters etc.

\$\endgroup\$
4
  • \$\begingroup\$ I'll probably switch to Entity Framework when I'll replace database with proper one in Sql, thanks for mentioning SELECT * issue, replacing that will come in handy. Also the disposing thing which I'm little bit ashamed of now :) \$\endgroup\$
    – yoger
    Jun 18, 2015 at 11:09
  • \$\begingroup\$ I've actually have a question about using, if I pass command to the method and place it within that statement, will it get properly disposed? \$\endgroup\$
    – yoger
    Jun 18, 2015 at 15:51
  • \$\begingroup\$ @yoger Just make sure the method call is inside a using, e.g. using (var command = new OleDbCommand(query, connection)) { MyMethod(command); } \$\endgroup\$
    – BCdotWEB
    Jun 18, 2015 at 17:30
  • \$\begingroup\$ Yes, but in this case method will be taking string as a parameter, which is query can be injected, right? If I'd pass command with parameters instead of string it would be safer, wouldn't it? In this case will command be disposed after using it this way using ( command), which is parameter? \$\endgroup\$
    – yoger
    Jun 18, 2015 at 18:22
7
\$\begingroup\$

Sql Injection

What you've shown isn't actually vulnerable to Sql injection because you're only using ints but the way you are writing your queries is a terrible habit to get into.

string.Format("SELECT * from tblOperator WHERE Operatornumber = {0}", id);

Now, because id is an int, you can't inject anything malicious, but what if you wanted to get the operator by name? Do you promise that you wouldn't write:

string.Format("SELECT * FROM tablOperator WHERE OperatorName = {0}", operatorName);

You should use parameterised queries.

Other comments

I prefer to see all SQL keywords capitalised or not (I personally just go all lower case and face the wrath of my colleagues), a schema on your table name, PascalCased column names and a list of columns to be returned.

Also confused about the name "tblOperator" - it's fairly obvious that it's a table you don't need to prefix it.

All of the other comments I'd make about not disposing resources etc have been mentioned already but one other thing I'd note is that I've worked with data access layers (written by others ;)) passing around data tables - it becomes an absolute disaster. You should return strongly typed objects, e.g. an Operator class. You could invest in adding an ORM like EF or NHibernate but you can also write the rehyrdation code yourself.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the comment, unfortunately I'm in this terrible position to have old database made by someone wit MS Access and I have to stick with it for now, so thats where all those strange names and prefixes come from. I'll totally forgot about disposing objects and probably start to get in habit building queries with parameters. \$\endgroup\$
    – yoger
    Jun 18, 2015 at 11:02
5
\$\begingroup\$

This isn't what you asked for, but it's another point to consider.

private IList<Employee> TableToEmployees(DataTable table)
{
    return (from DataRow row in table.Rows
        select RowToEmployee(row)).ToList();
}

While returning the most derived type you have generally makes sense, here you're actually returning more than you have. You're calling .ToList() just to get it, but ToList() has another problem, it completely voids all lazy evaluation.

If you swap the method to this:

private IEnumerable<Employee> TableToEmployees(DataTable table)
{
    return (from DataRow row in table.Rows
        select RowToEmployee(row));
}

You get the benefit that the RowToEmployee() method is only executed on the rows you actually want to deal with, since the Select method is lazily evaluated. If you want to grab the first item that meets your criteria, RowToEmployee() will only be executed on the rows up to and including the one that meets your.

\$\endgroup\$
0
0
\$\begingroup\$

Alright as no one had ideas on how to do that different way, I took the code & learn path, which lead me to re-designing adapters little bit. As I researched how the database looks like, it wasn't possible to create common interface without violating Interface Segregation Principle. Also decided to go for asynchronous DB operations to make program run smoother.

public interface IOrderTableAdapter
{
    Task<IEnumerable<Order>>  GetOrders(OrderFilter filter);
}

public interface IOrderItemTableAdapter
{
    Task FillOrder(Order order);
}

public interface IOrderItemCutTableAdapter
{
    Task InsertAndSetId(ProductionCut cut);
    Task FillItem(OrderedItem item);
}

Now the base class for all the adapters, it is responsible for executing queries:

public abstract class AccessDatabaseAdapter
{
    private readonly MsAccessConnectionStringGenerator _connectionStringGenerator;


    protected AccessDatabaseAdapter()
    {
        _connectionStringGenerator = new MsAccessConnectionStringGenerator();
    }


    protected async Task Insert(OleDbCommand command)
    {
        using (var connection = new OleDbConnection(_connectionStringGenerator.GetConnectionString()))
        {
            using (command)
            {
                command.Connection = connection;

                try
                {
                    connection.Open();
                    await command.ExecuteNonQueryAsync();
                }
                catch (Exception e)
                {
                    Console.WriteLine(e);
                }
                finally
                {
                    connection.Close();
                }
            }
        }
    }

    protected async Task<DataTable> GetTable(OleDbCommand command)
    {
        var dataTable = new DataTable();
        await Task.Run(() =>
        {
            try
            {
                using (var connection = new OleDbConnection(_connectionStringGenerator.GetConnectionString()))
                {
                    using (command)
                    {
                        command.Connection = connection;
                        using (var adapter = new OleDbDataAdapter(command))
                        {
                            adapter.Fill(dataTable);
                        }
                    }
                }
            }
            catch (Exception e)
            {
                Console.WriteLine(e);
            }
        });

        return dataTable;
    }

    protected abstract bool RowIsInvalid(DataRow row);
}

Last decision was really tough but I wanted to cut connections to minimum, because sometimes generating of single Order was taking too much time due to OrderItemcomplexity so each adapter has static list that gets filled with items the first time I request any information. Also thanks to the answers above I started to pass OleDbCommands instead of string commands to the base class and stopped using Select *.

public class MsAccessOrderAdapter : AccessDatabaseAdapter, IOrderTableAdapter
{
    private static List<Order> _Orders; 

    private readonly MsAccessOrderItemAdapter _orderItemAdapter = new MsAccessOrderItemAdapter();


    public async Task<IEnumerable<Order>>  GetOrders(OrderFilter filter)
    {
        if (_Orders == null)
        {
            var table = await GetTableAsync();
            await TableToOrders(table);
        }

        return filter.GetFilteredOrders(_Orders);
    }

    private async Task<DataTable> GetTableAsync()
    {
        var command = new OleDbCommand("SELECT OrderID, OrderNumber, DueDate " +
                                       "from tblOrder " +
                                       "ORDER BY OrderID DESC");
        return await GetTable(command);
    }


    private async Task TableToOrders(DataTable table)
    {
        _Orders = new List<Order>();

        foreach (DataRow row in table.Rows)
        {
            var order = await RowToOrder(row);

            if (order != null)
            {
                _Orders.Add(order);
            }
        }
    }

    private async Task<Order> RowToOrder(DataRow row)
    {
        if (RowIsInvalid(row))
        {
            return null;
        }

        var order = new Order((int) row[0], (string) row[1], (DateTime) row[2]);
        await _orderItemAdapter.FillOrder(order);

        return order;
    }


    protected override bool RowIsInvalid(DataRow row)
    {
        return row == null || row[0] is DBNull || row[1] is DBNull || row[2] is DBNull;
    }
}

public class MsAccessOrderItemAdapter : AccessDatabaseAdapter, IOrderItemTableAdapter
{
    private static List<OrderedItem> _Items; 

    private MsAccessOrderItemCutAdapter _cutAdapter = new MsAccessOrderItemCutAdapter();
    private MsAccessComponentAdapter _componentAdapter = new MsAccessComponentAdapter();
    private MsAccessBarSizeAdapter _barSizeAdapter = new MsAccessBarSizeAdapter();


    public async Task FillOrder(Order order)
    {
        if (_Items == null)
        {
            var table = await GetTableAsync();
            await TableToItems(table);
        }

        order.Items = new ObservableCollection<OrderedItem>(_Items.Where(x=>x.OrderId == order.Id));
    }


    private async Task<DataTable> GetTableAsync()
    {
        var command = new OleDbCommand("SELECT LineItemID, SideBarMark, BarSizeID, " +
                                       "BarOrderLengthMM, BarCutLengthMM, End1ComponentID, End2ComponentID, " +
                                       "BendComponentID, OrderQuantity, OrderID " +
                                       "from tblOrderItem ");

        return await GetTable(command);
    } 

    private async Task TableToItems(DataTable table)
    {
        _Items = new List<OrderedItem>();

        foreach (DataRow row in table.Rows)
        {
            var item = await RowToOrderItem(row);

            if (item != null)
            {
                _Items.Add(item);
            }
        }
    }

    private async Task<OrderedItem> RowToOrderItem(DataRow row)
    {
        if (RowIsInvalid(row))
        {
            return null;
        }

        var sbm = string.Empty;
        if (row[1] is string)
        {
            sbm = (string)row[1];
            if (Settings.Default.IgnoreTestBars && sbm == "Test Bar")
            {
                return null;
            }
        }

        var components = await GetComponents(row);
        var bar = await _barSizeAdapter.GetByIdAsync((int) row[2]);
        var item = new OrderedItem((int)row[0], sbm, bar, (double)row[3],
                                   (double) row[4], components[0], 
                                   components[1], components[2], (double) row[8], (int)row[9]);

        await _cutAdapter.FillItem(item);

        return item;
    }

    private async Task<Component[]> GetComponents(DataRow row)
    {
        var components = new Component[3];

        components[0] = row[5] is int? await _componentAdapter.GetByIdAsync((int)row[5]) : null;
        components[1] = row[6] is int? await _componentAdapter.GetByIdAsync((int)row[6]) : null;
        components[2] = row[7] is int? await _componentAdapter.GetByIdAsync((int)row[7]) : null;


        return components;

    }


    protected override bool RowIsInvalid(DataRow row)
    {
        return row == null || row[0] is DBNull || row[2] is DBNull ||
               row[3] is DBNull || row[4] is DBNull || row[8] is DBNull || row[9] is DBNull;
    }

}

Now the ProductionCut adapter is also supposed to upload to the database.

    public class MsAccessOrderItemCutAdapter : AccessDatabaseAdapter, IOrderItemCutTableAdapter
{

    private static List<ProductionCut> _Cuts; 
    private MsAccessEmployeeAdapter _employee = new MsAccessEmployeeAdapter();
    private MsAccessRebarHeatAdapter _barHeat = new MsAccessRebarHeatAdapter();


    public async Task InsertAndSetId(ProductionCut cut)
    {
        cut.Id = await GetNextIndex();

        var command = new OleDbCommand("INSERT INTO tblOrderItemProdCut " +
                                       "(ProdLine, LineItemId, OrderID, CutDate, CutQty, Operator, BarHeat) " +
                                       "VALUES (@prodLine, @itemId, @orderId, @date, @quantity, @operator, @heat)");
        command.Parameters.Add(new OleDbParameter("prodLine", cut.Id));
        command.Parameters.Add(new OleDbParameter("itemId", cut.ItemId));
        command.Parameters.Add(new OleDbParameter("orderId", cut.OrderId));
        command.Parameters.Add(new OleDbParameter("date", cut.Date.Date));
        command.Parameters.Add(new OleDbParameter("quantity", cut.Quantity));
        command.Parameters.Add(new OleDbParameter("operator", cut.Operator.Name));
        command.Parameters.Add(new OleDbParameter("heat", cut.Heat.HeatNumber));

        _Cuts.Add(cut);
        await Insert(command);
    }

    public async Task FillItem(OrderedItem item)
    {
        await LoadCuts();

        item.Cuts = new ObservableCollection<ProductionCut>(_Cuts.Where(x=>x.ItemId == item.Id));
    }


    private async Task<DataTable> GetTableAsync()
    {
        var command = new OleDbCommand("SELECT ProdLine, LineItemId, OrderID, CutDate, CutQty, Operator, BarHeat " +
                                       "from tblOrderItemProdCut");

        return await GetTable(command);
    } 

    private async Task TableToCuts(DataTable table)
    {
        _Cuts = new List<ProductionCut>();

        foreach (DataRow row in table.Rows)
        {
            var cut = await RowToCut(row);

            if (cut != null)
            {
                _Cuts.Add(cut);
            }
        }
    }

    private async Task <ProductionCut> RowToCut(DataRow row)
    {
        if (RowIsInvalid(row))
        {
            return null;
        }

        var employee = await _employee.GetByIdAsync(int.Parse((string)row[5]));
        var heat = await _barHeat.GetWithChargeNumberAsync((string) row[6]);
        var quantity = 0;
        var date = DateTime.Parse(row[3].ToString());

        if (row[4] is int)
        {
            quantity = (int)row[4];
        }

        return new ProductionCut((int) row[0], (int) row[1], (int) row[2],
            quantity, employee, heat, date);
    }

    private async Task LoadCuts()
    {
        if (_Cuts == null)
        {
            var table = await GetTableAsync();
            await TableToCuts(table);
        }
    } 

    private async Task<int> GetNextIndex()
    {
        await LoadCuts();

        if (_Cuts.Count > 0) 
        {
            return _Cuts.Max(x => x.Id) + 1;
        }
        else
        {
            return 0;
        }
    }

    protected override bool RowIsInvalid(DataRow row)
    {
        return row == null || row[0] is DBNull || row[1] is DBNull || 
               row[2] is DBNull || row[3] is DBNull || row[5] is DBNull || !(row[6] is string);
    }
}

Right now thats final, but I'm still opened to suggestions. Reason for static lists is that the program is simple tool that supervisor has to use for planning production and creating CutPlan for saw operator and upload results when shift ends. There's simply no need of updating any of those lists right now and for the future hopefully I'll have proper Sql database and switch to Entity Framework :].

\$\endgroup\$

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.