3
\$\begingroup\$

I have recently implemented my SQLite helper class that supports SQLite in a memory class to be opened to not to be lost. Please review it and tell me if there is a coding problem and tell me what to do to prevent\fix it.

using System;
using System.Collections.Generic;
using System.Data;
using System.Data.SQLite;
using System.Diagnostics;
using System.Globalization;
using System.IO;
using System.Linq;

namespace SQLite
{
    public class SqLiteDatabase : IDisposable
    {
        private readonly SQLiteConnection _dbConnection;

        /// <summary>
        /// Default Constructor for SQLiteDatabase Class.
        /// </summary>
        public SqLiteDatabase()
        {
            _dbConnection = new SQLiteConnection("Data Source=default.s3db");
        }

        /// <summary>
        /// Single Param Constructor to specify the datasource.
        /// </summary>
        /// <param name="datasource">The data source. Use ':memory:' for in memory database.</param>
        public SqLiteDatabase(String datasource)
        {
            _dbConnection = new SQLiteConnection(string.Format("Data Source={0}", datasource));
        }

        /// <summary>
        /// Single Param Constructor for specifying advanced connection options.
        /// </summary>
        /// <param name="connectionOpts">A dictionary containing all desired options and their values.</param>
        public SqLiteDatabase(Dictionary<String, String> connectionOpts)
        {
            String str = connectionOpts.Aggregate("",
                                                  (current, row) =>
                                                  current + String.Format("{0}={1}; ", row.Key, row.Value));
            str = str.Trim().Substring(0, str.Length - 1);
            _dbConnection = new SQLiteConnection(str);
        }

        #region IDisposable Members

        public void Dispose()
        {
            if (_dbConnection != null)
                _dbConnection.Dispose();

            GC.Collect();
            GC.SuppressFinalize(this);
        }

        #endregion

        public bool OpenConnection()
        {
            try
            {
                if (_dbConnection.State == ConnectionState.Closed)
                    _dbConnection.Open();
                return true;
            }
            catch (Exception e)
            {
                Console.WriteLine("SQLite Exception : {0}", e.Message);
            }

            return false;
        }

        public bool CloseConnection()
        {
            try
            {
                _dbConnection.Close();
                _dbConnection.Dispose();
            }
            catch (Exception e)
            {
                Console.WriteLine("SQLite Exception : {0}", e.Message);
            }

            return false;
        }

        /// <summary>
        /// Gets the specified table from the Database.
        /// </summary>
        /// <param name="sql">The table to retrieve from the database.</param>
        /// <returns>A DataTable containing the result set.</returns>
        public DataTable GetDataTable(string sql)
        {
            var table = new DataTable();

            try
            {
                using (SQLiteTransaction transaction = _dbConnection.BeginTransaction())
                {
                    using (var cmd = new SQLiteCommand(_dbConnection) {Transaction = transaction, CommandText = sql})
                    {
                        using (SQLiteDataReader reader = cmd.ExecuteReader())
                        {
                            table.Load(reader);
                            transaction.Commit();
                        }
                    }
                }

                return table;
            }
            catch (Exception e)
            {
                Console.WriteLine("SQLite Exception : {0}", e.Message);
            }
            finally
            {
                table.Dispose();
            }

            return null;
        }

        /// <summary>
        /// Executes a NonQuery against the database.
        /// </summary>
        /// <param name="sql">The SQL to execute.</param>
        /// <returns>A double containing the time elapsed since the method has been executed.</returns>
        public double? ExecuteNonQuery(string sql)
        {
            Stopwatch s = Stopwatch.StartNew();

            try
            {
                using (SQLiteTransaction transaction = _dbConnection.BeginTransaction())
                {
                    using (var cmd = new SQLiteCommand(_dbConnection) {Transaction = transaction})
                    {
                        foreach (string line in new LineReader(() => new StringReader(sql)))
                        {
                            cmd.CommandText = line;
                            cmd.ExecuteNonQuery();
                        }

                        transaction.Commit();
                    }
                }
                s.Stop();

                return s.Elapsed.TotalMinutes;
            }
            catch (Exception e)
            {
                Console.WriteLine("SQLite Exception : {0}", e.Message);
            }

            return null;
        }

        /// <summary>
        /// Gets a single value from the database.
        /// </summary>
        /// <param name="sql">The SQL to execute.</param>
        /// <returns>Returns the value retrieved from the database.</returns>
        public string ExecuteScalar(string sql)
        {
            try
            {
                using (SQLiteTransaction transaction = _dbConnection.BeginTransaction())
                {
                    using (var cmd = new SQLiteCommand(_dbConnection) {Transaction = transaction, CommandText = sql})
                    {
                        object value = cmd.ExecuteScalar();
                        transaction.Commit();
                        return value != null ? value.ToString() : "";
                    }
                }
            }
            catch (Exception e)
            {
                Console.WriteLine("SQLite Exception : {0}", e.Message);
            }

            return null;
        }

