5
\$\begingroup\$

I'm fairly new to C# and I thought this program was so simple it was a great candidate for a good ol' code review. This program syncs the results of one MSSQL database and puts them into a MySQL database based on the datetime value. I always tend to do things the hard way when I start off on a program and I am always seeking to improve this. Let me know if you see anything that needs improvement or if I did anything dumb.

Main Program

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Data.SqlClient;
using MySql.Data.MySqlClient;

namespace TransferOfData
{

    public class TransferData
    {

        static void Main(string[] args)
        {
            ErrorLogger el = new ErrorLogger();
            InitializeDB idb = new InitializeDB();
            MySqlDataReader myrdr = null;
            SqlDataReader msrdr = null;
            string strLastDBUpdate = null;

            idb.OpenDatabases();

            //Run the query that will collect the last date the database was updated from the mssql database
            try
            {
                string sqlLastDBUpdate = "select max(dIntervalStart) intervalStart from forecasts.statistics";
                MySqlCommand mycmd = new MySqlCommand(sqlLastDBUpdate, idb.myconn);
                myrdr = mycmd.ExecuteReader();
                if (myrdr.Read())
                {
                    strLastDBUpdate = myrdr.GetString(0);
                }
                myrdr.Close();
            }
            catch (Exception e)
            {
                el.errorCode = 2;
                el.errorDescription = e.ToString();
                el.createError();
            }

            //Now we will get all of the results from the mssql database and transfer them to the mysql database
            try
            {
                string strCatchUp = "select dIntervalStart, nExterntoInternAcdCalls, cName " +
                                    "from IWrkgrpQueueStats " +
                                    "where SiteId = 1 " +
                                    "and cname in ('applications' , 'employer', " +
                                    "'factfind', " +
                                    "'general', " +
                                    "'other') " +
                                    "and cReportGroup = '*' " +
                                    "and dIntervalStart >  '" + strLastDBUpdate + "'";

                SqlCommand mscmd = new SqlCommand(strCatchUp, idb.msconn);
                msrdr = mscmd.ExecuteReader();
                while (msrdr.Read())
                {
                    try
                    {
                        string strdIntervalStart = msrdr["dIntervalStart"].ToString();
                        int intnExternToInternAcdCalls = System.Convert.ToInt32(msrdr["nExternToInternAcdCalls"]);
                        string cName = msrdr["cName"].ToString();

                        string sqlInsertUpdate = "INSERT INTO `forecasts`.`statistics` (`dIntervalStart`, `nExternToInternAcdCalls`, `cName`, `DateCreated`, `DateModified`) "
                                                    + "VALUES (@dIntervalStart, @nExternToInternAcdCalls, @cName, now(), now())";

                        MySqlCommand cmdInsertUpdate = new MySqlCommand(sqlInsertUpdate, idb.myconn);
                        cmdInsertUpdate.Parameters.AddWithValue("@dIntervalStart", System.Convert.ToDateTime(strdIntervalStart));
                        cmdInsertUpdate.Parameters.AddWithValue("@nExternToInternAcdCalls", intnExternToInternAcdCalls);
                        cmdInsertUpdate.Parameters.AddWithValue("@cName", cName);

                        cmdInsertUpdate.ExecuteNonQuery();
                    }
                    catch (Exception e)
                    {
                        el.errorCode = 3;
                        el.errorDescription = e.ToString();
                        el.createError();
                    }
                }
            }
            catch (Exception e)
            {
                el.errorCode = 4;
                el.errorDescription = e.ToString();
                el.createError();
            }

            idb.myconn.Close();
            idb.msconn.Close();
        }
    }
}

Initialize Database and create connections

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Data.SqlClient;
using MySql.Data.MySqlClient;

namespace TransferOfData
{
    public class InitializeDB
    {
        ErrorLogger el = new ErrorLogger();

        public SqlConnection msconn { get; private set;}
        public MySqlConnection myconn { get; private set;}

        public void OpenDatabases()
        {
            string msConnectionString = "user id=user;" +
                                       "password='password';server=vsc002sql;" +
                                       "Trusted_Connection=yes;" +
                                       "database=I3_IC; " +
                                       "connection timeout=30";
            string myConnectionString = ("SERVER=localhost; DATABASE=forecasts;"
                                                        + "UID=root; PASSWORD=;");

            try
            {
                msconn = new SqlConnection(msConnectionString);
                myconn = new MySqlConnection(myConnectionString);

                msconn.Open();
                myconn.Open();
            }
            catch (Exception e)
            {
                el.errorCode = 1;
                el.errorDescription = e.ToString();
                el.createError();
            }
        }
    }
}

