Method Optimisation - C#

Question

I've developed a method that allows me to pass in a table (string), array of columns (string) and array of values (object) through the parameters which I then use to create a parameterized query. Although it works fine the length of the code as well as the multiple for loops gave off a code smell, in particular I feel the method I use to insert a comma between the columns and values can be done a different and better way.

public static int Insert(string source, string[] column, object[] values)
{
    int rowsAffected = 0;
    try
    {
        using (SQLiteConnection conn = new SQLiteConnection(connectionString))
        {
            StringBuilder query = new StringBuilder();
            query.Append(" INSERT INTO ");
            query.Append(source);
            query.Append("(");

            for (int i = 0; i < column.Length; i++)
            {
                query.Append(column[i]);

                if (i < values.Length - 1)
                {
                    query.Append(",");
                }
            }

            query.Append(")");
            query.Append(" VALUES ");
            query.Append("(");

            for (int i = 0; i < values.Length; i++)
            {
                query.Append("@" + values[i].ToString());

                if (i < values.Length - 1)
                {
                    query.Append(",");
                }
            }

            query.Append(")");

            conn.Open();
            using (SQLiteCommand cmd = new SQLiteCommand(query.ToString(), conn))
            {
                for (int i = 0; i < values.Length; i++)
                {
                    cmd.Parameters.AddWithValue("@" + values[i].ToString(), values[i]);
                }
                rowsAffected = cmd.ExecuteNonQuery();
            }
        }
        return rowsAffected;
    }
    catch (Exception e)
    {
        MessageBox.Show(e.Message);
    }
    return 0;
}

I'm using the System.Data.SQLite library to interact with the database.

Thanks for any suggestions!

Answer 1

This is my idiomatic way to append multiple values with a separator using StringBuilder:

string separator = ",";
for (int i = 0; i < column.Length; i++)
{
    query.Append(column[i]);
    query.Append(separator);
}
query.Length -= separator.Length;

This assumes you will have at least one value, and usually where I use it, it would be an error not to have at least one value (and it appears your scenario is like that).

It also appears that you have left this code wide open for SQL Injection.

You seem to be trying to use parameters, but I don't think you've done it correctly. The way I read the code, you are using the actual value of the parameters instead of their index. I would suggest this modification (this assumes your array of column names comes from a trusted source, but that your values do not):

        for (int i = 0; i < values.Length; i++)
        {
            query.Append("@" + i.ToString()); // instead of query.Append("@" + values[i].ToString());

            if (i < values.Length - 1)
            {
                query.Append(",");
            }
        }

        query.Append(")");

        conn.Open();
        using (SQLiteCommand cmd = new SQLiteCommand(query.ToString(), conn))
        {
            for (int i = 0; i < values.Length; i++)
            {
                cmd.Parameters.AddWithValue("@" + i.ToString(), values[i]); // instead of cmd.Parameters.AddWithValue("@" + values[i].ToString(), values[i]);
            }
            rowsAffected = cmd.ExecuteNonQuery();
        }
    }

Answer 2

The number of parameters and values has to always be the same for this to work, so you could eliminate a loop by using two StringBuilders. (Untested code, but it should get the point across)

            StringBuilder query = new StringBuilder();
            StringBuilder insertParams = new StringBuilder();
            query.Append(" INSERT INTO ");
            query.Append(source);
            query.Append("(");

            for (int i = 0; i < column.Length; i++)
            {

                if (i < values.Length - 1)
                {
                    query.Append(",");
                    insertParams.Append(",");
                }
                query.Append(column[i]);
                insertParams.Append("@" + values[i].ToString());
            }

            query.Append(")");
            query.Append(" VALUES ");
            query.Append("(");
            query.Append(insertValues.ToString());
            query.Append(")");

Since the lengths are the same you can build the parameter list and the value list at the same time, and then just stick the value list in the appropriate spot at the end of the loop. Net result should be faster. :)

Answer 3

Here is another option. Really doing the same thing your original code does but breaking it up into smaller chunks and abstracting it into a class called InsertBuilder.

public class InsertBuilder
{
    public InsertBuilder()
    {
    }

    public InsertBuilder(string tableName, string[] columns, object[] values)
    {
        this.tableName = tableName;
        this.columns = columns;
        this.values = values;
    }

    private string tableName;
    public string TableName
    {
        get { return tableName; }
        set { tableName = value; }
    }

    private string[] columns;
    public string[] Columns
    {
        get { return columns; }
        set { columns = value; }
    }


    private object[] values;
    public object[] Values
    {
        get { return values; }
        set { values = value; }
    }

    public string InsertString
    {
        get
        {
            return CreateInsertString();
        }
    }

    public void Clear()
    {
        this.values = null;
        this.columns = null;
        this.tableName = null;
    }

    private string CreateInsertString()
    {
        if(columns.Length == 0) 
            throw new InvalidOperationException(
                "Columns must contain atleast one column"
                );

        if(values.Length == 0) 
            throw new InvalidOperationException(
                "Values must contain atleast one value"
                );

        if(columns.Length != values.Length)
        {
            throw new InvalidOperationException(
                string.Format(
                    "Columns length {0} does not match Values length {1}",
                    columns.Length,
                    values.Length)
                    );
        }

        StringBuilder insertString = new StringBuilder();

        insertString.Append(CreateTableStatement());

        insertString.Append(CreateColumnsStatement());

        insertString.Append(CreateValuesStatement());

        return insertString.ToString();

    }

    private string CreateTableStatement()
    {
        return " INSERT INTO " + tableName;
    }

    private string CreateColumnsStatement()
    {
        StringBuilder columnsStatement = new StringBuilder();

        columnsStatement.Append("(");

        for(int i = 0;i < columnsStatement.Length;i++)
        {
            columnsStatement.Append(columnsStatement[i]);
            if(i < values.Length - 1) { columnsStatement.Append(","); }
        }

        columnsStatement.Append(")");

        return columnsStatement.ToString();
    }

    private string CreateValuesStatement()
    {
        StringBuilder valuesStatement = new StringBuilder();

        valuesStatement.Append("VALUES");
        valuesStatement.Append("(");

        for(int i = 0;i < values.Length;i++)
        {
            valuesStatement.Append("@" + values[i].ToString());

            if(i < values.Length - 1) { valuesStatement.Append(","); }
        }

        valuesStatement.Append(")");

        return valuesStatement.ToString();
    }

}

Then your original code looks something like this.

public static int Insert(string source, string[] column, object[] values)
{
    int rowsAffected = 0;
    try
    {
        using(SQLiteConnection conn = new SQLiteConnection(connectionString))
        {
            InsertBuilder insertBuilder = new InsertBuilder();
            insertBuilder.TableName = source;
            insertBuilder.Columns = column;
            insertBuilder.Values = values;

            using(SQLiteCommand cmd = new SQLiteCommand(insertBuilder.InsertString, conn))
            {
                for(int i = 0;i < values.Length;i++)
                {
                    cmd.Parameters.AddWithValue("@" + values[i].ToString(), values[i]);
                }

                conn.Open();

                rowsAffected = cmd.ExecuteNonQuery();
            }
        }

        return rowsAffected;
    }
    catch(Exception e)
    {
        MessageBox.Show(e.Message);
    }

    return 0;
}