Getting data from database and then outputing it to the DataGridView

Question

I am at the point where I feel that I am not doing it right. It works and does the job, but I am sure that there are more efficient and smarter ways of doing it.

I would like to see if there is a way of making it more efficient, clear, and easier to use.

This is my query getter:

    public DataTable querySQL_DT_Return(String query)
    {
        var form = Form.ActiveForm as Form1;
        DataTable RETURNME = null;
        OleDbConnection conn = new OleDbConnection(cnStr);

        try
        {
            try
            {
                conn.Open();
            }
            catch
            {
                MessageBox.Show("Can't connect to the database!");
            }

            OleDbDataAdapter Data1 = null;

            try
            {
                Data1 = new OleDbDataAdapter(query, conn);
            }
            catch
            {
                MessageBox.Show("Can't open the connection to the DB, please try again!");
                return RETURNME;
            }

            DataSet a = new DataSet();
            Data1.Fill(a);

            DataTable dt = a.Tables[0];
            //Adding data to the columns
            RETURNME = dt;
        }

        catch
        {
            //MessageBox.Show("Having issues retrieving data from Database, please try again!");
            // return RETURNME;
        }
        finally
        {
            if (conn.State.ToString() == "Open")
            {
                conn.Close();
            }
        }

        if (conn.State.ToString() == "Open")
        {

            conn.Close();
        }
        return RETURNME;
    }

When I receive the DataTable, I just loop through it getting records.

Answer 1

Even if you are stuck with the old/wrong technology (all the cool kids use ORMs these days), you can still dance around that and simplify the act of interfacing with it with clean code of your own. I like having a direct link to the database because I can do whatever the hell I want with it. If the author of an ORM library and I had different goals and priorities, then this library will actually hinder my productivity. Disclaimer: I have little experience with ORMs.

Anyhow, try using lots of smaller functions. Try passing everything a method needs in - that way you can move this code into a separate class. Use exception handling to signal that something went wrong. Do not use the MessageBox / return null combination. You might be running this code as a windows service where showing Message Boxes is not allowed. It also does not feel right. The error handling / display effort needs to be well thought out, not copy /pasted into every method. Features are assets; code is liability, hence less code is more. Try using descriptive variable/method/etc. names.

public DataTable RunQueryExpectOneTable(
    string queryText,
    string[] expectedColumnNames = null)
{
    return RunQueryExpectOneTable(
    connectionString: this.ConnectionString,
    queryText: queryText,
    expectedColumnNames: expectedColumnNames);
}

public static DataTable RunQueryExpectOneTable(
    string connectionString,
    string queryText,
    string[] expectedColumnNames = null)
{
    // Even the body of this using method is too long, it can be further split up into methods.
    using(var connection = new OleDbConnection(connectionString))
    {
        // Opens it and makes sure that it is open.
        this.OpenAndCheckConnection();
        var adapter = new OleDbDataAdapter(query, conn);
        DataSet ds = new DataSet();
        adapter.Fill(ds);

        // if more than one table, then throw an exception.
        if (expectedColumnNames != null)
        {
            // Also check the number and the names of the columns
        }

        return ds.Tables[0];
    }
}

Answer 2

You might try using entity framework and direct data binding. Unless you are doing something fancy, you shouldn't have to manually loop through records to create UI elements.