Logging Errors

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.IO;

namespace TransferOfData
{
    public class ErrorLogger
    {
        public int errorCode { private get; set; }
        public string errorDescription { private get; set; }

        public void createError()
        {
            string logPath = @"C:\Logs\Scheduler\transferFileLog.txt";
            string errorFinal;

            errorFinal = "TransferData Error code: " + errorCode + "\n";
            errorFinal += "Description:\n";
            errorFinal += errorDescription + "\n";
            errorFinal += "------------------------------------------------------------\n\n";

            using (StreamWriter sw = File.AppendText(logPath))
            {
                sw.Write(errorFinal);
            }

            Console.WriteLine("An error has occurred.  Check the error log at " + logPath + " for further information.");
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ have you tested this code to make sure that it runs? \$\endgroup\$ – Malachi May 1 '14 at 17:06
  • \$\begingroup\$ ErrorLogger.createError() should be ErrorLogger.CreateError() to be consistent ;) \$\endgroup\$ – Mathieu Guindon May 1 '14 at 17:32
7
\$\begingroup\$

you should use some Using statements so that your connections are disposed of no matter what happens with the code, like if there is an exception.

string sqlLastDBUpdate = "select max(dIntervalStart) intervalStart from forecasts.statistics";
Using (mycmd = new MySqlCommand(sqlLastDBUpdate, idb.myconn))
{    
    myrdr = mycmd.ExecuteReader();
    if (myrdr.Read())
    {
        strLastDBUpdate = myrdr.GetString(0);
    }
    myrdr.Close();
}

You can also put myrdr = mycmd.ExecuteReader(); into a using like this

string sqlLastDBUpdate = "select max(dIntervalStart) intervalStart from forecasts.statistics";
Using (mycmd = new MySqlCommand(sqlLastDBUpdate, idb.myconn))
{    
    Using (myrdr = mycmd.ExecuteReader())
    {
        if (myrdr.Read())
        {
            strLastDBUpdate = myrdr.GetString(0);
        }
    }
}

and then you don't actually need the closing statement because the Using does the closing automatically.


You should also take advantage of the try catch's Finally statement, which you actually won't need if you use a Using statement because it automatically closes the connections.

it would look something like this if you decide to use the Try Catch Finally

try
{
    string sqlLastDBUpdate = "select max(dIntervalStart) intervalStart from forecasts.statistics";
    MySqlCommand mycmd = new MySqlCommand(sqlLastDBUpdate, idb.myconn);
    myrdr = mycmd.ExecuteReader();
    if (myrdr.Read())
    {
        strLastDBUpdate = myrdr.GetString(0);
    }
}
catch (Exception e)
{
     el.errorCode = 2;
     el.errorDescription = e.ToString();
     el.createError();
}
finally
{
     myrdr.Close();
}

with the closing inside the try statement, you don't need it there when you put it in the finally, inside the `finally it will be closed no matter what happens with your code.


I don't know what your InitializeDB idb = new InitializeDB(); is made up of, but if it makes use of the IDisposable interface you could also use that in a Using statement surrounding your data reader Using statement that way no matter where the exception happens the connection is disposed of and so it the reader, and they would be disposed in the order that they need to be.


instead of using a StreamWriter.Write() you could use a StreamWriter.WriteLine() and get rid of an extra variable in the function.

    public void createError()
    {
        string logPath = @"C:\Logs\Scheduler\transferFileLog.txt";
        Using (StreamWriter sw = File.AppendText(logPath))
        {
            sw.WriteLine("TransferData Error code: " + errorCode);
            sw.WriteLine("Description:");
            sw.WriteLine(errorDescription);
            sw.WriteLine("------------------------------------------------------------\n");
            //sw.Write(errorFinal);
        }
       Console.WriteLine("An error has occurred.  Check the error log at " + logPath + " for further information.");
    }

you should add parameters to this function as well, then you can send the variables errorCode and errorDescription to it and take care of it in one swift movement, then you can also get rid of the property declarations, I don't think they are necessary.

just change the Method Declaration to:

public void CreateError(int errorCode, string errorDescription)
\$\endgroup\$
5
\$\begingroup\$

One of the things I see that could be improved is this line:

 "and dIntervalStart >  '" + strLastDBUpdate + "'";

You should use parameters as you did after.

\$\endgroup\$
  • \$\begingroup\$ You are correct. I have just added it as a parameter per your suggestion. \$\endgroup\$ – Neggly May 1 '14 at 16:44

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.