CODEWITHSUNDEEP
c#idisposable
ezpresso
ezpresso

Reputation: 8126

Dealing with exceptions in constructor when implementing IDisposable

I read I need to implement IDisposable if my class has a member variable that is itself IDisposable. Well, I am implementing IDisposable interface because my class contains a database object (db class member below) which is IDisposable itself:

    public class CommissionModel : IDisposable
    {
        protected PetaPoco.Database db;

        public CommissionModel()
        {
            string connectionString = "Server=localhost;...";

            // The line below may throw an exception (!!!)
            db = new PetaPoco.Database(connectionString, "mysql");              
        }

        // Automatically close database connection
        public void Dispose()
        {
            if (db != null)
                db.Dispose();

            db = null;
        }

        public void InsertRecord(Record somerecord)
        {
            // ...
            db.Insert(somerecord);
        }

The problem is instantiation of db member may fail under some circumstances. What should I do when the exception is thrown in the constructor and the database object gets not created? Should I rethrow the exception or maybe check if db != null in InsertRecord method?

Upvotes: 8

Views: 1505

Answers (5)

supercat
supercat

Reputation: 81169

If there is no way that an exception can occur(*) between the time the constructor acquires a resource and the time it gets returned to the application, there's nothing to worry about.

If it's possible than an exception might occur in such circumstances, I would suggest a pattern like:

 public class CommissionModel : IDisposable
    {
        protected PetaPoco.Database db;
        protected OtherResourceType resource2;

        public CommissionModel()
        {
            Boolean ok;
            string connectionString = "Server=localhost;...";

            ok = false;
            try
            {
                // Either of the next two lines may throw an exception
                db = new PetaPoco.Database(connectionString, "mysql");
                resource2 = new OtherResourceType();
                // Once we make it this far, we should be successful
                ok = true;
            }
            finally
            {
                if (!ok)
                  Dispose();
            }
        }

        // Automatically close database connection
        public void Dispose()
        {
            Zap(ref db);  // Method to Dispose and null out, only if not null
            Zap(ref resource2);
        }
    }

(*) It's almost always possible for a ThreadAbortException to occur if some mean nasty ogre calls Tread.Abort on your code, but there's really nothing to be done about that in any case.

Upvotes: 1

Henk Holterman
Henk Holterman

Reputation: 273264

It looks like you don't have to do anything.

If your construction process is interrupted the resource is not created. In most usage scenarios (this lays outside your class), the Dispose() will never be called. But even when it is, your if (db != null) code is enough of a guard.

A few minor points:

  • the db = null; inside Dispose() is pointless.
  • InsertRecord() should start by checking if (db != null)

Upvotes: 2

Dan Bryant
Dan Bryant

Reputation: 27505

If your constructor throws an exception, the consumer of your code will never receive a reference to an instance of your class. The partially initialized class memory will eventually be collected and Dispose will not be called.

I would generally recommend moving an operation that can fail due to outside circumstances (and not due to misusage, like an invalid parameter value) to a separate method, like "Connect".

Upvotes: 4

Andy Christianson
Andy Christianson

Reputation: 371

Ideally you should not be doing any "work" in the constructor. Work in the constructor should really be linking up object references. This would allow you to unit test this class by hooking up mock classes instead.

Try this instead:

public CommissionModel(PetaPoco.Database db) {
    this.db = db;
}

Upvotes: 2

Novus
Novus

Reputation: 197

You should only catch exceptions that you can handle. From the code, it appears that initializing the variable db may show that there is a problem communicating with your database server.

My suggestion is to leave your code as is and handle exceptions in a central location such as the Application_Error event in the Global.asax or the event that is initializing the CommissionModel.

Upvotes: 1

Related Questions