5
\$\begingroup\$

The below sections of code are part of a list administration project I am working on to teach myself C#. This is from a tab that is used for list administration. There is a drop down box that allows selection of list administrators, which populates a dropdown box with available lists.

Once a list is selected, it populates two listboxes with users associated with the list administrators organization (such as a dance school), one has users that are in the list, the others are ones not in the list.

Before I start coding the moving of users between boxes and updating the tables, have I approached the methodology in the most efficient way? I know it works, but is there a method of setting it up I should look at to make things easier as I move forward?

Code for list selection and listbox population:

/* 
 * DATA MANIPULATION AND FORM ROUTINES FOR THE LISTS TAB
 * 
 */
private void btnEditList_Click(object sender, EventArgs e)
{
    string disp = cmbListAdmins.SelectedValue.ToString();

    string listQuery = "SELECT * FROM test.tbl_lists WHERE test.tbl_lists.user_id = " + disp + ";";

    dbconn = new MySqlConnection(connString);
    MySqlDataAdapter listadapter = new MySqlDataAdapter(listQuery, dbconn);
    DataTable dtAvailLists = new DataTable();
    try
    {
        listadapter.Fill(dtAvailLists);
        dtAvailLists.Columns.Add("FullName", typeof(string), "list_name + ' ' + list_description");
        BindingSource bindsource1 = new BindingSource();
        bindsource1.DataSource = dtAvailLists;
        cmbListSelect.DataSource = bindsource1;
        cmbListSelect.DisplayMember = "FullName";
        cmbListSelect.ValueMember = "list_id";
        cmbListSelect.Enabled = true;

    }
    catch (Exception err)
    {
        File.AppendAllText(logFileName,
            string.Format("{0}: Unable to fill list admin combo: Query is {1}, result is {2}.",
            DateTime.Now,
            listQuery,
            err,
            System.Environment.NewLine));
    }
    dbconn.Close();
}

private void cmbListSelect_SelectionChangeCommitted(object sender, EventArgs e)
{
    lstExistingMembers.Items.Clear();
    lstAvailableMembers.Items.Clear();
    string disp = cmbListSelect.SelectedValue.ToString();
    string memberQuery = "SELECT lm.list_id, lm.user_id, ua.user_first_name, ua.user_last_name, ua.user_id " +
                            "FROM test.tbl_listmembers AS lm " +
                            "LEFT JOIN test.tbl_user_accounts AS ua ON (ua.user_id = lm.user_id) " +
                            "WHERE (lm.list_id = '" + disp + "')";

    dbconn = new MySqlConnection(connString);
    MySqlDataAdapter listadapter = new MySqlDataAdapter(memberQuery, dbconn);
    DataTable dtListMembers = new DataTable();
    try
    {
        string fullname = "";
        string notClause = "";
        listadapter.Fill(dtListMembers);
        foreach (DataRow dr in dtListMembers.Rows)
        {
            fullname = dr[2].ToString() + " " + dr[3].ToString();
            lstExistingMembers.Items.Add(fullname);
            lstExistingMembers.ValueMember = dr[4].ToString();
            notClause += "'" + dr[4].ToString() + "',";
        }

        notClause = notClause.Substring(0, notClause.Length - 1);

        string nonMemberQuery = "SELECT concat(user_first_name, ' ', user_last_name) as username, user_id FROM test.tbl_user_accounts " +
                                "WHERE user_id NOT IN (" + notClause + ");";

        dbconntwo = new MySqlConnection(connString);
        MySqlDataAdapter listadaptertwo = new MySqlDataAdapter(nonMemberQuery, dbconntwo);
        DataTable dtListNonMembers = new DataTable();
        try
        {

            listadaptertwo.Fill(dtListNonMembers);
            foreach (DataRow dr2 in dtListNonMembers.Rows)
            {
                lstAvailableMembers.Items.Add(dr2[0].ToString());
                lstAvailableMembers.ValueMember = dr2[1].ToString();
            }
        }

        catch (Exception err)
        {
            File.AppendAllText(logFileName,
                string.Format("{0}: Unable to fill list non members listbox: Query is {1}, result is {2}.",
                DateTime.Now,
                nonMemberQuery,
                err,
                System.Environment.NewLine));
        }
        dbconntwo.Close();
    }
    catch (Exception err)
    {
        File.AppendAllText(logFileName,
            string.Format("{0}: Unable to fill list members listbox: Query is {1}, result is {2}.",
            DateTime.Now,
            memberQuery,
            err,
            System.Environment.NewLine));
    }
    dbconn.Close();
}

If needed, I can post the code from the database class that I have as well.

\$\endgroup\$
5
\$\begingroup\$

MySQL

I can't speak to C# but I have a few ideas to optimize your MySQL queries. There is very likely a way to pass parameters to SQL that is better than concatenating SQL within your code, I suggest you look into that.

As far as MySQL is concerned, you would obtain multiple benefits from using stored PROCEDURE instead of just passing code to the RDBMS:

  1. Better performance for repeat queries.

  2. Easier to maintain.

  3. Consistent result set.

  4. Can be called from multiple scripts in multiple languages easily.

Here is the code I would use to create the 3 PROCEDUREs in this case:

