Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm in the process of learning SQLite in C# and I've come up with some generic methods for finding and updating data. I would appreciate any pointers with the design of my methods and/or my SQL syntax.

FYI: I'm using System.Data and Mono.Data.SqliteClientfor C# Mono 2.0 (Unity3D).

Here is an example of the database used for this post. Let's also assume that I check for null prior to obtaining/updating data.

Info - Table name

    | display_name | coins | total_score |
    |    Michael   |  100  |    1356     |
    |     John     |   50  |     904     |

Below is my FindData method, which is used to lookup data in a specified table.

    public object FindData (string tableName, string selectHeader, string whereHeader, object whereValue)
    {
        using (var dbConnection = new SqliteConnection ("URI=file:" + Application.dataPath + connection))
        {
            dbConnection.Open ();

            using (var dbCommand = dbConnection.CreateCommand ())
            {
                dbCommand.CommandText = string.Format ("SELECT {0} " +
                                                       "FROM {1} " +
                                                       "WHERE {2} = @whereValue", selectHeader, tableName, whereHeader);

                SqliteParameter whereParam = new SqliteParameter ("@whereValue", whereValue);
                dbCommand.Parameters.Add (whereParam);

                using (var dbReader = dbCommand.ExecuteReader ())
                {
                    while (dbReader.Read ())
                    {
                        if (dbReader.GetValue (0) != null)
                            return dbReader.GetValue (0);
                    }
                }
            }
        }
        return null;
    }

I attempted to have SQL Injection prevention parameters for tableName, selectHeader and whereHeader, however, it kept throwing an error saying the syntax was wrong. I'm assuming this can't be done then.

That method can be used like so:

int coins = (int)FindData ("Info", "coins", "display_name", "Michael");

This attempts to obtain the value under column coins, from the table Info where the display_name column is equal to Michael

Next I have the UpdateData method. This is used to update/save the data in the specified table.

public void UpdateData (string tableName, string whereHeader, object whereValue, string setHeader, object setValue)
{
    using (var dbConnection = new SqliteConnection ("URI=file:" + Application.dataPath + connection))
    {
        dbConnection.Open ();

        using (var dbCommand = dbConnection.CreateCommand ())
        {

            dbCommand.CommandText = string.Format ("UPDATE {0} " +
                                                   "SET {1} = @setValue " +
                                                   "WHERE {2} = @whereValue",
                tableName, setHeader, whereHeader);

            SqliteParameter setParam = new SqliteParameter ("@setValue", setValue);
            SqliteParameter whereParam = new SqliteParameter ("@whereValue", whereValue);

            dbCommand.Parameters.Add (setParam);
            dbCommand.Parameters.Add (whereParam);

            Debug.Log (dbCommand.CommandText);

            dbCommand.ExecuteNonQuery ();
        }
    }
}

This method can be used like so:

UpdateData ("Info", "display_name", character.displayName, "coins", character.coins);

This method attempts to update the Info table, based on the column displayName, which equals character.displayName, which then updates the coins column based on the character.coins variable

Can anyone provide any criticism/advice on these generic methods for saving and loading data from a Sqlite database.

share|improve this question
up vote 6 down vote accepted
        SqliteParameter setParam = new SqliteParameter ("@setValue", setValue);
        SqliteParameter whereParam = new SqliteParameter ("@whereValue", whereValue);

        dbCommand.Parameters.Add (setParam);
        dbCommand.Parameters.Add (whereParam);

instead of doing this, you could actually do this

dbCommand.Parameters.AddWithValue("@setParam", setValue);
dbcommand.Parameters.AddWithValue("@whereValue", whereValue);

that is how I would do with with SQL Server, I don't think it is any different with SqlLite.

also, I would do this a little differently as well

