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.

Answer 1

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.

Answer 2

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.

Answer 3

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.