14

Refactoring some code again. Seeing some of this in one of the ASP.NET pages:

using (TextBox txtBox = e.Row.Cells[1].FindControl("txtBox") as TextBox)
{
}

There is no need to dispose txtBox, because it's just a reference to an existing control. And you don't want the control disposed at all. I'm not even sure this isn't harmful - like it would appear to ask for the underlying control to be disposed inappropriately (although I have not yet seen any ill effects from it being used this way).

Cade Roux
  • 88,164
  • 40
  • 182
  • 265
  • 1
    I think the using is not needed in there. – Davide Piras Mar 11 '11 at 16:24
  • 8
    Looks like a rookie mistake on the original dev's part, not fully understanding what `using` does. – squillman Mar 11 '11 at 16:25
  • To quote Chris Griffin, "I don't think you need the usin...Whaaaaaaa!?" – Justin Niessner Mar 11 '11 at 16:25
  • 7
    "Wow, `using` isn't just for namespaces? I'm gonna **use** everything!" – BoltClock Mar 11 '11 at 16:25
  • 1
    @Davide Piras: I believe you are correct there. Looks like someone got excited by Using and is using it everywhere. The underlying object should not be being disposed of as there will still be references to it elsewhere in the app. – Lazarus Mar 11 '11 at 16:29
  • @BoltClock Of course, there are many legitimate (non-namespace) usings in this code - particularly with the database access. I don't think it's in a lot of the forms, because this is the first time I think I've seen it. – Cade Roux Mar 11 '11 at 16:29
  • 1
    I've seen lots of negative side-effects when I used `using` improperly with the `Image` or `Bitmap` class. – Uwe Keim Mar 11 '11 at 16:33
  • 1
    Get rid of the `using` statement, it's doing nothing but causing an eyesore... – code4life Mar 11 '11 at 16:36

6 Answers6

7

TextBox inherits its implementation of IDisposable from its Component superclass. That implementation removes the component from its site container if it has one.

So, doing that might have nefarious effects if the text box actually resides in a site container. Also, after calling Dispose() on an object, you should not use it again, no matter what (it's not in a usable state anymore).

I'd suggest you avoid that pattern with ASP.NET web controls.

Frédéric Hamidi
  • 258,201
  • 41
  • 486
  • 479
4

This is wrong, it shouldnt be used like this. I would imagine there are potential problems using this that wont show up immediately. The textboxes dispose is called upon leaving the using statement but it wont be garbage collected immediately. If it is collected then you will have problems later when you try to access that control.

SecretDeveloper
  • 3,140
  • 2
  • 31
  • 36
4

The TextBox instance could potentially be null if not found, so Dispose() is called a NullReferenceException would be thrown.

I've never seen that pattern in practice, but if you need to use it, it'd be worth handling any potential errors.

James Simm
  • 1,569
  • 1
  • 16
  • 28
  • 5
    The specification for the `using` statement ensures that `Dispose()` won't be called if the value is `null`. – zinglon Mar 11 '11 at 17:21
  • @zinglon Good point, and I think perhaps the original coder thought that putting the using would also mean that the inner code would only be called for non-NULL parameters, too, which is not the case. – Cade Roux Mar 13 '11 at 04:12
2

There should be no negative secondary effects, but it's not necessary either. If we did using (x) { ... } on everything that implements IDisposable in the CLR most C# code would be unreadable.

kprobst
  • 16,165
  • 5
  • 32
  • 53
  • 1
    Since the obejct is in a (potentially) unusable state after calling Dispose there definately potential unwanted secondary effects. – Rune FS Mar 11 '11 at 19:46
2

Actually, here the TextBox instance is accessible only to the context inside the brackets of using statement, maybe that was the main reason of using it.

Tengiz
  • 8,011
  • 30
  • 39
  • Possibly a good use, I guess, but they could always have just put them in a scope with a naked {} scope. There is at least one case in this form where they've put 8 usings (not 8 within one using) nested with brackets. – Cade Roux Mar 11 '11 at 16:33
0

From MSDN:

Within the using block, the object is read-only and cannot be modified or reassigned.

So I guess you can only read the textbox properties, but not change them, inside the using block.

DOK
  • 32,337
  • 7
  • 60
  • 92
  • The properties definitely get changed, including .Text, .Visible and others. – Cade Roux Mar 11 '11 at 16:41
  • 3
    All that means is you can't do `using (TextBox txtBox = ...) { txtBox = new TextBox(); }` The object reference is read-only, but the object itself is not necessarily immutable. – David Yaw Mar 11 '11 at 16:46