To Dispose or not to Dispose (CA2000)

I'm switching on Code Analysis on an older project. Most remarks that result I can understand, but the CA2000: Dispose objects before losing scope is hard to get right.

For instance, this code from an ASP.Net page:

private void BuildTable()
{
    HtmlTableRow tr = new HtmlTableRow();
    HtmlTable开发者_Go百科Cell td = new HtmlTableCell();

    tr.Cells.Add(td);
    // add some controls to 'td'

    theTable.Rows.Insert(0, tr);
    // 'theTable' is an HtmlTable control on the page
}

Gives CA messages:

CA2000 : Microsoft.Reliability : In method 'BuildTable()', call System.IDisposable.Dispose on object 'tr' before all references to it are out of scope.

CA2000 : Microsoft.Reliability : In method 'BuildTable()', object 'td' is not disposed along all exception paths. Call System.IDisposable.Dispose on object 'td' before all references to it are out of scope. (and similar messages about the controls that are added to that 'td'.)

I can resolve the second problem:

private void BuildTable()
{
    HtmlTableRow tr = new HtmlTableRow();
    HtmlTableCell td = new HtmlTableCell();

    try
    {
        tr.Cells.Add(td);
        // add some controls to 'td'

        td = null; // this line is only reached when there were no exceptions
    }
    finally
    {
        // only dispose if there were problems ('exception path')
        if (td != null) td.Dispose();
    }

    theTable.Rows.Insert(0, tr);
}

But I don't think it is possible to resolve the message about the 'tr'. I can't Dispose of that, because it's still needed after the method has exited.

Or did I miss something?

By the way: changing that theTable.Rows.Insert into theTable.Rows.Add changes the CA message to 'not disposed along all exception paths'

The code analysis is unable to completely understand your code and simply warns if you create a disposable object that seems to not be disposed. In your case you should turn off the warning because the object should not be disposed before leaving the method. You can turn warnings off either for the entire project by customizing the code analysis rule set or on each method having this warning where it is obvious that the code analysis is wrong.

That said, I recommend that you use the using construct when dealing with IDisposable objects:

using (var tr = new HtmlTableRow()) {
  using (var td = new HtmlTableCell()) {
    tr.Cells.Add(td);
    theTable.Rows.Insert(0, tr);
  }
}

Except this code is nonsense as you don't want to dispose the row and cell you just added to the table.

I think you have just shown that the CA2000 rule is not very useful on most code bases As far as I know,

• Dispose on HtmlTableRow does nothing useful unless it is being used inside of a UI designer; I have never seen anyone call dispose on the Asp.net controls. (Winforms/WPF is a different case)
• You store the reference to td inside the table, so you should not dispose it anyway.

As both of the above is very common in normal code, I don’t see the CA2000 rule to be of value to most code bases – there are so many false positives you are very likely to miss in 1 in 50 cases when it is a real problem.

this code will get rid of both warnings (I use a using(HtmlTable) to simulate your global HtmlTable member...):

using (HtmlTable theTable = new HtmlTable())
{
    HtmlTableRow tr = null;
    try
    {
        HtmlTableCell td = null;

        try
        {
            td = new HtmlTableCell();

            // add some controls to 'td'


            tr = new HtmlTableRow();
            tr.Cells.Add(td);

            /* td will now be disposed by tr.Dispose() */
            td = null;
        }
        finally
        {
            if (td != null)
            {
                td.Dispose();
                td = null;
            }
        }

        theTable.Rows.Insert(0, tr);

        /* tr will now be disposed by theTable.Dispose() */
        tr = null;
    }
    finally
    {
        if (tr != null)
        {
            tr.Dispose();
            tr = null;
        }
    }
}

but I think you will consider using an approach that uses subfunctions to make the code more clear:

    private static void createTable()
    {
        using (HtmlTable theTable = new HtmlTable())
        {
            createRows(theTable);
        }
    }

    private static void createRows(HtmlTable theTable)
    {
        HtmlTableRow tr = null;
        try
        {
            tr = new HtmlTableRow();
            createCells(tr);

            theTable.Rows.Insert(0, tr);

            /* tr will now be disposed by theTable.Dispose() */
            tr = null;
        }
        finally
        {
            if (tr != null)
            {
                tr.Dispose();
                tr = null;
            }
        }
    }

    private static void createCells(HtmlTableRow tr)
    {
        HtmlTableCell td = null;

        try
        {
            td = new HtmlTableCell();

            // add some controls to 'td'


            tr.Cells.Add(td);

            /* td will now be disposed by tr.Dispose() */
            td = null;
        }
        finally
        {
            if (td != null)
            {
                td.Dispose();
                td = null;
            }
        }
    }

Add the control to the collection directly after creating it, but before you do anything with the control.

HtmlTableRow tr = new HtmlTableRow();
theTable.Rows.Insert(0, tr);

HtmlTableCell td = new HtmlTableCell();
tr.Cells.Add(td);

// add some controls to 'td'

As there can't be an exception between creating and adding/inserting the control to the collection there is no need for try/catch. When an exception occurs after the control is added to the collection the page will dispose of it. You won't get the CA2000 this way.

In case you think that Code Analysis is wrong (happened to me that it asked to call Dispose for an object that didn't implement IDisposable) or you don't feel it is necessary to Dispose that object, you can always suppress that message like this.

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2000: DisposeObjectsBeforeLosingScope")]
public static IDataReader RetrieveData(string conn, string sql)
{
    SqlConnection connection = new SqlConnection(conn);
    SqlCommand command = new SqlCommand(sql, conn);
    return command.ExecuteReader(CommandBehavior.CloseConnection);
    //Oops, I forgot to dispose of the command, and now I don't get warned about that.
}