C# overloaded constructors issue

Question

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();
    }
}

You shouldn't have a finalizer for this class. Finalizers are for cleaning up *unmanaged* resources, not for disposing of managed resources. You should implement `IDisposable` and dispose of the composed objects in the `Dispose` method and not have any finalizer. — Servy, Jan 04 '17 at 15:09

score 9 · Answer 1 · answered Jan 04 '17 at 15:12

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

score 6 · Answer 2 · edited May 23 '17 at 11:52

6

you can call the simpler constructor from the more complex one using this(parameter)

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

public UniformData(string sql, List<SqlParameters> myParams): this(sql)
{
    foreach (SqlParameters Param in myParams)
    {
        com.Parameters.Add(Param.ParamName, Param.ParamValue);
    }
}

The original post is 7 years old, may be you missed it when researching.

The Using Constructors (C# Programming Guide) might yield further helpful information including my answer

edited May 23 '17 at 11:52

Community

1
1

answered Jan 04 '17 at 15:09

Mong Zhu

23,309
10
44
76

I think it is more preferable to use the reverse way when you define the constructor with the biggest count of parameters and use it in other overloaded constructors via `this` – Pavel the coder Jan 04 '17 at 15:17
1

@Simon you are welcome. I would consider the advise of Jon Skeet and Servy to implement IDisposable – Mong Zhu Jan 04 '17 at 15:25
Will do. I'm still learning this stuff though. If I make the class IDisposable and put the code from the destructor into a Dispose method will that be enough? Will this method run automatically after the class is no longer being used or will I have to formally call it from my code whenever I've finished with the class? – Simon Jan 04 '17 at 15:37
1

@Simon you could use a [using block](https://msdn.microsoft.com/en-us/library/yh598w02.aspx) and the Dispose method will be called automatically at the end. [using-block-vs-idisposabe-dispose](http://stackoverflow.com/questions/10984336/using-block-vs-idisposabe-dispose) elaborates on the pros and contras – Mong Zhu Jan 04 '17 at 15:41

score -1 · Answer 3 · answered Jan 04 '17 at 15:08

Simply have one of the constructors call the other. Either have the overload without the parameters call the overload with the parameters, but passing an empty list, or have the overload with parameters call the overload without parameters to initialize the connection, so that it only needs to add the parameters.

C# overloaded constructors issue

3 Answers3