2
\$\begingroup\$

When the application connects to the database it keeps the connection open until the user closes the file using File->Close.

Is this bad practice?

Should I be opening and closing the connection every time I need to use it?

using System.Data;
using System.Data.SQLite;
using System.IO;
using System.Windows.Forms;

namespace timelord
{
    /// <summary>
    /// Manages the tasks database
    /// </summary>
    class Timesheet
    {
        private string filePath;
        private SQLiteConnection sqlite;
        private SQLiteDataAdapter adapter;
        private SQLiteCommandBuilder builder;
        private DataTable datatable;

        /// <summary>
        /// Creates a timesheet object that determines if the filepath exists.
        /// If the filepath exists then it opens a database.
        /// If it does not exist it creates a database.
        /// </summary>
        /// <param name="filePath">The path to open or create a database file at.</param>
        public Timesheet(string filePath)
        {
            this.filePath = filePath;

            if (File.Exists(filePath))
            {
                openDatabase();

            }else
            {
                createDatabase();

                createSchema();
            }

            prepareQueries();

            this.datatable = new DataTable();

            this.adapter.Fill(this.datatable);
        }

        /// <summary>
        /// Calls the methods that create the database file, connection, and schema
        /// </summary>
        private void createDatabase()
        {
            SQLiteConnection.CreateFile(filePath);

            openDatabase();
        }

        /// <summary>
        /// Creates the database connection
        /// </summary>
        private void openDatabase()
        {
            sqlite = new SQLiteConnection("Data Source=" + this.filePath + ";Version=3;");
            try
            {
                sqlite.Open();
            }
            catch(SQLiteException e)
            {
                MessageBox.Show(e.Message);
            }
        }

        /// <summary>
        /// Creates the schema for all tables nessicary
        /// </summary>
        private void createSchema()
        {
            string query = "CREATE TABLE task (id INTEGER PRIMARY KEY AUTOINCREMENT, description TEXT, begindate TEXT, enddate TEXT, status INTEGER default 0)";

            SQLiteCommand cmd = new SQLiteCommand(query, sqlite);

            cmd.ExecuteNonQuery();

        }

        /// <summary>
        /// Prepares the queries used to manipulate the timesheet
        /// </summary>
        private void prepareQueries()
        {
            adapter = new SQLiteDataAdapter("select id,description,begindate,enddate,status from task", sqlite);

            builder = new SQLiteCommandBuilder(adapter);

            adapter.UpdateCommand = builder.GetUpdateCommand();
            adapter.DeleteCommand = builder.GetDeleteCommand();
            adapter.InsertCommand = builder.GetInsertCommand();
        }

        /// <summary>
        /// Return the task table
        /// </summary>
        /// <returns></returns>
        public DataTable Tasks()
        {
            return this.datatable;
        }

        public void Update()
        {
            adapter.Update(datatable);

            datatable.Clear();

            adapter.Fill(datatable);
        }

