2
\$\begingroup\$

I am just a beginner trying to do my due diligence and research things properly. I am trying to go about the correct way of adding data into a database. I have read articles on the following:

  • Not showing ConnectionString in plaintext. I currently store it in App.Config and I am still trying to figure out how to encrypt it.
  • using keyword to deal with disposable items.
  • catch try finally to deal with exceptions.
  • parameterized SQL statements to deal with SQL Injection.

I am looking for any advise to make my code better. I have revised it several times taking into account all the articles I have read about the topics above.

private void Btn_InsertData_Click(object sender, EventArgs e)
{
    if (TextboxesAreNullorEmpty() == true)
    {
        MessageBox.Show("Please fill out all fields!");
        return;
    }
    else
    {
        try
        {
            using (MySqlConnection conn = new MySqlConnection(Properties.Settings.Default.MySQLDBConnection))
            using (MySqlCommand cmd = new MySqlCommand())
            {
                conn.Open();
                cmd.Connection = conn;
                cmd.CommandText = "INSERT INTO `sites` (`ContractNumber`, `SiteName`, `SitePhoneNumber`, `SiteAddLine1`, `SiteAddLine2`, `SiteAddCity`, `SiteAddCounty`, `SiteAddPostcode`, `SiteAddCountry`) VALUES (@contract_name, @site_name, @site_phone_number, @site_add_line1, @site_add_line2, @site_add_city, @site_add_county, @site_add_postcode, @site_add_country)";
                cmd.Prepare();
                cmd.Parameters.AddWithValue("@contract_name", this.txtBox_ContractNumber.Text);
                cmd.Parameters.AddWithValue("@site_name", this.txtBox_SiteName.Text);
                cmd.Parameters.AddWithValue("@site_phone_number", this.txtBox_SitePhoneNumber.Text);
                cmd.Parameters.AddWithValue("@site_add_line1", this.txtBox_SiteAddLine1.Text);
                cmd.Parameters.AddWithValue("@site_add_line2", this.txtBox_SiteAddLine2.Text);
                cmd.Parameters.AddWithValue("@site_add_city", this.txtBox_SiteAddCity.Text);
                cmd.Parameters.AddWithValue("@site_add_county", this.txtBox_SiteAddCounty.Text);
                cmd.Parameters.AddWithValue("@site_add_postcode", this.txtBox_SiteAddPostcode.Text);
                cmd.Parameters.AddWithValue("@site_add_country", this.txtBox_SiteAddCountry.Text);
                cmd.ExecuteNonQuery();
                MessageBox.Show("Site Details Added");
            }
        }
        catch (Exception ex)
        {
            MessageBox.Show(string.Format("An error occurred {0}", ex.Message), "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
        }
        finally
        {
           ClearAllTextBoxes();
        }
    }

}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Looking at private void Btn_InsertData_Click ... where is this code actually going to be running? \$\endgroup\$ Feb 19 '18 at 1:51
  • 2
    \$\begingroup\$ It seems rather harsh to clear the text boxes if the INSERT failed, doesn't it? \$\endgroup\$ Feb 19 '18 at 6:40
  • \$\begingroup\$ @Michael-sqlbot It is a test app to muck about with but it is running directly of the UI form which I now know is not a great way to it. I am going to make changes as suggested by Flater below. \$\endgroup\$ Feb 19 '18 at 17:10
  • \$\begingroup\$ @200_success You are correct. I logically never thought it through correctly it would seem. Thanks for pointing it out. \$\endgroup\$ Feb 19 '18 at 17:11
4
\$\begingroup\$

Not showing ConnectionString in plaintext. I currently store it in App.Config and I am still trying to figure out how to encrypt it.

Anything you can decrypt locally, a malevolent (and sufficiently skilled) user can too. More information given in the answer here.

There are other ways to avoid this:

  • Add a webservice backend layer. The frontend only connects to the backend (no connectionstring needed for that), and the backend has the connection string (which is out of reach for the frontend and its users).
  • Use Integrated Security, i.e. using the current Windows account for logging in to the database; as opposed to providing a username and password in the connection string. But you'd need to provide the needed rights to all of your users' Windows account, and then everyone is working under their own account (some companies do this, most don't).

catch try finally to deal with exceptions.

Your current implementation seems okay, but I do wonder about the finally.

If there is no error, the textboxes get cleared. That seems okay. But if there is an error, the textboxes still get cleared? That doesn't seem user friendly. I'd expect you'd want to specifically keep the content of the textboxes so the user can retry saving the data.

try
{
    //save the data

    ClearAllTextBoxes();
}
catch (Exception ex)
{
    MessageBox.Show(string.Format("An error occurred {0}", ex.Message), "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
}

If an exception occurs when saving the data; the exception will skip over the ClearAllTextBoxes(); and go straight to the catch. It will never empty the textboxes.
However, if no exception is encountered, then the code will continue with ClearAllTextBoxes(); and therefore clear the textboxes.

Minor comment
You may want to put a return; in your catch. Not necessary at the moment, but it will become relevant if there is more code after your try catch.


In general, it's not good to put data handling (the SQL query) inside the UI. You'd want to put at least one layer of separation between the two.

However, you did mention you're a beginner; so I'm not going to hammer this point right now. If this project is a simple test application; you don't have to implement that layer of separation.

But I would advise trying to separate the two in future projects. You don't want a bad habit to form.


This is nitpicking, but

if (TextboxesAreNullorEmpty() == true)

really grinds my gears. == true is redundant, you can simply use

if (TextboxesAreNullorEmpty())

In order to reduce nesting, you can change your code. As it currently stands, you have:

if (TextboxesAreNullorEmpty())
{
     MessageBox.Show("Please fill out all fields!");
     return;
}
else
{
      //Do the work
}

You can omit the else.

if (TextboxesAreNullorEmpty())
{
     MessageBox.Show("Please fill out all fields!");
     return;
}

//Do the work

This works exactly the same way. The only difference is that you have less indentation in your code. While one extra level of indentation may not be an issue; a sufficiently complex algorithm can have several ìnstances of redundant indentation, at which point it does become a relevant consideration.

But since you are a beginner, this isn't really a big issue right now. Just keep it in mind for the future, to avoid a bad habit.


Although you're not yet at the point of abusing it; you should start looking into breaking up the code you have and putting it into multiple smaller methods.

Currently, you have a single method that does many different things:

  • Handle the button click event
  • Validate the form (this is already correctly abstracted into a separate method)
  • Set up the SQL connection and command
  • Update the data in the database
  • Handle errors

I would suggest you add two extra methods here:

  • Instead of putting the logic inside the click event handler, have the click event handler call a StartSave() method which does the work. The benefit of this becomes clear once you can have multiple triggers to save the data (since the act of saving data isn't uniquely connected to a single user control anymore).
  • Put the entire try catch into a separate method, e.g. UpdateDatabase(). Ideally, you'd want to use method parameters here for all the input variables; which will give you a layer of separation between your form controls and the SQL parameters.
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for being constructive. I find it extremely difficult to find constructive criticism without people belittling you at the same time. Many good points you've raised. I will endeavour to incorporate and revise. \$\endgroup\$ Feb 19 '18 at 17:09
  • \$\begingroup\$ @RyanWalkowski: As a beginner, you've implemented most things correctly and relatively neatly, so you're on the right track :) The things I brought up, like the separate layers; are things you will encounter in medium to large enterprise applications; and you generally only learn to do so after having worked on a large project that failed to do so properly ;) \$\endgroup\$
    – Flater
    Feb 20 '18 at 8:37

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.