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

Hello to anyone that wants to give suggestions, and thank you in advance. Here is the code I am using. Very specifically I am using code review for just that (critical opinions from better programmers, meaning almost everyone :-) ) I just hope any feedback is directed to a new programmer, even if that means a link for me to learn something applicable I am not afraid to read.

Objective: This is crap bulky code, very simply I want to move this to a class that will open an sqlite connection that can be re-used, rather than disposing of it every time I write to the database. Furthermore I am more than happy for any best practices from sqlite users! I am aware of a similar thread and am trying to absorb it: SQLite Helper Class

    void createDB()
    {
        // Dont forget to del if refreshing
        if (!File.Exists(connstring))
        {
            SQLiteConnection.CreateFile(connstring);
            SQLiteConnection sqliteCon = new SQLiteConnection(@"Data Source=" + connstring);
            sqliteCon.Open();

            // Define db structure
            string createfilesTableSQL = "CREATE TABLE files ( id INTEGER PRIMARY KEY, filename TEXT, creationtime TEXT, lastwrite TEXT, lastaccess TEXT, checksum TEXT);";
            string createfoldersTableSQL = "CREATE TABLE folders ( id INTEGER PRIMARY KEY, dirname TEXT, creationtime TEXT, lastwrite TEXT, lastaccess TEXT, checksum TEXT);";

            using (SQLiteTransaction sqlTransaction = sqliteCon.BeginTransaction())
            {
                // Create the tables
                SQLiteCommand createCommand = new SQLiteCommand(createfilesTableSQL, sqliteCon);
                createCommand.ExecuteNonQuery();
                createCommand.Dispose();

                SQLiteCommand createCommand2 = new SQLiteCommand(createfoldersTableSQL, sqliteCon);
                createCommand2.ExecuteNonQuery();
                createCommand2.Dispose();

                // Commit the changes into the database
                sqlTransaction.Commit();
            } // end using

            // Close the database connection
            sqliteCon.Close();
            sqliteCon.Dispose();
        }
    }
    public int fileExists(string filename, string lastwrite, string hash)
    {
        /* Here is a quick Map
         0 "The file does not exist in the database
         1 "The Hash Matches, leave it alone
         2 "the file is newer than that of the databse
         3 "the file is older than that of the databse"
         4 "the file is the same as that of the database"
         */

        // Connect to database
        SQLiteConnection sqliteCon = new SQLiteConnection(@"Data Source=" + connstring);
        sqliteCon.Open();

        //Query
        //string selectSQL = "SELECT EXISTS(SELECT 1 FROM files WHERE filename=" + filename + " LIMIT 1) as result;";
        string selectSQL = "SELECT * FROM files WHERE filename=" + filename + ";";
        SQLiteCommand selectCommand = new SQLiteCommand(selectSQL, sqliteCon);
        SQLiteDataReader dataReader = selectCommand.ExecuteReader();

        //does file exist
        bool exists = dataReader.Read();

        if (!exists)
        {
            dataReader.Dispose();
            sqliteCon.Dispose();
            return 0; //it doesnt exist in the database
        }

        string dbhash = dataReader["checksum"].ToString();
        string dblastwrite = dataReader["lastwrite"].ToString();
        string lastwritex = lastwrite.Replace("'", "");
        string hashx = hash.Replace("'", "");
        DateTime dblastwriteDT = Convert.ToDateTime(dblastwrite);
        DateTime lastwriteDT = Convert.ToDateTime(lastwritex);

        dataReader.Dispose();
        sqliteCon.Dispose();


        if (dbhash == hashx)
        {
            return 1; //hash matches the database version
        }

        if (lastwriteDT > dblastwriteDT)
        {
            return 2; //the file is newer than that of the databse
        }

        if (lastwriteDT < dblastwriteDT)
        {
            return 3; //the file is older than that of the databse
        }
        else
        {
            return 4; //the file is the same as that of the database
        }
    }
    void cleanDB()
    {
        SQLiteConnection sqliteCon = new SQLiteConnection(@"Data Source=" + connstring);
        sqliteCon.Open();

        // Define db structure
        string createfilesTableSQL = "delete from files;";
        string createfoldersTableSQL = "delete from folders;";

        using (SQLiteTransaction sqlTransaction = sqliteCon.BeginTransaction())
        {
            // Create the tables
            SQLiteCommand createCommand = new SQLiteCommand(createfilesTableSQL, sqliteCon);
            createCommand.ExecuteNonQuery();
            createCommand.Dispose();

            SQLiteCommand createCommand2 = new SQLiteCommand(createfoldersTableSQL, sqliteCon);
            createCommand2.ExecuteNonQuery();
            createCommand2.Dispose();

            // Commit the changes into the database
            sqlTransaction.Commit();
        } // end using

        // Close the database connection
        sqliteCon.Dispose();
    }
    private void addFile(string filename, string creationtime, string lastwrite, string lastaccess, string checksum)
    {
        // Open connection to database
        SQLiteConnection sqliteCon = new SQLiteConnection(@"Data Source=" + connstring);
        sqliteCon.Open();
        using (SQLiteTransaction SQLiteTrans = sqliteCon.BeginTransaction())
        {
            using (SQLiteCommand cmd = sqliteCon.CreateCommand())
            {
                // Insert a new file record
                cmd.CommandText = "INSERT INTO files(filename, creationtime, lastwrite, lastaccess, checksum)" + " VALUES (?,?,?,?,?)";

                SQLiteParameter Field1 = cmd.CreateParameter();
                SQLiteParameter Field2 = cmd.CreateParameter();
                SQLiteParameter Field3 = cmd.CreateParameter();
                SQLiteParameter Field4 = cmd.CreateParameter();
                SQLiteParameter Field5 = cmd.CreateParameter();

                cmd.Parameters.Add(Field1);
                cmd.Parameters.Add(Field2);
                cmd.Parameters.Add(Field3);
                cmd.Parameters.Add(Field4);
                cmd.Parameters.Add(Field5);
                Field1.Value = filename;
                Field2.Value = creationtime;
                Field3.Value = lastwrite;
                Field4.Value = lastaccess;
                Field5.Value = checksum;

                cmd.ExecuteNonQuery();
            }

            SQLiteTrans.Commit();
        }
        sqliteCon.Dispose();
    }
    private void addFolder(string dirname, string creationtime, string lastwrite, string lastaccess, string checksum)
    {

        // Open connection to database
        SQLiteConnection sqliteCon = new SQLiteConnection(@"Data Source=" + connstring);
        sqliteCon.Open();
        using (SQLiteTransaction SQLiteTrans = sqliteCon.BeginTransaction())
        {
            using (SQLiteCommand cmd = sqliteCon.CreateCommand())
            {
                // Insert a new file record
                cmd.CommandText = "INSERT INTO folders(dirname, creationtime, lastwrite, lastaccess, checksum)" + " VALUES (?,?,?,?,?)";

                SQLiteParameter Field1 = cmd.CreateParameter();
                SQLiteParameter Field2 = cmd.CreateParameter();
                SQLiteParameter Field3 = cmd.CreateParameter();
                SQLiteParameter Field4 = cmd.CreateParameter();
                SQLiteParameter Field5 = cmd.CreateParameter();

                cmd.Parameters.Add(Field1);
                cmd.Parameters.Add(Field2);
                cmd.Parameters.Add(Field3);
                cmd.Parameters.Add(Field4);
                cmd.Parameters.Add(Field5);
                Field1.Value = dirname;
                Field2.Value = creationtime;
                Field3.Value = lastwrite;
                Field4.Value = lastaccess;
                Field5.Value = checksum;

                cmd.ExecuteNonQuery();
            }

            SQLiteTrans.Commit();
        }
        sqliteCon.Dispose();
    }
