Disposing of objects - is there such a thing as overkill?

Question

I have the following using statement, used specifically for the purpose of archiving content in SharePoint libraries. In this statement my objects are disposed in two places:

finally
{
    web.Dispose();
    site.Dispose();
}

and at the termination of my using statement.

The full using statement is:

using (var site = new SPSite(connectionString))
            {

                SPWeb web = site.OpenWeb(); // open the site
                Console.WriteLine("[{0}] Opened site: {1}", DateTime.Now.ToShortTimeString(), web);
                Console.WriteLine("[{0}] Web Relative URL is: {1}", DateTime.Now.ToShortTimeString(), web.ServerRelativeUrl);

                try
                {
                    // Get your source and destination libraries
                    var source = web.GetList(web.ServerRelativeUrl + @"/Approval%20History%20%20Sales1");
                    var destination = web.GetList(web.ServerRelativeUrl + @"/Approval%20History%20%20Sales");

                    Console.WriteLine("[{0}] Source set to: {1}", DateTime.Now.ToShortTimeString(), source);
                    Console.WriteLine("[{0}] Destination set to: {1}", DateTime.Now.ToShortTimeString(), destination);

                    // Get the collection of items to move, use source.GetItems(SPQuery) if you want a subset
                    SPListItemCollection items = source.Items;

                    // Get the root folder of the destination we'll use this to add the files
                    SPFolder folder = web.GetFolder(destination.RootFolder.Url);

                    Console.WriteLine("[{0}] Moving {1} files from {2} to {3} - please wait...", DateTime.Now.ToShortTimeString(),
                                      items.Count, source, destination);

                    // Now to move the files and the metadata
                    foreach (SPListItem item in items)
                    {
                        //Get the file associated with the item
                        SPFile file = item.File;

                        // Create a new file in the destination library with the same properties
                        SPFile newFile = folder.Files.Add(folder.Url + "/" + file.Name, file.OpenBinary(), file.Properties, true);

                        // Optionally copy across the created/modified metadata                        
                        SPListItem newItem = newFile.Item;
                        newItem["Editor"] = item["Editor"];
                        newItem["Modified"] = item["Modified"];
                        newItem["Modified By"] = item["Modified By"];
                        newItem["Author"] = item["Author"];
                        newItem["Created"] = item["Created"];
                        newItem["Created By"] = item["Created By"];

                        // UpdateOverwriteVersion() will preserve the metadata added above. 
                        newItem.UpdateOverwriteVersion();

                        // Delete the original version of the file
                        // todo: make local backup before deleting?
                        file.Delete();
                        fileCount++;
                    }

                    Console.WriteLine("[{0}] Completed moving {1} files to {2}", DateTime.Now.ToShortTimeString(), fileCount,
                                      destination);
                }
                catch (System.IO.FileNotFoundException)
                {
                    Console.WriteLine("[{0}] Unable to set a location. Please check that paths for source and destination libraries are correct and relative to the site collection.", DateTime.Now.ToShortTimeString());
                }
                finally
                {
                    web.Dispose();
                    site.Dispose();
                }
            }

My question is: Is my finally statement here redundant? Is there any reason that even if potentially it is redundant that I should retain it anyway?

Answer 1

Well, one is redundant, Dispose() on the site object is done for you automatically in the context of using. To be idiomatic in C#, you should do the following:

using (var site = new SPSite(connectionString))
using (SPWeb web = site.OpenWeb()) // open the site
{
        try
        {
            Console.WriteLine("[{0}] Opened site: {1}", DateTime.Now.ToShortTimeString(), web);
            Console.WriteLine("[{0}] Web Relative URL is: {1}", DateTime.Now.ToShortTimeString(), web.ServerRelativeUrl);

            // Get your source and destination libraries
            var source = web.GetList(web.ServerRelativeUrl + @"/Approval%20History%20%20Sales1");
            var destination = web.GetList(web.ServerRelativeUrl + @"/Approval%20History%20%20Sales");

            Console.WriteLine("[{0}] Source set to: {1}", DateTime.Now.ToShortTimeString(), source);
            Console.WriteLine("[{0}] Destination set to: {1}", DateTime.Now.ToShortTimeString(), destination);

            // Get the collection of items to move, use source.GetItems(SPQuery) if you want a subset
            SPListItemCollection items = source.Items;

            // Get the root folder of the destination we'll use this to add the files
            SPFolder folder = web.GetFolder(destination.RootFolder.Url);

            Console.WriteLine("[{0}] Moving {1} files from {2} to {3} - please wait...", DateTime.Now.ToShortTimeString(),
                              items.Count, source, destination);

            // Now to move the files and the metadata
            foreach (SPListItem item in items)
            {
                //Get the file associated with the item
                SPFile file = item.File;

                // Create a new file in the destination library with the same properties
                SPFile newFile = folder.Files.Add(folder.Url + "/" + file.Name, file.OpenBinary(), file.Properties, true);

                // Optionally copy across the created/modified metadata                        
                SPListItem newItem = newFile.Item;
                newItem["Editor"] = item["Editor"];
                newItem["Modified"] = item["Modified"];
                newItem["Modified By"] = item["Modified By"];
                newItem["Author"] = item["Author"];
                newItem["Created"] = item["Created"];
                newItem["Created By"] = item["Created By"];

                // UpdateOverwriteVersion() will preserve the metadata added above. 
                newItem.UpdateOverwriteVersion();

                // Delete the original version of the file
                // todo: make local backup before deleting?
                file.Delete();
                fileCount++;
            }

            Console.WriteLine("[{0}] Completed moving {1} files to {2}", DateTime.Now.ToShortTimeString(), fileCount,
                              destination);
        }
        catch (System.IO.FileNotFoundException)
        {
            Console.WriteLine("[{0}] Unable to set a location. Please check that paths for source and destination libraries are correct and relative to the site collection.", DateTime.Now.ToShortTimeString());
        }
}

Answer 2

There is no reason to have to call dispose on site, the using statement takes care of it.

On the same note, the web variable should be wrapped in a using statement too, then the finally block is redundant.

using (var site = new SPSite(connectionString))
{
    using(var web = site.OpenWeb())
    {
       //...
    }
}

Microsoft Help

Answer 3

Prefer to use RAII above finally; you are already doing this with site, so disposing of it in the finally doesn't make much sense. On the other hand, you are not disposing of web that way and I would say that this could leak to (extremely unlikely) errors. For example, if web.ServerRelativeUrl was a property and threw, you would now fail to dispose of web correctly.

Answer 4

It depends on the logic of the Dispose implementation in each class:

If the class can tolerate consequent invocations to its Dispose method, then all is fine and dandy.

If not, then, maybe an exception would be thrown...

In any case, I would avoid invoking a Dispose method more than once - indicates a code smell.