Is it OK to put a database initialization call in a C# constructor? [closed]

Question

I've seen this is various codebases, and wanted to know if this generally frowned upon or not.

For example:

public class MyClass
{
   public int Id;

   public MyClass()
   {
      Id = new Database().GetIdFor(typeof(MyClass));
   }
}

Answer 1

There are several reasons this is not generally considered good design some of which like causing difficult unit testing and difficulty of handling errors have already been mentioned.

The main reason I would choose not to do so is that your object and the data access layer are now very tightly coupled which means that any use of that object outside of it original design requires significant rework. As an example what if you came across an instance where you needed to use that object without any values assigned for instance to persist a new instance of that class? you now either have to overload the constructor and then make sure all of your other logic handles this new case, or inherit and override.

If the object and the data access were decoupled then you could create an instance and then not hydrate it. Or if your have a different project that uses the same entities but uses a different persistence layer then the objects are reusable.

Having said that I have taken the easier path of coupling in projects in the past :)

Answer 2

Well.. I wouldn't. But then again my approach usually involves the class NOT being responsible for retrieving its own data.

Answer 3

You can use the disposable pattern if you refer to a DB connection:

public class MyClass : IDisposable
{
    private Database db;
    private int? _id;

    public MyClass()
    {
        db = new Database();
    }

    public int Id
    {
        get
        {
            if (_id == null) _id = db.GetIdFor(typeof(MyClass));
            return _id.Value;
        }
    }

    public void Dispose()
    {
        db.Close();
    }
}

Usage:

using (var x = new MyClass()) 
{
    /* ... */

} //closes DB by calling IDisposable.Dispose() when going out of "using" scope