Proper way of getting a data from an Access Database

Question

I'm a bit confused of how to get a data from an access database. Is it proper to gather it first in a List then get those data from your List OR it is okay to just directly get it in you database ?

My codes work perfectly fine, but I wanna know if there is a better way to do this?? :

 private void button3_Click(object sender, EventArgs e)
    {
        OleDbConnection connection = new OleDbConnection(@"Provider=Microsoft.ACE.OLEDB.12.0;Data Source=C:\Users\redgabanan\Desktop\Gabanan_Red_dbaseCon\Red_Database.accdb");
        connection.Open();
        OleDbDataReader reader = null;
        OleDbCommand command = new OleDbCommand("SELECT * from  Users WHERE LastName='"+textBox8.Text+"'", connection);
        reader = command.ExecuteReader();
        listBox1.Items.Clear();

        while (reader.Read())
        {

            listBox1.Items.Add(reader[1].ToString()+","+reader[2].ToString());
        }

        connection.Close();

*I'm getting my records directly from a database then display it in a listbox.

Brace yourself for unhandled exception. – kiran Feb 27 '14 at 07:14 — kiran, Feb 27 '14 at 07:14

Jeremy Thompson · Accepted Answer · 2021-08-24T09:08:40.277

One thing that is sticking out like a sore thumb is the SQLInjection and to use Parameterised queries, eg:

OleDbCommand command = new OleDbCommand("SELECT * from  Users WHERE LastName='@1'", connection);
        
command.Parameters.AddWithValue("@1", textBox8.Text)

What your doing is perfectly acceptable, although you would generally be better off to use a SQL Database.

Edit: Here is how you seperate your business logic from the GUI:

Class BusLogic
{
 public List<string> ListboxItems = new List<string>();
 public void PopulateListBoxItems(string userName)
 {
  string connString = @"Provider=Microsoft.ACE.OLEDB.12.0;Data Source=C:\Users\redgabanan\Desktop\Gabanan_Red_dbaseCon\Red_Database.accdb";
  using (OleDbConnection connection = new OleDbConnection(connString))
  {
        connection.Open();
        OleDbDataReader reader = null;
        OleDbCommand command = new OleDbCommand("SELECT * from  Users WHERE LastName='@1'", connection);            
        command.Parameters.AddWithValue("@1", userName)
        reader = command.ExecuteReader();    
        while (reader.Read())
        {
            ListboxItems.Add(reader[1].ToString()+","+reader[2].ToString());
        }    
   }
 }    
}

GUI

private void button3_Click(object sender, EventArgs e)
{        
      var busLogic = new BusLogic();
      busLogic.PopulateListBoxItems(textBox8.Text);          
      \\listBox1.Items.Clear();
      ListboxItems.DataSource = busLogic.ListboxItems;
}

The beauty of this "MVC" approach is that we only really need to test the BusLogic if we rely on controls being bound using Binding.

ps Ideally ListboxItems would be an IEnumerable instead of List so that we don't expose any functionality to Add/Remove etc from the caller. This is good API design.

right, that is very vulnerable to SQL injection, better use parameters AddwithValue :) — Pyromancer, Mar 01 '13 at 01:05
Thanks. That's all i wanna know that my codes are fine. I used Parametised queries now. — Red Gabanan, Mar 01 '13 at 01:13
Adding some exception handling mechanism will be good.As there is no guarantee that there will be a access database with hard coded table name with read access permission. — kiran, Feb 27 '14 at 07:12

score 4 · Answer 2 · answered Mar 01 '13 at 01:05

I would say the answer is "yes" to both.

What you're doing now is perfectly acceptable for simple cases. Just be aware that it doesn't "scale" very well. That is, loading 10 or 20 items is fine. But what happens if it becomes 10 thousand or a million?

In that case you want to look at using a Model-View-Controller (MVC) architecture. That's a topic in itself, but basically you decouple the listbox (the "view") from the data (the "model").

See this site for a C#-centric MVC discussion

In between what you're doing now and a full-blown MVC architecture, you may simply want to do as you suggest - load the list first then add them to the list box. That gains you nothing if you just load it once, but if the list is loaded "all over the place", you can save the database IO overhead each time by just accessing it once.

The fact that you thought to ask the question indicates you're on the right track.

score 2 · Answer 3 · answered Mar 01 '13 at 01:10

2

Although your code works without any problem, I suggest you to perform some exception handling as in this example, since both OleDbConnection.Open() and OleDbCommand.ExecuteReader() might throw an InvalidOperationException.

It is also common to wrap the connection with a using statement, so in the end connection.close() is called automatically, but this is just a personal preference.

answered Mar 01 '13 at 01:10

A. Rodas

20,171
8
62
72

+1 Thanks for this. I've included code demonstrating this in my answer – Jeremy Thompson Mar 01 '13 at 01:13

score 1 · Answer 4 · answered Mar 01 '13 at 00:59

1

You can maybe separate your data access functions in different classes or create generic functions to retrieve records.

answered Mar 01 '13 at 00:59

whastupduck

1,156
11
25

+1 Thanks for this. I've included code demonstrating this in my answer – Jeremy Thompson Mar 01 '13 at 01:14

Proper way of getting a data from an Access Database

4 Answers4

Linked

Related