A question of style/readability regarding the C# "using" statement

Question

I'd like to know your opinion on a matter of coding style that I'm on the fence about. I realize there probably isn't a definitive answer, but I'd like to see if there is a strong preference in one direction or the other.

I'm going through a solution adding using statements in quite a few places. Often I will come across something like so:

{
    log = new log();
    log.SomeProperty = something;  // several of these
    log.Connection = new OracleConnection("...");
    log.InsertData(); // this is where log.Connection will be used
    ... // do other stuff with log, but connection won't be used again
}

where log.Connection is an OracleConnection, which implements IDisposable.

The neatnik in me wants to change it to:

{
    using (OracleConnection connection = new OracleConnection("..."))
    {
        log = new log();
        log.SomeProperty = something;
        log.Connection = conn;
        log.InsertData();
        ...
    }
}

But the lover of brevity and getting-the-job-done-slightly-faster wants to do:

{
    log = new log();
    log.SomeProperty = something; 
    using (log.Connection = new OracleConnection("..."))
        log.InsertData();
    ...
}

For some reason I feel a bit dirty doing this. Do you consider this bad or not? If you think this is bad, why? If it's good, why?

EDIT: Please note that this is just one (somewhat contrived) example of many. Please don't fixate on the fact that this happens to indicate a logger class with a poorly thought-out interface. This is not relevant to my question, and I'm not at liberty to improve the classes themselves anyway.

Answer 1

They are both horrid. Do neither of them.

You're making what I call a "high maintenance class" here. The high maintenance class has a contract that says "I require you to give me a bunch of resources, and you're required to know when I'm done with them and clean them up appropriately". This contract means that the user of the class has to know how the class is implemented, thereby violating the principle of encapsulation and abstraction that motivated making a class in the first place.

You can tell this by your comment: this is where the connection is used, I know the connection will not be used again. How do you know that? You only know that if that is the documented contract of the class. That's not a good contract to impose upon the consumer of a class.

Some ways to make this better:

1) make the logger disposable. Have it clean up the connection when it is done. The down side of this is that the logger holds on to the connection for longer than necessary.

2) make InsertData take the connection as a parameter. The caller can still be responsible for cleaning up the connection because the logger does not hold onto it.

3) make a third class "Inserter" which is disposable and takes a log and a connection in its constructor. The inserter disposes of the connection when it is disposed; the caller then is responsible for disposing the inserter.

Answer 2

I agree that ideally log itself should implement IDisposable, but let's assume that's not possible and address the question the OP actually asked.

The second way is better, simply because it's less code that accomplishes the same thing. There is no advantage to introducing an additional connection variable here.

Also note that you could do other initialisation outside of the using block. It won't matter here, but may matter if you're "using" some really expensive resource. That is:

log = new log();
log.SomeProperty = something; // This can be outside the "using"
using (OracleConnection connection = new OracleConnection("..."))
{
    log.Connection = conn;
    log.InsertData();
    ...
}