C# overloaded constructors issue

I have the following class that I use to minimise code duplication when often calling different sets of data from an Oracle database. Primarily I need help to remove the code duplication in the overloaded constructor, but any other advice would be appreciated too.

public class UniformData
{
    private string connection = "My Connection String";
    private OracleConnection con;
    private OracleCommand com;
    private OracleDataReader reader;

    public UniformData(string sql)
    {
        con = new OracleConnection(connection);
        con.Open();
        com = new OracleCommand(sql, con);
    }

    public UniformData(string sql, List<SqlParameters> myParams)
    {
        con = new OracleConnection(connection);
        con.Open();
        com = new OracleCommand(sql, con);

        foreach (SqlParameters Param in myParams)
        {
            com.Parameters.Add(Param.ParamName, Param.ParamValue);
        }
    }

    public OracleDataReader GetReader()
    {
        reader = com.ExecuteReader();
        return reader;
    }

    ~UniformData()
    {
        con.Close();
        con.Dispose();
        com.Dispose();
        reader.Close();
        reader.Dispose();
    }
}
Jon Skeet
people
quotationmark

Normally I'd have a "canonical" constructor which all the other constructors chain to. In your case that would involve creating an empty list though:

public UniformData(string sql) : this(sql, new List<SqlParameters>())
{
}

public UniformData(string sql, List<SqlParameters> parameters)
{
    con = new OracleConnection(connection);
    con.Open();
    com = new OracleCommand(sql, con);

    foreach (SqlParameters parameter in parameters)
    {
        com.Parameters.Add(parameter.ParamName, parameter.ParamValue);
    }
}

Alternatively, change the type of the parameter to IEnumerable<SqlParameters> at which point you can use Enumerable.Empty:

public UniformData(string sql) : this(sql, Enumerable.Empty<SqlParameters>())
{
}

public UniformData(string sql, IEnumerable<SqlParameters> parameters)
{
    // Body as before
}

You can split the work the other way, as Mong Zhu's code does - but I tend to find it cleaner to keep all the work in a single place where possible. That makes it easy to validate that you've initialized all your variables properly in all cases - you only need to check that all constructors chain to the canonical one, and that the canonical one initializes everything.

Additionally I would:

  • Make your class implement IDisposable
  • Remove the finalizers

people

See more on this question at Stackoverflow