        /// <summary>
        /// Updates specific rows in the database.
        /// </summary>
        /// <param name="tableName">The table to update.</param>
        /// <param name="data">A dictionary containing Column names and their new values.</param>
        /// <param name="where">The where clause for the update statement.</param>
        /// <returns>A boolean true or false to signify success or failure.</returns>
        public bool Update(String tableName, Dictionary<String, String> data, String where)
        {
            string vals = "";
            if (data.Count >= 1)
            {
                vals = data.Aggregate(vals,
                                      (current, val) =>
                                      current +
                                      String.Format(" {0} = '{1}',", val.Key.ToString(CultureInfo.InvariantCulture),
                                                    val.Value.ToString(CultureInfo.InvariantCulture)));
                vals = vals.Substring(0, vals.Length - 1);
            }
            try
            {
                ExecuteNonQuery(String.Format("update {0} set {1} where {2};", tableName, vals, where));
                return true;
            }
            catch (Exception e)
            {
                Console.WriteLine("SQLite Exception : {0}", e.Message);
            }

            return false;
        }

        /// <summary>
        /// Deletes specific rows in the database.
        /// </summary>
        /// <param name="tableName">The table from which to delete.</param>
        /// <param name="where">The where clause for the delete.</param>
        /// <returns>A boolean true or false to signify success or failure.</returns>
        public bool Delete(String tableName, String where)
        {
            try
            {
                ExecuteNonQuery(String.Format("delete from {0} where {1};", tableName, where));
                return true;
            }
            catch (Exception e)
            {
                Console.WriteLine("SQLite Exception : {0}", e.Message);
            }

            return false;
        }

        /// <summary>
        /// Inserts new data to the database.
        /// </summary>
        /// <param name="tableName">The table into which the data will be inserted.</param>
        /// <param name="data">A dictionary containing Column names and data to be inserted.</param>
        /// <returns>A boolean true or false to signify success or failure.</returns>
        public bool Insert(String tableName, Dictionary<String, String> data)
        {
            string columns = "";
            string values = "";
            foreach (var val in data)
            {
                columns += String.Format(" {0},", val.Key);
                values += String.Format(" '{0}',", val.Value);
            }
            columns = columns.Substring(0, columns.Length - 1);
            values = values.Substring(0, values.Length - 1);
            try
            {
                ExecuteNonQuery(String.Format("insert into {0}({1}) values({2});", tableName, columns, values));
                return true;
            }
            catch (Exception e)
            {
                Console.WriteLine("SQLite Exception : {0}", e.Message);
            }

            return false;
        }

        /// <summary>
        /// Wipes all the data from the database.
        /// </summary>
        /// <returns>A boolean true or false to signify success or failure.</returns>
        public bool WipeDatabase()
        {
            DataTable tables = null;

            try
            {
                tables = GetDataTable("select NAME from SQLITE_MASTER where type='table' order by NAME;");
                foreach (DataRow table in tables.Rows)
                {
                    WipeTable(table["NAME"].ToString());
                }
                return true;
            }
            catch (Exception e)
            {
                Console.WriteLine("SQLite Exception : {0}", e.Message);
            }
            finally
            {
                if (tables != null) tables.Dispose();
            }

            return false;
        }