share|improve this question
First suggestion is for public int fileExists you are returning an int that has meaning to it. It would be better to make a enum with those values so that there is a name attached to those numbers. This way in the future you'll know what those values returned mean without having to reread your code – Robert Snyder Apr 21 at 20:02
absolutely! and thanks. It truly is easier to work with, as you can see from my own comments trying to track what return did what. – intaipei Apr 24 at 10:22

1 Answer

up vote 1 down vote accepted

Just a few comments:

  1. Try to use the 'using' construct for SQLiteTransaction, SQLiteCommand and SQLConnection objects everywhere, not just in selected places. Get rid of explicit calls to Dispose() method. Also there is a connection string parameter 'Pooling=true', which tells driver to return previously opened connection to the connection pool and reuse it later. This will improve performance.
  2. Be consistent with naming of your variables. Try to use camelCase for every variable name. There are several places in your code where you start using all lowercase for no good reason.
  3. Try to use parameterization in all queries. The 'SELECT' query concatenates filename directly, but should use parameter instead.
  4. Try to avoid plain numeric return values. You should use enum with self-descriptive member names instead. This will also make the comment describing return codes redundant. Also the function 'fileExists' does a little bit more than just checking if the file exists. You should probably refactor it into several functions with distinct responsibility and self-documenting names.
share|improve this answer
Thank you nullop! I would assume that I should still be closing the connection if I set pooling to true? – intaipei Apr 24 at 10:14
Thank you for your time nullop! Point well taken on enum! I have looked into it and have added it to my code. I am curious, what is the detriment to a direct concat in the select string? Is it just bad SOP or is there something else to learn here? – intaipei Apr 24 at 10:22
If you are using 'using' construct then Close() is called for you automatically by Dispose() method. If you're not using the dispose pattern, then you should call Close() explicitly to let the driver return connection back to the pool. As for the concatenation, it could possibly lead to sql injection if the part of filename comes from user input and is inserted into the sql query text. Parameters are processed automatically to avoid possible sql injections. Also parameterization allows sql engine to re-use previously calculated execution plan and reduces the load applied to sql engine. – nullop Apr 25 at 10:22

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.