Why does CodeRush warn me of an Unused Declaration in code?

Question

I have this code, which I use all over my applications to save data back to the database.

    public bool SaveDemo()
    {
        bool success = false;

        try
        {
            using (DataTable dt = DataAccess.ExecuteDataTable("[dbo].[udp_Customers_ups]",
                DataAccess.Parameter(CustomerIdColumn, CustomerId),
                DataAccess.Parameter(CodeColumn, Code),
                DataAccess.Parameter(CompanyColumn, Company),
                DataAccess.Parameter(IsDeletedColumn, IsDeleted),
                DataAccess.Parameter(LastUpdatedColumn, LastUpdated),
                DataAccess.Parameter(UpdatedByColumn, UpdatedBy)))

                success = true;
        }
        catch
        {
            success = false;
        }

        return success;
    }

The code works as is, which by that I mean it saves the data back to the database. However CodeRush complains about the dt being an Unused Declaration. And since the Using is (I think) using the dt I would think that the warning is a false positive. So I am left wondering if CodeRush is wrong or if I am missing something?

I started using the Using block because I have read that is automatically calls the Dispose() method and was a better practice then doing it without it. — Buck Hicks, Apr 04 '15 at 15:40
@WorkSmarter This is not the case - the *declaration* is unused (OP's response is correct) — Ruben Bartelink, Apr 04 '15 at 15:51

score 2 · Accepted Answer · edited Apr 04 '15 at 16:51

2

What CR is trying to say is that in:

using (DataTable dt =  DataAccess.ExecuteDataTable ...

you are not using the declaration of dt; the variable remains untouched after.

The refactor button will transform this to

using ( DataAccess.ExecuteDataTable ...

i.e. it will still be a using statement but you won't have a variable to refer to it.

While you're doing that, you can do some Inline Result transformations, yielding:

    try
    {
        using (DataAccess.ExecuteDataTable("[dbo].[udp_Customers_ups]",
            DataAccess.Parameter(CustomerIdColumn, CustomerId),
            DataAccess.Parameter(CodeColumn, Code),
            DataAccess.Parameter(CompanyColumn, Company),
            DataAccess.Parameter(IsDeletedColumn, IsDeleted),
            DataAccess.Parameter(LastUpdatedColumn, LastUpdated),
            DataAccess.Parameter(UpdatedByColumn, UpdatedBy)))
            return true;
    }
    catch
    {
        return false;
    }

I'll let others question whether wrapping calls like this in a catch block is a good idea...

edited Apr 04 '15 at 16:51

WorkSmarter

3,738
3
29
34

answered Apr 04 '15 at 15:48

Ruben Bartelink

59,778
26
187
249

Since you bring up the Try Catch as possibly a bad practice, Let me ask this. If I get rid of it can I assume that a successful using (DataAccess.ExecuteDataTable will allways hit the true and an unsuccessfull one will not? If that is the case I can remove it and still get the results I am trying to get. – Buck Hicks Apr 04 '15 at 16:15
@BukHix Not 100% what you mean. If the `DataAccess....` call throws 1) the Dispose will happen 2) execution will not run any statements after. The one thing you'll need to do is supply a statement to be the body of the using block. Because an empty statement (`;`) will trigger a compiler warning, the next best thing is an empty block:- `{ }`. Can you calrify what you are asking? My complaint re a catch is that you should only catch at top level and log etc. rather than sweeping it under the carpet deep inside your logic – Ruben Bartelink Apr 04 '15 at 17:52
I think I am going to ask this in a new question because I really want to understand what this code does and don't want to make any assumptions about it. – Buck Hicks Apr 04 '15 at 17:58

Why does CodeRush warn me of an Unused Declaration in code?

1 Answers1

Linked