        /// <summary>
        /// Wipes all the data from the specified table.
        /// </summary>
        /// <param name="table">The table to be wiped.</param>
        /// <returns>A boolean true or false to signify success or failure.</returns>
        public bool WipeTable(String table)
        {
            try
            {
                ExecuteNonQuery(String.Format("delete from {0};", table));
                return true;
            }
            catch (Exception e)
            {
                Console.WriteLine("SQLite Exception : {0}", e.Message);
            }

            return false;
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ throw new Exception(e.Message); is just bad. I no longer have Visual Studio, so I cannot list all of the problems. You do have a few. \$\endgroup\$ – Leonid Feb 28 '13 at 20:56
  • \$\begingroup\$ @Leonid So what do you suggest instead ? \$\endgroup\$ – RuneS Feb 28 '13 at 20:58
  • \$\begingroup\$ In some places you should not catch it at all; otherwise read this carefully - stackoverflow.com/questions/178456/… \$\endgroup\$ – Leonid Feb 28 '13 at 21:00
  • 2
    \$\begingroup\$ The following code is useless: catch (Exception e) { throw new Exception(e.Message); }. It has almost the same effect as not having the try/catch at all (unless you also have a finally clause - in that case you do need a try). \$\endgroup\$ – Leonid Feb 28 '13 at 21:05
  • 1
    \$\begingroup\$ catch (Exception e) { Console.WriteLine("SQLite Exception : {0}", e.Message); This is bad. If it is reused in a GUI application then there is no console attached (and only can be attached via some API calls). You should either throw those exceptions or accept some sort of Logger. \$\endgroup\$ – Bobby Jul 22 '13 at 12:55
3
\$\begingroup\$

In Dispose():

  1. You are calling GC.Collect() which isn't necessarily need unless it is very important to collect all inaccessible memory when disposing.
  2. GC.SuppressFinalize(this) isn't need because the class don't have a finalizer/deconstructor

In CloseConnection() you are always returning false. Why not make it void?

You are handling a lot of exceptions in this "framework", why not let the "client" handle the errors? It will make you code more maintainable and easy to read. It would remove a bunch of return and try..catch statements (as discussed in the comments), and allows the "client" to handle the exceptions differently if needed.

\$\endgroup\$
  • \$\begingroup\$ You mean that i remove the try catch from the database sided codes ? \$\endgroup\$ – RuneS Feb 28 '13 at 23:00
  • 2
    \$\begingroup\$ Yes, fx. in OpenConnection() why not remove the try..catch completely and let the user handle the exception the way they want to? \$\endgroup\$ – Xharze Feb 28 '13 at 23:16
3
\$\begingroup\$

It looks to be a reasonable design and I am reworking mine a bit based on some ideas I have seen here. But I don't like the idea of not being able to unit test, which this design would not allow for.

I also do not like to see hardcoded values in the helper class. Those should come in from the outside. I do agree with the other posts that are saying not to do the try catch here, as it hides it from the call code. At this level in a framework, you'd not have try/catch blocks and you let the errors simply roll up.

I'd recommend adding dependency injection. Then in your application that uses this framework, simply define the objects there, and everything will pass in. This will allow you to mock the class and add true unit tests. Currently you cannot do that.

Also, if you wanted to use this helper for an application with multiple database files, you'd have to create multiple helpers like this. A better approach would be to pass it in through construction injection, and allow the helper to be used with any database.

Also, I am not sure what the point is of a transaction on the GetDataTable() method. When you commit this, what is being committed? It is a read-only process.

Here's my winForm code I used to setup the database. This is the only place in the code where the database is defined. If I wanted a second database as well, I could simply create a second _dbConnection object using a different name.

I am using a DAO class per database I am connecting to. So my form talks to the DAO, my DAO is the only way to get to the SQLite helper class. I setup the db connection in the form, and pass it in. That way I can mock the DAO class for unit testing without dependencies on the database itself. I have not, but I will implement interfaces as needed. But I still need more time to work through this is bit.

So this is my current approach after about 12-hours of working with SQLite. So take that for what it is worth, as I am new to SQLite.

UPDATE: As I was writing this I desided to change my connection to just a connection string, that way my forms could work with Access, SQLite, or SQL Server by just using different DAO or Helper classes.

    private readonly SQLiteConnection _dbConnection = new SQLiteConnection("Data Source=LanguageTutor.db;Version=3;New=False;Compress=True;");        
    private readonly Dao _dao;

    public Form2()
    {
        InitializeComponent();
        _dao = new Dao(_dbConnection);
    }

Then my DAO class

    public class Dao
    {

    private readonly SqLiteHelper _sql;
    private readonly SQLiteConnection _dbConnection;

    public Dao(SQLiteConnection dbConnection)
    {
        _dbConnection = dbConnection;
        _sql = new SqLiteHelper(_dbConnection);
    }

Now here's how I reworked my own SQLite helper class. This just covers the constructors and dependency injection and I took the dictionary one from you as well.

    private readonly SQLiteConnection _dbConnection; 

    #region Constructors

    /// <summary>
    /// Default Constructor for SQLiteDatabase Class.
    /// </summary>
    public SqLiteHelper(SQLiteConnection sqLiteConnection)
    {
        _dbConnection = sqLiteConnection;
    }

    public SqLiteHelper(SQLiteConnection sqLiteConnection, Dictionary<String, String> connectionOpts)
    {
        String str = connectionOpts.Aggregate("", (current, row) => current + String.Format("{0}={1}; ", row.Key, row.Value));
        str = str.Trim().Substring(0, str.Length - 1);

        _dbConnection = sqLiteConnection;
        _dbConnection.ConnectionString = str;
    }

    #endregion
\$\endgroup\$
2
\$\begingroup\$

IDisposable implementation

Implementing IDisposable is a little more complicated than just Dispose(). You will want to override at least the protected virtual void Dispose(bool disposing) method. For details see Jon Skeet's answer here: https://stackoverflow.com/questions/574019/calling-null-on-a-class-vs-dispose/574659#574659

Exception handling

I personally dislike bool return values to report success or error. It makes me write:

using (SqLiteDatabase db = new SqLiteDatabase())
{
    if (!db.OpenConnection())
        // handle error
        return;

    DataTable table = db.GetDataTable("table");
    if (table == null)
        // handle error
        return;

    string value = db.ExecuteScalar(statement);
    if (value == null)
        // handle error
        return;

    if (!db.Update(...))
        // handle error
        return
}

Not only the error conditions are inconsistent (sometimes false is returned, other times null), it's quite easy to ignore the return values.

I prefer

try
{
    using (SqLiteDatabase db = new SqLiteDatabase())
    {
        db.OpenConnection();
        DataTable table = db.GetDataTable("table");
        string value = db.ExecuteScalar(statement);
        db.Update(...)
    }
}
catch (SqLite.FailedToOpenDatabaseException e)
{
    // handle
}
catch (SqLite.TableDoesNotExistException e)
{
    // handle
}
catch (SqLite.FailedToUpdateTable e)
{
    // handle
}

(given that Dispose closes the connection)

Loose ends

  • I would't include timing/profiling in ExecuteNonQuery as it does not really fit with the rest of the interface.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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