using (var dbConnection = new SqliteConnection ("URI=file:" + Application.dataPath + connection))
{
    dbConnection.Open ();

    using (var dbCommand = dbConnection.CreateCommand ())
    {

I would instead write it like this

using (var dbConnection = new SqliteConnection ("URI=file:" + Application.dataPath + connection))
using (var dbCommand = dbConnection.CreateCommand ())
{
    dbConnection.Open ();

it looks a lot cleaner and removes some of the nesting.


also, try to limit what you do inside the using statements, to whatever you actually need in the using statements and not more than that.

Another thing, don't spread your string onto 3 lines like that, it looks messy.

pull your SQL statement out of the using statements, and make it a simple string, then pass it into the constructor of the dbCommand.

this:

public void UpdateData (string tableName, string whereHeader, object whereValue, string setHeader, object setValue)
{
    using (var dbConnection = new SqliteConnection ("URI=file:" + Application.dataPath + connection))
    {
        dbConnection.Open ();

        using (var dbCommand = dbConnection.CreateCommand ())
        {

            dbCommand.CommandText = string.Format ("UPDATE {0} " +
                                                   "SET {1} = @setValue " +
                                                   "WHERE {2} = @whereValue",
                tableName, setHeader, whereHeader);

            SqliteParameter setParam = new SqliteParameter ("@setValue", setValue);
            SqliteParameter whereParam = new SqliteParameter ("@whereValue", whereValue);

            dbCommand.Parameters.Add (setParam);
            dbCommand.Parameters.Add (whereParam);

            Debug.Log (dbCommand.CommandText);

            dbCommand.ExecuteNonQuery ();
        }
    }
}

should look more like this

public void UpdateData (string tableName, string whereHeader, object whereValue, string setHeader, object setValue)
{
    var sql = string.Format ("UPDATE {0} SET {1} = @setValue WHERE {2} = @whereValue", tableName, setHeader, whereHeader);
    Debug.Log (sql.ToString());

    using (var dbConnection = new SqliteConnection ("URI=file:" + Application.dataPath + connection))
    using (var dbCommand = dbConnection.CreateCommand (sql, dbConnection))
    {
        dbConnection.Open ();

        dbCommand.Parameters.AddWithValue("@setParam", setParam);
        dbcommand.Parameters.AddWithValue("@whereValue", whereValue);

        dbCommand.ExecuteNonQuery ();
    }
}
share|improve this answer
    
Thanks for the criticism! The only issue I have with this is my version of SQLite does not have AddWithValue. So, unfortunately, I am stuck with creating them and adding them separately. In regards to cleaning my method up, the method you provided looks so much cleaner and I will take all of what you said into account when creating the rest of my methods. Do you think it is worth me following up on prepared statements? – Mike Hunt Mar 11 at 20:45
1  
I am not sure that prepared statements are going to work for you with SQLite... you can look into it, it won't hurt to know about them either way. – Malachi Mar 11 at 21:12
    
Ok thanks for the advice! – Mike Hunt Mar 11 at 21:24

To address one aspect in particular:

I attempted to have SQL Injection prevention parameters for tableName, selectHeader and whereHeader, however, it kept throwing an error saying the syntax was wrong. I'm assuming this can't be done then.

Good remark. Yes, this cannot be done, by design (read: it should not be done this way, ideally). What you are trying to do can seem like a good idea on the surface, to allow you to dynamically write an arbitrary SQL query on the fly by passing it DB object identifier names, and even SQL executable code as object parameters.

However, this is a very bad idea in almost 100% of cases (with the very rare edge cases where you purposefully want to inject SQL to execute, which should always be done with extreme prudence).

Here are a few reasons why you shouldn't use this method in production code:

  1. Potential SQL injection vulnerability, obviously;

  2. Queries are not compiled (which is part of what preparing statements does) and therefore ran "ad hoc" against the database each time;

  3. Prepared statements must know about the database objects they run on before they are executed, safeguarding against SQL errors at compile time instead of throwing errors from the database at run time.

  4. Compiled queries are re-usable so this will increase performance.

  5. Debugging dynamic SQL queries is much more difficult than static queries.

Ideally, you would want to write prepared queries that are specific to a single purpose. This takes more time to write the code, but in the end you will be much better off for the reasons stated above.

You should read the SqlCommand.Prepare documentation. I also found an article by Visual Studio Magazine called SQLite Performance and Prepared Statements which looks like it has some more vendor-specific functionality for SQLite.

As a thumb rule for what you actually can use as parameters for queries, see the @param in the following statements:

SELECT col1, col1 FROM table1 WHERE col1 = @param1;

INSERT INTO table1 (col1, col2) VALUES (@param1, @param2);

UPDATE table1 SET col1 = @param1 WHERE col2 = @param2;

DELETE FROM table1 WHERE col1 = @param1;
share|improve this answer
2  
Well I added a comment before but I guess my internet was trashy and didn't post it. Thanks for your answer @PinCrash and I agree with what you said. I will be implementing my system based off yours and Malachi's answer so I thank you guys very much. – Mike Hunt Mar 11 at 22:26

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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