1
\$\begingroup\$

Goal:

I am trying to get the hang of using MySQL connections within C# windows forms. These are such areas I am targeting:

  • Loading dataGridView correctly
  • Refresh dataGridView from an external form (See public void RefreshGrid())
  • Update query for MySQL connection

I'd like to hear some reviews from you guys. Does the structure look good, can something be changed to, because of some irrelevant code?

This is what I have so far:

using System;
using System.Configuration;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using MySql.Data.MySqlClient;

namespace Final_Version
{
    public partial class Form3 : Form
    {
        public Form3()
        {
            InitializeComponent();
        }

        //MySQL connection variable
        private MySqlConnection connection;

        //open connection to database
        private bool OpenConnection()
        {
            try
            {
                connection.Open();
                return true;
            }
            catch (MySqlException ex)
            {
                //When handling errors, you can your application's response based on the error number.
                switch (ex.Number)
                {
                    case 0:
                        MessageBox.Show("Cannot connect to server. Contact administrator");
                        break;
                    default:
                        MessageBox.Show(ex.Message);
                        break;
                }
                return false;
            }
        }

        //Close connection
        private bool CloseConnection()
        {
            try
            {
                connection.Close();
                return true;
            }
            catch (MySqlException ex)
            {
                MessageBox.Show(ex.Message);
                return false;
            }
        }

        //Adapter
        private MySqlDataAdapter mySqlDataAdapter;

        private void Form3_Load(object sender, EventArgs e)
        {
            // MySQL connection string
            string connString = ConfigurationManager.ConnectionStrings["Final_Version.Properties.Settings.technicalConnectionString"].ConnectionString;
            connection = new MySqlConnection(connString);

            if (this.OpenConnection() == true)
            {
                //Logic
                mySqlDataAdapter = new MySqlDataAdapter("select * from user", connection);
                DataSet DS = new DataSet();
                mySqlDataAdapter.Fill(DS);
                dataGridView1.DataSource = DS.Tables[0];

                //close connection
                this.CloseConnection();
            }
        }

                //Refresh Data Grid from external Forms
        public void RefreshGrid()
        {
            // MySQL connection string
            string connString = ConfigurationManager.ConnectionStrings["Final_Version.Properties.Settings.technicalConnectionString"].ConnectionString;
            connection = new MySqlConnection(connString);

            if (this.OpenConnection() == true)
            {
                //Logic
                mySqlDataAdapter = new MySqlDataAdapter("select * from user", connection);
                DataSet DS = new DataSet();
                mySqlDataAdapter.Fill(DS);
                dataGridView1.DataSource = DS.Tables[0];

                //close connection
                this.CloseConnection();
            }
        }


        //Open new form *Form 4 - Add user*
        private void button1_Click(object sender, EventArgs e)
        {
            var form4 = new Form4(this);
            form4.Show();
        }


        //Remove user
        private void button2_Click(object sender, EventArgs e)
        {
            var confirmResult = MessageBox.Show("Remove this user?",
                                    "Confirm",
                                    MessageBoxButtons.YesNo);
            if (confirmResult == DialogResult.Yes)
            {
                if (this.dataGridView1.SelectedRows.Count > 0)
                {
                    string user = dataGridView1.SelectedCells[0].Value.ToString();
                    string connString = ConfigurationManager.ConnectionStrings["Final_Version.Properties.Settings.technicalConnectionString"].ConnectionString;
                    MySqlConnection conn = new MySqlConnection(connString);
                    conn.Open();
                    MySqlCommand comm = conn.CreateCommand();
                    comm.CommandText = "delete from user where username = @user";
                    comm.Parameters.AddWithValue("@user", user);
                    comm.ExecuteNonQuery();
                    conn.Close();
                    MessageBox.Show("User (" + user + ") removed");
                    this.RefreshGrid();
                }
                else
                {
                    MessageBox.Show("Select Row");
                }
            }
            else
            {
                return;
            }
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! Can you tell us more about your table structure? For example, the result of EXPLAIN? \$\endgroup\$
    – Mast
    Feb 13 '20 at 12:01
1
\$\begingroup\$

Don't have methods like OpenConnection() and CloseConnection(). Instead, use a using block:

using(var conn = new MySqlConnection(connString))
{
   // db stuff
}

Same for MySqlDataAdapter and DataSet: both implement IDisposable and thus should be handled properly. Instead of writing lots of code to do this yourself, use the built-in using.

using(var conn = new MySqlConnection(connString))
{
   using(var mySqlDataAdapter= new MySqlDataAdapter("select * from user", connection))
   {
      using(var dataSet = new DataSet())
      {
          // custom code
      }
   }
}

Do not store your MySqlConnection at class level!


Why do you have almost identical Form3_Load and RefreshGrid methods? Do not copy-paste code, instead move it to a method and call that.


Store ConfigurationManager.ConnectionStrings["Final_Version.Properties.Settings.technicalConnectionString"].ConnectionString in a dedicated class. If you ever need to change this parameter name, you need to change it in three places in this short class alone and that is just asking for problems.

I usually have a class called AppConfiguration which then contains things like:

    public static string ConnectionString()
    {
        return ConfigurationManager.ConnectionStrings["connection_string_name"].ConnectionString;
    }

You're using WinForms which I'd consider "ancient" technology, and thus you'll likely encounter old and outdated example code. I'd urge you to look at more modern practices and consider better ways to get the same result.

Case in point is the binding of a DataSet to a Grid: this is fairly easy, but once you need to add custom properties etc. you'll likely end up with hacky code. Instead consider moving all of your DB logic to a service class and using Dapper to return a list of data objects with properly named properties. that way you also separate your UI from your business logic.

Also, you seem very concerned about exceptions thrown by opening or closing your db connection. IMHO it is pointless to anticipate such exceptions and capture them unless you are connecting to a very volatile database environment -- and then you should solve that problem instead of anticipating it.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for this, will improve and look into my code \$\endgroup\$
    – EdGzi
    Feb 13 '20 at 13:38
  • \$\begingroup\$ Hi again, I see you said you consider WinForms as ancient. What do you recommend using now? To create Desktop applications to communicate with MySQL databases? \$\endgroup\$
    – EdGzi
    Feb 14 '20 at 11:00
  • \$\begingroup\$ @LV98 Depends on your needs: docs.microsoft.com/en-us/windows/apps/desktop . The main thing is to separate the UI from your business logic, and WinForms has a tendency to make it easy not to do so. At least apply MVVM(-like) practices, because those are things you'll see in all kinds of technologies used in modern projects. \$\endgroup\$
    – BCdotWEB
    Feb 14 '20 at 14:52

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.