Multiple forms for accessing records in database

Question

I have a main form for adding and deleting records and a button that opens a child form that displays records from an access database. Child form will have buttons for moving though the rows like first, last, next, previous and search. When user selects a record the child form will close and the selected record will be displayed in the main form, from which it can be changed, deleted, etc. This is the logic behind what I’m doing. So far I have written a class for accessing the data and is called from the child form to display the records. Before I move on, I want to get advice on what I have done so far and if it follows best practices. Any advice would be greatly appreciated.

Main Form:

namespace Test
{
public partial class frmTest : Form
{
    public frmTest()
    {
        InitializeComponent();
    }

    public class myClass
    {
        private static OleDbConnection myConn = new OleDbConnection();
        private static DataSet myDS;       
        private static DataRow myDR;
        public static int MaxRows = 0;
        public static int increment = 0;
        public static string name;

        public static void GetConnection()
        {             
            myClass.myConn.ConnectionString = @"Provider=Microsoft.ACE.OLEDB.12.0;Data Source=C:\Temp\Testing\TestDatabase.accdb";             
            try
            {
                myConn.Open();
                myDS = new DataSet();
                string SQL = "SELECT * From Test";
                OleDbDataAdapter myDA = new OleDbDataAdapter(SQL, myConn);
                myDA.Fill(myDS, "People");
                NavigateRecords();
                MaxRows = myDS.Tables["People"].Rows.Count;
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message);
            }
            finally
            {
                myConn.Close();
            }
        }

        public static void NavigateRecords()
        {
            myDR = myDS.Tables["People"].Rows[increment];
            name = myDR.ItemArray.GetValue(1).ToString();
        }          
    }

    private void frmNewForm_Click(object sender, EventArgs e)
    {
        frmNewForm fNew = new frmNewForm();
        fNew.ShowDialog();
    }      
}
}

Child form:

namespace Test
{
public partial class frmNewForm : Form
{
    public frmNewForm()
    {
        InitializeComponent();
    }

    private void frmNewForm_Load(object sender, EventArgs e)
    {
        MyAlias.GetConnection();
        txtName.Text = MyAlias.name;
    }

    private void FillTextBox()
    {
        txtName.Text = MyAlias.name;
    }

    private void btnNext_Click(object sender, EventArgs e)
    {
        if (MyAlias.increment != MyAlias.MaxRows - 1)
        {
            MyAlias.increment++;
            MyAlias.NavigateRecords();
            FillTextBox();
        }
        else
        {
            MessageBox.Show("No more rows");
        }
    }
}
}

Answer 1

Learn about Data Binding

Here is one tutorial. My main point is that with the Visual Studio designer linking back end data and the UI is pretty straight forward. And you can get navigation automagically; and along with "master / detail" synchronization it is so "thank goodness I didn't have to write this."

Separate UI from Data Fetching

I would pass in a MyClass object to the Form. This dependency injection is generally superior to "newing up" objects internally; making the design far more flexible and testable. I.E. we can inject mock (fake) stuff for the sake of testing.

One Form

Once the data fetching is factored out of frmTest then it becomes clear that a second Form is not needed. However, if you want that child form look/feel/behavior, then the data binding above makes it easy peasy.

Code Convention for Data Fetching

• Don't leave open / unused connections lying about
• Always wrap DB calls in try/catch. There's so much that can go wrong.

Here is a generally accepted good way of writing the code, taken right out of MSDN.

public void GetPeopleData() {
    // I put these outside of "using" because they are class variables.
     myClass.myConn.ConnectionString = @"Provider=Microsoft.ACE.OLEDB.12.0;Data Source=C:\Temp\Testing\TestDatabase.accdb"; 
 myDS = new DataSet();

    // everything created in "using" gets destroyed/goes out of scope at end
    // of "using" block. And in particular "myConn" gets disposed / destroyed 
    // at the end because it is the "using" parameter. We really want to make
    // sure all the resources - and in particular the OleDbConnection - go 
    // away. 
    using (OleDbConnection myConn= new OleDbConnection(connectionString))
        {
            string SQL = "SELECT * From Test";
            OleDbDataAdapter myDA = new OleDbDataAdapter(SQL, myConn);

            // Open the connection and execute the insert command.
            try
            {
                connection.Open();
                myDA.Fill(myDS, "People");
                NavigateRecords();
                MaxRows = myDS.Tables["People"].Rows.Count;
            } catch (Exception ex)
            {
                Console.WriteLine(ex.Message);
            }
      } //using

        // The connection is automatically closed when the
        // code exits the using block.
} //GetPeopleData()

Rename GetConnection()

This method gets the data. "Getting the connection" is merely a detail in that process.

EDIT

Changed the general example to be specific to the question code.

Renamed GetConnection() to GetPeopleData()

Refactor GetPeopleData()

GetPeopleData() should do that and only that. I do not understand NavigateRecords() in there. I should think navigating records is done in the child form ... or does NavigateRecoreds() even navigate the records?

Answer 2

Pull your MyClass out, and don't make it static. For the most part, you should avoid static variables and methods unless you really understand what you're doing.

MyClass is trying to do 2 different things - data access and keeping state of the current row. I'd suggest splitting those responsibilities out, and making MyClass only responsible for filling the dataset and returning it. Currently, your navigation is simple enough that your child form can track the state without too much complexity, so I would move that there. If it becomes more complex, you may want to introduce a new class to handle it.

Don't try to handle exceptions at the source unless you can recover from it automatically. Usually, you can't recover from it so it's best to let it go back to the form - the form can show a message box if it wants to.

Pay attention to naming - even at this early stage. Naming is not just for readability, but it'll help you keep the concepts and responsibilities clear in your head as you design. MyClass is meaningless, and doesn't tell you anything about what that class does - you may as well name it Class1.

You'll want to figure out how to get data from one form to another. Currently, it looks like you plan to use static variables for that - that's not a good idea. You should either use events, pass forms along or create a custom class to pass along to hold state. Passing the form along is simple and probably the easiest option to start with.

At the end, you'll have something like:

 class ParentForm {
     DataRow SelectedRow { get; Set; }

     void GetSelectedRow() { 
        Var f = new ChildForm();
        F.ShowDialog();
        this.SelectedRow = f.SelectedRow;
     }
 }

 class DataAccess {
     Dataset GetAllPeople();
 }

 class ChildForm {
     int selectedRecordIndex;
     int maxRows;
     DataRow SelectedRow { get { return AllPeople.Rows[selectedRecordIndex]; } }
     Dataset allPeople;

     ChildForm_Load() {
        Var da = new DataAccess();
        try {
            this.allPeople = da.GetAllPeople();
        } catch (Exception ex) {
            MessageBox.Show(ex.Message);
            // nothing for this form to do anymore
            this. Close();
        }
        this.maxRows = this.allPeople.Rows.Count;
        this.selectedRecordIndex = 0;
        this.FillTextBox();
     }
 }