4

I've been rewriting some old code so that the using statement is used for my DataTables, instead of remembering to Dispose each time:

using (DataTable dt = BLL.GetDataTable()) {
   foreach(DataRow dr in dt.Rows) {
     // iteration logic
   }
}

However in one particular case, the DataTable content differs based on a variable, so I create the initial DataTable and then assign the value afterwards:

DataTable dt = new DataTable();
switch(foo) {
  case bar:
     dt = BLL.GetDataTable(bar);
     break;
  default:
     dt = BLL.GetDataTable();
     break;
}
// iteration logic here
dt.Dispose();

Changing this to use using, I have:

using (DataTable dt = new DataTable()) {
    switch(foo) {
      case bar:
         dt = BLL.GetDataTable(bar);
         break;
      default:
         dt = BLL.GetDataTable();
         break;
    }
    // iteration logic here
}

Is that good practice (i.e. creating the empty DataTable with a using statement)? I don't know why but it doesn't feel quite right.

EvilDr
  • 8,943
  • 14
  • 73
  • 133
  • Your last example shouldn't compile and you should get the following error: "Cannot assign to 'dt' because it is a 'using variable'" – ProgrammingLlama May 29 '19 at 08:47
  • @John I was testing the same (checking which dispose was called). Indeed, you cannot assign a variable used in a using – Jeroen van Langen May 29 '19 at 08:48
  • It is best practice to use ```using()``` on all disposable objects. As @John stated, your third example will not work. But you can assign your cases to a ```func<>``` and use this in your ```using()```. – Rabban May 29 '19 at 08:49
  • Thank you all kindly. @Rabban could you please post a `func<>` example for my understanding? – EvilDr May 29 '19 at 08:57

3 Answers3

5

As I stated in the comments, your last example won't work. If you want to do something like that, you could move the DataTable generation into a separate function:

public DataTable GetBLLDataTable()
{
    switch(foo)
    {
        case bar:
            return BLL.GetDataTable(bar);
            break;
        default:
            return BLL.GetDataTable();
            break;
    }
}

And then use the DataTable returned by this method in your using statement:

using (DataTable dt = GetBLLDataTable()) {
    // iteration logic here
}
ProgrammingLlama
  • 36,677
  • 7
  • 67
  • 86
4

In your last example, you are only disposing the first DataTable object and not the other that gets assigned.

The using statement is just syntactic sugar for try/finally. You can instead write your last example like:

DataTable dt;
try
{
    switch (foo)
    {
        case bar:
            dt = BLL.GetDataTable(bar);
            break;
        default:
            dt = BLL.GetDataTable();
            break;
    }
}
finally
{
    dt?.Dispose();
}

This will ensure your IDisposable object is always disposed. It is a bit of a weird example in this case though since I don't see why you'd assign a DataTable in the switch and then immediately dispose it.

Owen Pauling
  • 11,349
  • 20
  • 53
  • 64
  • 1
    if BLL.GetDataTable(bar) throw an exception - dt will be null. and dt.Dispose(); - throw one more exception( "Object reference not set to an instance of an object") – Basil Kosovan May 29 '19 at 08:54
  • 1
    @Basil But, that will be the same issue with your code once OP calls off to their code to generate the data table in `BLL`. – ProgrammingLlama May 29 '19 at 08:58
  • Could you please clarify your first sentence *"...not the other that gets assigned"*? I had assumed that because there's only a single variable, that calling Dispose on that would ensure there's nothing left in memory? – EvilDr May 29 '19 at 09:04
  • 2
    @EvilDr Imagine `DataTable a = new DataTable(); a = new DataTable(); a.Dispose();` - You've created two DataTable objects, but you've only disposed one of them. With a `using` statement, .NET does stop you building this kind of thing, as I mentioned in the comments :) Note that unassigning an object doesn't delete it from memory. .NET will eventually collect _managed_ memory, but the `Dispose` pattern is mostly used to get rid of _unmanaged_ resources, that the garbage collector doesn't know about. – ProgrammingLlama May 29 '19 at 09:06
  • Hmm very insightful. I didn't realise unassigned objects were not automatically removed straight away. Thank you. – EvilDr May 29 '19 at 09:18
2

Just another approach but similar to that what John said. You can use a func<> to set your get method and use this in the using()

Func<DataTable> func = null;
switch (foo)
{
    case bar:
        func = () => BLL.GetDataTable(bar);
        break;
    default:
        func = () => BLL.GetDataTable();
        break;
 }

 using (var dt = func())
 {
     // iteration logic here
 }

Personally i would prefer Johns approach, its a bit more readable. But all do the same, so it's up to you to use what you like most.

Rabban
  • 2,451
  • 1
  • 19
  • 21