views:

1639

answers:

6

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

    }
}
A: 

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 + ");";
Elie
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
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
A: 

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'

Jeremy
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
yeah but the question was not how do I prevent a sql injection attack. It was why am i getting this error.
Jeremy
A: 

What error are you getting when you try to run this?

Crippledsmurf
+3  A: 

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.

Good luck! :)

JP Alioto
+17  A: 

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

Joel Coehoorn
+1 for common sense (parametrized query)
Krzysztof Koźmic
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.
James Black
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.
Joel Mueller
With the exception of changing the type to IEnumerable, I used the OP's method signature.
Joel Coehoorn
+1 (also for common sense)
Mike C.
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
What version of visual studio/.Net?
Joel Coehoorn
+1 for performance!
Jeff Olson
A: 

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+"')";

jeremy