1

Hey guys i want to execute my SQL statement but im having synatx trouble, can someone help me understand what i doin wrong please?

Thanks, Ash.

public void AddToDatabase(string[] WordArray, int Good, int Bad, int Remove)
{

    for (int WordCount = 0; WordCount < WordArray.Length; WordCount++)
    {
        string sSQL = "INSERT INTO WordDef (Word, Good, Bad, Remove) VALUES (" + WordArray[WordCount] + ", " + Good + ", " + Bad + ", " + Remove + ")";

        Debug.Print(sSQL);

        //Private m_recordset As ADODB.Recordset
        //Private m_connection As ADODB.Connection
        ADODB.Recordset RS;
        ADODB.Connection CN ;


        CN = new ADODB.Connection();
        RS = new ADODB.Recordset();

        CN.CursorLocation = ADODB.CursorLocationEnum.adUseClient;

        CN.ConnectionString = "Provider=Microsoft.Jet.OLEDB.4.0;Data Source=doom_calc_dict.mdb;jet OLEDB:database";
        CN.Open(CN.ConnectionString,"","",0);

        object dummy = Type.Missing;

        CN.Execute(sSQL,out dummy,0);

        RS.Close(); 
        CN.Close(); 

        //string sSQL = "SELECT Word FROM WordDef WHERE Word='" + WordArray[WordCount] + "'";
        DatabaseTools.LoadDataFromDatabase(sSQL);
        //DatabaseTools.LoadDataFromDatabase(sSQL);

    }
}
2
  • What the result of your Debug.Print(sSql) ? Where your code is broking if you go in debug, in CN.Execute ? or when opening the sql connection ?
    – Melursus
    Apr 23, 2009 at 20:02
  • Just a note that I updated my answer to work with VS 2005/.Net2.0. It had relied on a VS2008 feature. Apr 23, 2009 at 20:54

5 Answers 5

18

The most important thing you need to fix is to use query parameters rather than building the string dynamically. This will improve performance, maintenance, and security.

Additionally, you want to use the newer strongly-typed ADO.Net objects. Make sure to add using directives for System.Data.OleDb.

Notice the using statements in this code. They will make sure your connection is closed when you finish with it. This is important because database connections are a limited and unmanaged resource.

Finally, you're not really using an array in your code. All you really care about is the ability to iterate over a collection of words, and so you want to accept an IEnumerable<string> instead of an array. Don't worry: this function will accept an array as an argument if that's what you need to pass it.

public void AddToDatabase(IEnumerable<string> Words, int Good, int Bad, int Remove)
{
    string sql = "INSERT INTO WordDef (Word, Good, Bad, Remove) VALUES (@Word, @Good, @Bad, @Remove)";

    using (OleDbConnection cn = new OleDbConnection("connection string here") )
    using (OleDbCommand cmd = new OleDbCommand(sql, cn))
    {
        cmd.Parameters.Add("@Word", OleDbType.VarChar);
        cmd.Parameters.Add("@Good", OleDbType.Integer).Value = Good;
        cmd.Parameters.Add("@Bad", OleDbType.Integer).Value = Bad;
        cmd.Parameters.Add("@Remove", OleDbType.Integer.Value = Remove;

        cn.Open();

        foreach (string word in Words)
        {
            cmd.Parameters[0].Value = word;
            cmd.ExecuteNonQuery();
        }
    }
}

One more thing: when using query parameters in OleDb it's important to make sure you add them in order.

Update: Fixed to work on VS 2005 / .Net 2.0 (had relied on VS 2008 features).

6
  • This is the best approach as prepared statements are always safer than dynamically generating the queries. I will dynamically generate them if there is absolutely no info coming from a user, that is the only time. Apr 23, 2009 at 20:16
  • This is a much better approach, but the Pascal-case method parameters had me looking around for the class-level properties you were referring to for a moment. Apr 23, 2009 at 20:26
  • With the exception of changing the type to IEnumerable, I used the OP's method signature. Apr 23, 2009 at 20:28
  • Awesome thankyou! Jus one question, i have included the using statement for the new library but the using(var inside the functon isnt reconised. Have i missed something?
    – Ash
    Apr 23, 2009 at 20:33
  • What version of visual studio/.Net? Apr 23, 2009 at 20:50
3

It will appear at first that I am not being helpful. But in truth, I am trying to help you, so please take it that way. You need to read this and this STAT! Once you've done that, here are some good, clean ADO.NET examples.

0

Try this (and you should try running the SQL from outside your application):

string sSQL = "INSERT INTO WordDef (Word, Good, Bad, Remove) VALUES ('" + WordArray[WordCount] + "', " + Good + ", " + Bad + ", " + Remove + ");";
2
  • 1
    Don't just concatenate your SQL statements together - that's a BAD BAD BAD practice and opens up your application to SQL injection attacks. Use parametrized queries instead! Simple, fast, secure.
    – marc_s
    Apr 23, 2009 at 20:18
  • 2
    I wouldn't write my app this way, and I did vote for the answer that was ultimately accepted. Doesn't change the technical correctness of the query, which is what the question was about.
    – Elie
    Apr 23, 2009 at 20:27
0

You need to add single quotes around the first argument in the SQL Statement.

string sSQL = "INSERT INTO WordDef (Word, Good, Bad, Remove) VALUES ('" + WordArray[WordCount] + "', " + Good + ", " + Bad + ", " + Remove + ")";

Character and Date fields require values to be surrounded with 'single quotes'

2
  • Don't just concatenate your SQL statements together - that's a BAD BAD BAD practice and opens up your application to SQL injection attacks. Use parametrized queries instead! Simple, fast, secure.
    – marc_s
    Apr 23, 2009 at 20:19
  • yeah but the question was not how do I prevent a sql injection attack. It was why am i getting this error.
    – Jeremy
    Apr 23, 2009 at 20:35
0

No you need it like this:

string query = "INSERT INTO Table_PersonInfo
(PersonID,Surname
,[Family Name]
,AddsOnName
,Street,Number
,PostalCode
,[City of Birth]
,[Year of Birth]
,[Phone Number])
 VALUES
 ('"+@personID+"'
, '"+ @surname + "' 
, '"+@familyname+"'
, '"+@nameExtension+"'
, '"+@street+"'
, '"+@houseNumber+"'
, '"+ @postalCode+"'
, '"+@yearofbirth+"'
, '"+@placeofbirth+"'
, '"+@phoneNumber+"')";

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.