Query and data manipulation

Question

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.

Answer 1

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.

• JohnP: awesome. Thank you very much, I am just starting into t-sql.

Answer 2

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.)

• JohnP: well, THAT would never happen :p. How do you trap an error in a catch? Another try catch?
• palacsint: It could, see the edit please :)
• dreza: +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.

Answer 3

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.

Answer 4

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

• JohnP: 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.
• Sleiman Jneidi: @JohnP brilliant, I've reviewed the code provided in the post