CODEWITHSUNDEEP
c#winformsms-access
1337Atreyu
1337Atreyu

Reputation: 223

Trouble writing to database in C# WinForm Application

I'm relatively inexperienced with professional programming, but I'm trying to write a program that interfaces with a MS Access Database. Essentially I'm gathering information in the form and trying to pass information at a new line for each entry. I have an open OleDbConnection and my test is showing that I am able to see what row will have the new entry, but when I hit the submit button, there is no error shown in the catch, but the database remains unchanged. I originally had the code in a method that I called from the click event, but I just brought the code over to the event handler to verify the issue wasn't with the call.

private void btnSubmit_Click(object sender, EventArgs e)
    {

        if (DBConnection.State.Equals(ConnectionState.Closed))
        {
            DBConnection.Open();
        }

        try
        {
            MessageBox.Show("Save Data at index: " + intRowPosition.ToString());

            OleDbCommand OledbInsert = new OleDbCommand("Insert INTO RetentionTable (DateTime,Center,CSP,MemberID,ContractNumber,RetentionType,RetentionTrigger,MemberReason,ActionTaken,Other) VALUES('" + DateTime.Now.ToString() + "','" + GetCenter("") + "','" + GetName("") + "','" + GetMemberID("") + "','" + GetContractNumber("") + "','" + GetType("") + "','" + GetTrigger("") + "','" + GetReason("") + "','" + GetAction("") + "', + GetOther("")," DBConnection);

            intRowPosition++;
        }

        catch (Exception ex)
        {
            MessageBox.Show(ex.Message.ToString());
            MessageBox.Show(ex.StackTrace.ToString());
        }
        finally
        {
            RefreshDBConnection();
        }

    }

Any ideas as to why this is not writing would be much appreciated.

Upvotes: 3

Views: 168

Answers (2)

Steve
Steve

Reputation: 216313

There are many problems in your code above.

  • First, a command should be executed, not simply declared. (this is why the database is not being modified)
  • Second, you use reserved keywords in your statement (so even if you execute your statement, it will fail and throw an exception)
  • Third, you are concatenating strings to build the command text. A very bad move that would leave your application susceptible to SQL injection attacks
  • Fourth, the connection should be closed after usage

Let me try to write a replacement code

string cmdText = "Insert INTO RetentionTable " +
                "([DateTime],Center,CSP,MemberID,ContractNumber,RetentionType," + 
                "RetentionTrigger,MemberReason,ActionTaken,Other) " + 
                "VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
 using(OleDbConnection cn = new OleDbConnection(conString))
 using(OleDbCommand cmd = new OleDbCommand(cmdText, cn))
 {
    cmd.Parameters.AddWithValue("@p1", DateTime.Now.ToString());
    cmd.Parameters.AddWithValue("@p2", GetCenter("")); 
    cmd.Parameters.AddWithValue("@p3", GetName(""));
    cmd.Parameters.AddWithValue("@p4", GetMemberID(""));
    cmd.Parameters.AddWithValue("@p5", GetContractNumber(""));
    cmd.Parameters.AddWithValue("@p6", GetType("")); 
    cmd.Parameters.AddWithValue("@p7", GetTrigger(""));
    cmd.Parameters.AddWithValue("@p8", GetReason(""));
    cmd.Parameters.AddWithValue("@p9", GetAction(""));
    cmd.Parameters.AddWithValue("@p10", GetOther(""));
    cmd.ExecuteNonQuery();
 }

The DATETIME is a reserved keyword in Access and thus, if you want to use it for your column names then you need to enclose it in square brackets.

The string concatenation is a bad practice in MSAccess but it is a fatal flaw in other database where your code could be used for Sql Injections (more difficult in Access but not impossible). If you use a parameterized query as in my example, you remove the Sql Injection problem, but also you let the framework code to pass the correct value to the database engine with the correct formatting required for dates, strings and decimals.

Another point to consider is to not have a global OleDbConnection object, but create, use and destroy the object whenever your need it. The Connection Pooling will avoid performance problems and your code will not suffer from memory leaks when a connection fails for whatever reason

I want also add that your GetXXXXX methods seems all to return strings. Remember that these methods should return values compatible with the underlying database field where you want to write

Upvotes: 7

cja
cja

Reputation: 10026

It might be the speechmarks around the values you're putting into the database. Try changing to apostrophes.

Anyway, I strongly recommend storing the final SQL in a string and printing it to a log file or the screen and then copying that to your Access SQL editor and trying to run it. Then you'll see if there's an error and what it is.

Upvotes: 1

Related Questions