Is it bad practice to use a using statement on a class level variable?

Question

I have code similar to the following.

class MyController
{
    [ThreadStatic] private DbInterface db;

    public void ImportAllData()
    {
        using (db = new DbInterface())
        {
            var records = PullData();
            PushData(records);
        }
    }

    private DbRecord[] PullData()
    {
        return db.GetFromTableA();
    }

    private void PushData(DbRecord[] records)
    {
        db.InsertIntoTableB(records);
    }
}

The alternative is a lot more cumbersome to maintain.

class MyController
{
    public void ImportAllData()
    {
        using (var db = new DbInterface())
        {
            var records = PullData(db);
            PushData(records, db);
        }
    }

    private DbRecord[] PullData(DbInterface db)
    {
        return db.GetFromTableA();
    }

    private void PushData(DbRecord[] records, DbInterface db)
    {
        db.InsertIntoTableB(records);
    }
}

As far as I can see, my first implementation:

• is thread safe (assuming DbInterface is thread safe),
• prevents any other process from touching the db variable, and
• ensures db will always be disposed, even during an exception.

Is it bad practice to use the using statement on a variable with class scope? Have I missed something?

Answer 1

Personally, I prefer your second option.

The issue with the first design is that your effectively adding unnecessary coupling to the design. Your PullData and PushData methods cannot be used alone - they require that a call to ImportAllData or some other method that will setup and properly cleanup the db variable be called first.

The second option, while slightly more code (though not much), makes the intent very clear for each and every method. Each method knows that it needs to work on an external DbInterface instance passed into it. There is little or no chance of this being misused in the future.

Answer 2

Your first variant exposes db outside of the scope where it is managed by the using block. That opens the possibility of unintended side effects. For example, another method might use or even dispose of db. That could happen if you or a later maintainer forget the implicit contract for db or even through a typo in code.

I would not use the first variant.