4
\$\begingroup\$

I am creating an application that will move specific data from one Oracle database and moving it to a MySQL database. I created this class to help keep myself organized and this class holds all of the operations/communication that will happen with this database. I have learned C# on my own with no formal training I was just wanting some pointers that could help me make my code to be acceptable if I was working as a developer for a company.

class Oracle
{
    //private properties
    private OracleConnection oConnection;
    private XmlDocument oXML;
    private string oUID;
    private string oPassword;
    private string oSecurity;
    private string oDb;
    private string oConString;
    private OracleDataAdapter oCommand;
    private string oScript;
    private DataSet oData;
    private DataTable oTable;
    public Oracle()
    {
        oConnection = new OracleConnection();
        oXML = new XmlDocument();
        try
        {

            oXML.Load(@"C:\Users\V64016\documents\visual studio 2013\Projects\Test_DB_Connection\Test_DB_Connection\Resources\Test.xml");
            oSecurity = oXML.SelectSingleNode("/Connections/Oracle/Security").InnerText;
            oDb = oXML.SelectSingleNode("/Connections/Oracle/Database").InnerText;
            oUID = oXML.SelectSingleNode("/Connections/Oracle/UId").InnerText;
            oPassword = Encrypt.DecryptString(oXML.SelectSingleNode("/Connections/Oracle/Pwd").InnerText,oUID);
            oConString = "Data Source=" + oDb + ";User Id=" + oUID + ";Password=" + oPassword + ";";
        }
        catch(Exception ex)
        {
            Console.WriteLine(ex.ToString());
            Debug.Print(ex.ToString());
        }
    }
    //method that handles opening the connection to the  database
    public void Open()
    {
        try
        {
            oConnection.ConnectionString = oConString;
            oConnection.Open();
            Console.WriteLine("Oracle DB Connection Established");
        }
        catch(Exception ex)
        {
            Console.WriteLine(ex.ToString());
            Debug.Print(ex.ToString());
        }
    }
    //method that handles closing the connection to the database
    public void Close()
    {
        try
        {
            oConnection.Close();
            Console.WriteLine("Oracle DB Connection Closed");
        }
        catch(Exception ex)
        {
            Console.WriteLine(ex.ToString());
            Debug.Print(ex.ToString());
        }
    }
    //method that handles pulling the data from a table
    public DataTable Pull()
    {
        oCommand = new OracleDataAdapter();
        oData = new DataSet();
        oTable = new DataTable();
        try
        {
            oScript = System.IO.File.ReadAllText(@"C:\Users\V64016\documents\visual studio 2013\Projects\Test_DB_Connection\Test_DB_Connection\Resources\CASE_SELECT.sql");
            oCommand.SelectCommand = new OracleCommand(oScript, oConnection);
            oCommand.Fill(oData, "uds9");
            oTable = oData.Tables["uds9"];
        }
        catch(Exception ex)
        {
            Console.WriteLine(ex.ToString());
            Debug.Print(ex.ToString());
        }
        return oTable;
    }
}
\$\endgroup\$
1
\$\begingroup\$

The code looks good, however, some point I like to mention about readability..

I can see there are multiple try-catch blocks. Every catch block does same thing. You can easily terminate duplicate code.

public static void LogThisCode(Action action)
{
    try
    {
        action();
    }
    catch (Exception ex)
    {
        Console.WriteLine(ex.ToString());
        Debug.Print(ex.ToString());
    }
}

Call it like this:

public void Open()
{
    LogThisCode(() => {

        oConnection.ConnectionString = oConString;
        oConnection.Open();
        Console.WriteLine("Oracle DB Connection Established");

    });  
}

  • You don't need all the variables to be global. Avoid using them. They aren't easy to track and create confusion when code grows.

  • I've moved oConnection.ConnectionString = oConString; inside constructor to avoid global variable.

  • Instead of comments on methods, use XML Documentation Comments

  • Always check and remove any unused code

  • You can use String.Format instead of concatenate strings

  • I would suggest you to eliminate all hard coded strings by introducing a class having constants.


After all above changes code will look like this:

class Oracle
{
    private OracleConnection oConnection;
    private OracleDataAdapter oCommand;

    public static void LogThisCode(Action action)
    {
        try
        {
            action();
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.ToString());
            Debug.Print(ex.ToString());
        }
    }

    public Oracle()
    {
        LogThisCode(() =>
        {
            oConnection = new OracleConnection();
            var oXML = new XmlDocument();
            oXML.Load(@"C:\Users\V64016\documents\visual studio 2013\Projects\Test_DB_Connection\Test_DB_Connection\Resources\Test.xml");

            string oDb = oXML.SelectSingleNode("/Connections/Oracle/Database").InnerText;
            string oUID = oXML.SelectSingleNode("/Connections/Oracle/UId").InnerText;
            string oPassword = Encrypt.DecryptString(oXML.SelectSingleNode("/Connections/Oracle/Pwd").InnerText, oUID);
            string oConString = string.Format("Data Source={0};User Id={1};Password={2};", oDb, oUID, oPassword);
            oConnection.ConnectionString = oConString;
        });
    }

    /// <summary>
    /// Method that handles opening the connection to the  database
    /// </summary>
    public void Open()
    {
        LogThisCode(() =>
        {
            oConnection.Open();
            Console.WriteLine("Oracle DB Connection Established");  
        });
    }

    /// <summary>
    /// Method that handles closing the connection to the database
    /// </summary>
    public void Close()
    {
        LogThisCode(() =>
        {
            oConnection.Close();
            Console.WriteLine("Oracle DB Connection Closed");
        });
    }

    /// <summary>
    /// Method that handles pulling the data from a table
    /// </summary>
    /// <returns></returns>
    public DataTable Pull()
    {
        DataTable oTable = new DataTable();

        LogThisCode(() =>
        {
            string oScript = System.IO.File.ReadAllText(@"C:\Users\V64016\documents\visual studio 2013\Projects\Test_DB_Connection\Test_DB_Connection\Resources\CASE_SELECT.sql");
            oCommand.SelectCommand = new OracleCommand(oScript, oConnection);

            DataSet oData = new DataSet();
            oCommand.Fill(oData, "uds9");
            oTable = oData.Tables["uds9"];
        });

        return oTable;
    }
}
\$\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.