        /// <summary>
        /// Closes the database connection
        /// </summary>
        public void close()
        {
            sqlite.Close();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I don't know how to answer your question. But MS EntityFramework will be your good friend about managing database. \$\endgroup\$ – AechoLiu Aug 17 '16 at 7:07
  • \$\begingroup\$ stackoverflow.com/a/10460418/648075 \$\endgroup\$ – BCdotWEB Aug 17 '16 at 7:52
4
\$\begingroup\$

You don't need to create the database file if it doesn't exist, the SQLite driver will take care of it; to ensure your table does not exist on creation, you can just add IF NOT EXISTS to the query.

I would also close the connection every time, this has already been covered by other answers. In addition I would make the class stateless as it doesn't need a state if we open/close the connection every time (Being stateless brings a lot of other advantages).

I would also remove any logic from the constructor, to do this you will need a state or you will have to check on every command unfortunately so I left it to you.

Here is my 0.02:

internal class Timesheet
{
    private const string SelectTaskQuery = "SELECT id, description, begindate, enddate, status FROM task";

    private readonly string _filePath;

    public Timesheet(string filePath)
    {
        _filePath = filePath;

        InitializeDb();
    }

    private DataTable Tasks() => ExecuteWithConnection(Tasks);

    private void InitializeDb() => ExecuteWithConnection(connection =>
    {
        using (var command = connection.CreateCommand())
            CreateSchemaIfNotExists(command);
    });

    private SQLiteConnection GetConnection() => new SQLiteConnection("Data Source=" + _filePath + ";Version=3;");

    private void ExecuteWithConnection(Action<SQLiteConnection> action)
    {
        using (var connection = GetConnection())
        {
            connection.Open();

            action(connection);
        }
    }

    private T ExecuteWithConnection<T>(Func<SQLiteConnection, T> action)
    {
        using (var connection = GetConnection())
        {
            connection.Open();

            return action(connection);
        }
    }

    private static DataTable Tasks(SQLiteConnection connection)
    {
        var adapter = new SQLiteDataAdapter(SelectTaskQuery, connection);

        var datatable = new DataTable();

        adapter.Fill(datatable);

        return datatable;
    }

    public void Update(DataTable dataTable)
    {
        ExecuteWithConnection(connection =>
        {
            var adapter = new SQLiteDataAdapter(SelectTaskQuery, connection);

            var builder = new SQLiteCommandBuilder(adapter);

            adapter.Update(dataTable);
        });
    }

    private static void CreateSchemaIfNotExists(IDbCommand command)
    {
        const string query = "CREATE TABLE IF NOT EXISTS task (id INTEGER PRIMARY KEY AUTOINCREMENT, description TEXT, begindate TEXT, enddate TEXT, status INTEGER default 0)";

        command.CommandText = query;

        command.ExecuteNonQuery();
    }
}

I would also add a small ORM like Dapper and add statically typed entities for insert/update.

\$\endgroup\$
  • \$\begingroup\$ Damn this is complicated. I don't know half of these syntaxes. \$\endgroup\$ – Ben Aug 17 '16 at 20:55
  • 1
    \$\begingroup\$ @Ben using implicitly calls Dispose(). \$\endgroup\$ – Stefano d'Antonio Aug 24 '16 at 18:14
  • 1
    \$\begingroup\$ Correct, I think "Update" is a bit confusing as name then. I would call it "Commit" maybe and add the other commands. \$\endgroup\$ – Stefano d'Antonio Aug 24 '16 at 18:58
  • 1
    \$\begingroup\$ @Stefanod'Antonio this looks great. I think the Update() should be done within a transaction too, particularly because SQLite will be very slow when updating lots of rows as it'll create an implicit transaction for each one. \$\endgroup\$ – Rory Feb 14 '18 at 23:24
  • 1
    \$\begingroup\$ @Stefanod'Antonio - method private T ExecuteWithConnection<T>(Func<SQLiteConnection, T> action) should also open the connection before use. \$\endgroup\$ – Rory Feb 15 '18 at 14:31
3
\$\begingroup\$

Few remarks

  1. Try to make your class reusable. For example it cannot be used in web or console application, because it has reference to namespace System.Windows.Forms. Delete it, delete MessageBox.Show(e.Message); and let client of your library decide itself how to handle exceptions.
  2. There are a lot of general .Net name convention violations ... Here for example createSchema. Use CreateSchema instead.
  3. I would make your class more lazy :). I mean i would not get all data from database in constructor but only when Tasks method will be called.
\$\endgroup\$
2
\$\begingroup\$

First of all, you shouldn't use comments unless you're publishing this as a library or it's very hard to represent what function and classes do by its name.

CreateDatabase() or OpenDatabase() it's pretty straightfoward it does what its name says so a comment it isn't really useful, unless you have many variations of the same function.

Instead of leaving the connection open you could do this when executing queries:

 using (MySqlConnection con = new MySqlConnection(ConnectionString))
 {
   using(MysqlCommand cmd = new MySqlCommand(Command, con);   
   {
     //Add parameters and execute
   }
 }

Besides this I can't see anythin "wrong".

\$\endgroup\$
  • \$\begingroup\$ My college assignments require everything commented so I have a bad habit of making useless comments \$\endgroup\$ – Ben Aug 17 '16 at 15:16
2
\$\begingroup\$

I would create a separate class to handle all interactions wit SQL. Preferably a singleton class so I can reuse it. From there you can handle updates by something like passing in list of columns and the values you would like to update, also including the table. It is pretty flexible on how you want to implement it.

I normally just try to apply the rule of re-usability and scalability, also with KISS (keep it simple stupid) lastly DRY (Don't repeat yourself).

\$\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.