DELIMITER |
CREATE PROCEDURE sp_cmbListAdmins (IN p_cmbListAdmins)
AS
BEGIN
  SELECT * 
  FROM test.tbl_lists 
  WHERE test.tbl_lists.user_id = cmbListAdmins
  ;
END|
CREATE PROCEDURE sp_memberQuery (IN p_list_id)
AS
BEGIN
  SELECT 
    lm.list_id, 
    lm.user_id, 
    ua.user_first_name, 
    ua.user_last_name, 
    ua.user_id
  FROM test.tbl_listmembers AS lm
  LEFT JOIN test.tbl_user_accounts AS ua 
  ON ua.user_id = lm.user_id 
  WHERE lm.list_id = p_list_id
  ;
END|
CREATE PROCEDURE sp_nonMemberQuery (IN p_notClause)
AS
BEGIN
  SELECT concat(user_first_name, ' ', user_last_name) as username, 
  user_id 
  FROM test.tbl_user_accounts
  WHERE user_id NOT IN notClause
  ;
END|
DELIMITER ;

Note that the above code only needs executed once.

Then from C# you can just do something like...

    string disp = cmbListAdmins.SelectedValue.ToString();

    string listQuery = "CALL sp_cmbListAdmins(" + disp + ")";

And...

    string disp = cmbListSelect.SelectedValue.ToString();
    string memberQuery = "CALL sp_memberQuery(" + disp + "')";

And finally...

        notClause = notClause.Substring(0, notClause.Length - 1);

        string nonMemberQuery = "CALL sp_nonMemberQuery(" + notClause + ")";

Of course you would want to name things appropriately, I just picked names from the stuff you wrote in your C# script.

\$\endgroup\$
  • \$\begingroup\$ awesome. Thank you very much, I am just starting into t-sql. \$\endgroup\$ – JohnP Jul 10 '14 at 1:11
6
\$\begingroup\$

It seems to me that if File.AppendAllText throws an exception dbConn won't be closed. The dbConn.Close() should be in a finally block. (Exception inside catch block.)

\$\endgroup\$
  • \$\begingroup\$ well, THAT would never happen :p. How do you trap an error in a catch? Another try catch? \$\endgroup\$ – JohnP Jun 29 '12 at 22:55
  • \$\begingroup\$ It could, see the edit please :) \$\endgroup\$ – palacsint Jun 29 '12 at 22:59
  • \$\begingroup\$ +1 I agree close() should defo in a finally block. In fact I would put the dbConn = new MySQLConnection and it's 2 following lines in the try as well (to handle db connection errors) and check for null on dbConn in the finally block. \$\endgroup\$ – dreza Jun 29 '12 at 23:43
2
\$\begingroup\$

Current code do the work.

I recommend you have a further reading about "n-tiers" because you have dataaccess in your frontend, and this practice brings a high code coupling. Working in layers you could continue your C# developer skills with concepts like unit test (with tools like NUnit) and TDD.

\$\endgroup\$
1
\$\begingroup\$

Code duplication is not a nice idea, it's a massive indication of code smell. You can create a helper class for your common code, ex: getting db connections, creating data tables, etc...

 private static IDbConnection GetConnection()
 {
       var connectionString = ""; //read from config file
        IDbConnection connection = new MySqlConnection(connectionString);
        connection.Open();
        return connection;
  }

 public DataTable GetDataTable(string sql, params object[] parameterValues)
 {
        DataTable dataTable = new DataTable();
        using (var connection = GetConnection())
        {
            using (var command = connection.CreateCommand())
            {
                using (var da = new MySqlDataAdapter((MySqlCommand)command))
                {
                    command.CommandText = sql;
                    CreateCommandParameters(command, parameterValues);
                    da.Fill(dataTable);

                }
            }
        }
        return dataTable;
 }
 private void CreateCommandParameters(IDbCommand command, object[] parameterValues)
 {
        if (parameterValues != null)
        {
            int index = 1;
            foreach (object paramValue in parameterValues)
            {
                IDbDataParameter param = command.CreateParameter();
                param.ParameterName = "@P" + index;
                if (paramValue == null)
                    param.Value = DBNull.Value;
                else
                    param.Value = paramValue;
                command.Parameters.Add(param);
                index++;
            }
        }
  }

Note that here I am using Command Parameters instead of string concatenation, YOU SHOULD always use command parameters to avoid SQL injection attacks.

Moreover, the proper way for closing a connection is not calling Close() explicitly nor calling it in a finally, the proper way is using using statement. And as mentioned in previous answers your queries can be reworked to stored procedures that accepts parameters from your application, and there is a lot of benefits from doing that.

  • You gain performance ( you don't need to send the sql command to the database every time, you would perform an ExecuteScalar on procedure call instead).
  • Changing the query wouldn't require a code change which requires a build and a release
\$\endgroup\$
  • \$\begingroup\$ Thank you for the pointers. As I noted in the original post (2 years ago), I was teaching myself C#. I have since created a logging class and a database access class that I include when I need those functions. I am working on cleaning up the SQL queries and the rest. \$\endgroup\$ – JohnP Jul 10 '14 at 1:23
  • \$\begingroup\$ @JohnP brilliant, I've reviewed the code provided in the post \$\endgroup\$ – Sleiman Jneidi Jul 10 '14 at 1:27

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.