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.

Phrancis · Accepted Answer · 2014-07-10 01:13:05Z

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:

Better performance for repeat queries.
Easier to maintain.
Consistent result set.
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.

awesome. Thank you very much, I am just starting into t-sql. — Kevin Flynn
– Kevin Flynn, Commented Jul 10, 2014 at 1:11

Community · Accepted Answer · 2017-05-23 12:40:50Z

6

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

edited May 23, 2017 at 12:40

CommunityBot

1

answered Jun 29, 2012 at 22:52

palacsint

30.4k9 gold badges82 silver badges157 bronze badges

\$\begingroup\$ well, THAT would never happen :p. How do you trap an error in a catch? Another try catch? \$\endgroup\$

Kevin Flynn
– Kevin Flynn

2012-06-29 22:55:19 +00:00
Commented Jun 29, 2012 at 22:55
\$\begingroup\$ It could, see the edit please :) \$\endgroup\$

palacsint
– palacsint

2012-06-29 22:59:49 +00:00
Commented Jun 29, 2012 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
– dreza

2012-06-29 23:43:59 +00:00
Commented Jun 29, 2012 at 23:43

Add a comment |

MarioG · Accepted Answer · 2012-07-04 04:51:14Z

2

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.

answered Jul 4, 2012 at 4:51

MarioG

212 bronze badges

Add a comment |

Sleiman Jneidi · Accepted Answer · 2014-07-10 01:13:37Z

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

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. — Kevin Flynn
– Kevin Flynn, Commented Jul 10, 2014 at 1:23
@JohnP brilliant, I've reviewed the code provided in the post — Sleiman Jneidi
– Sleiman Jneidi, Commented Jul 10, 2014 at 1:27

Stack Exchange Network

Query and data manipulation

4 Answers 4

You must log in to answer this question.

Hot Network Questions

Query and data manipulation

4 Answers 4

You must log in to answer this question.

Related

Hot Network Questions