Implementing IDisposable for an Excel class

Question

I am developing a class (in a C# MVC 5 project) that originally had a using block within a single method, and it got to be very huge after placing formatting statements and other code in the using block.

I am using the Nuget Epplus package to export a model's contents to an Excel document, and all of the tutorials I can find place the ExcelPackage variable within a using block.

using (ExcelProject excelProject = new ExcelProject())
{
   // lots of code happens nere, making it bloated and ugly
}

In order to reorganize the class and make the ExportToExcel method truly that -- something that only outputs a byte[] array and doesn't contain statements that can be moved into other methods -- I took the excelProject variable out of the using block and made it a class-level variable. That's when Visual Studio code analysis told me I needed to implement IDisposable.

warning CA1001: Microsoft.Design : Implement IDisposable on 'EpplusExcelPackage' because it creates members of the following IDisposable types: 'ExcelPackage'.

I have encountered programmers who tell me things like, "Warnings are meant to be ignored! Turn them off!" I disagree with that and am making the time to address this warning.

I combed through the available IDisposable tutorials and found inspiration for the three methods at the very bottom of the class: public void Dispose(), protected virtual void Dispose(bool disposing), and ~EpplusExcelPackage().

Am I correctly implementing IDisposable in this class?

using OfficeOpenXml;
using System;
using System.Collections.Generic;
using System.Reflection;

// uses Nuget Epplus package

namespace WebApplication1.CentralFunctions
{
    public class EpplusExcelPackage<T> : IDisposable
    {
        private IEnumerable<T> data;
        private byte[] response;
        private string worksheetTitle;
        private ExcelPackage excelPackage = null;
        private ExcelWorksheet worksheet;

        public EpplusExcelPackage(IEnumerable<T> Data)
        {
            this.data = Data;
            this.worksheetTitle = null;
            excelPackage = new ExcelPackage();
            CreateAndLoadWorksheet();
        }

        public EpplusExcelPackage(IEnumerable<T> Data, string WorksheetTitle)
        {
            this.data = Data;
            this.worksheetTitle = WorksheetTitle;
            excelPackage = new ExcelPackage();
            CreateAndLoadWorksheet();
        }

        public void CreateAndLoadWorksheet()
        {
            excelPackage.Workbook.Worksheets.Add("Sheet1");
            worksheet = excelPackage.Workbook.Worksheets[1];
            worksheet.Name = "Sheet1";
            worksheet.Cells[1, 1].LoadFromCollection(data, true);
        }

        public void IdentifyDataTypes()
        {
            // Gather type info of T properties.
            PropertyInfo[] dataPropertiesInfos = typeof(T).GetProperties();

            int i = 1;
            foreach (System.Reflection.PropertyInfo dataPropertyInfo in dataPropertiesInfos)
            {
                if (dataPropertyInfo.PropertyType.ToString().Contains("[System.DateTime]"))
                {
                    worksheet.Column(i).Style.Numberformat.Format = System.Globalization.DateTimeFormatInfo.CurrentInfo.ShortDatePattern;
                }
                i++;
            }
        }

        public byte[] ExportToExcel()
        {
            response = excelPackage.GetAsByteArray();

            return response;
        }

        /* Inspired by http://www.codeproject.com/Articles/413887/Understanding-and-Implementing-IDisposable-Interfa */


        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }

        protected virtual void Dispose(bool disposing)
        {
            if (disposing == true)
            {
                if (excelPackage != null)
                    excelPackage.Dispose();
            }
            else
            {
                // The example said nothing goes here.
            }
        }

        ~EpplusExcelPackage()
        {
            Dispose(false);
        }
    }
}

Answer 1

Almost.

From the MSDN docs on the Disposable Pattern:

✓ DO allow the Dispose(bool) method to be called more than once. The method might choose to do nothing after the first call.

This is typically done by introducing a private boolean member variable and a guard clause.

    private bool disposed;

    protected virtual void Dispose(bool disposing)
    {
        if (disposed) 
            return;

        if (disposing == true)
        {
            if (excelPackage != null)
                excelPackage.Dispose();
        }
        else
        {
            // The example said nothing goes here.
        }

        disposed = true;
    }

And now you have it. Of course, no harm would have been done without the guard clause in your particular case, but I find it good to just implement the same boiler plate every time. It kind of burns into your brain after a while.

This allows us to follow another bullet point from the doc.

✓ DO throw an ObjectDisposedException from any member that cannot be used after the object has been disposed of.

So, in any public member of your class that uses excelPackage, you should check the new disposed field and throw accordingly. For example:

    public byte[] ExportToExcel()
    {
        if (disposed)
        {
            throw new ObjectDisposedException();
        }

        response = excelPackage.GetAsByteArray();

        return response;
    }

---

Some other notes:

• if (disposing == true) is exactly equivalent to if( disposing ).

• Why, why, why is private byte[] response; available to your whole class?!

  It's only used in one method, so declare it there and reduce its scope.

• Do you want null reference exceptions? Because this is how you get null reference exceptions. (Sorry, Archer reference...)

          foreach (System.Reflection.PropertyInfo dataPropertyInfo in dataPropertiesInfos)
      {
          if (dataPropertyInfo.PropertyType.ToString().Contains("[System.DateTime]"))
  

  We have no idea if dataPropertyInfo.PropertyType has a reference when you call ToString() on it. Okay, so given the context, it's probably fine. I do expect it to have a reference, but this pattern will bite you eventually if you blindly call ToString() on things without null checking them first.

  Also, I'm not very good at reflection in .Net, but I suspect there's a better way to check the property type than by casting it to a string and then running Contains(...) on it. I'd say some research is due on how to do this more cleanly.

• Your constructors have a lot of duplication.

       public EpplusExcelPackage(IEnumerable<T> Data)
      {
          this.data = Data;
          this.worksheetTitle = null;
          excelPackage = new ExcelPackage();
          CreateAndLoadWorksheet();
      }
  
       public EpplusExcelPackage(IEnumerable<T> Data, string WorksheetTitle)
      {
          this.data = Data;
          this.worksheetTitle = WorksheetTitle;
          excelPackage = new ExcelPackage();
          CreateAndLoadWorksheet();
      }
  

  You can clean this up by chaining them together.

      public EpplusExcelPackage(IEnumerable<T> Data)
          :this(Data, null)
      {
      }
  
      public EpplusExcelPackage(IEnumerable<T> Data, string WorksheetTitle)
      {
          this.data = Data;
          this.worksheetTitle = WorksheetTitle;
          excelPackage = new ExcelPackage();
          CreateAndLoadWorksheet();
      }
  

  Which in turn, highlights that you're setting the worksheet title to null instead of giving it a reasonable default value.

---

In summary, this is a lot of work and complexity to add to your project. As you can see, it's a bit difficult to get the Disposable pattern right. Weigh your options here. This is really only worth it if you're calling this code a lot.

Every time I've ever found myself thinking that I need to implement IDisposable, I was wrong. Each and every time I was able to create a cohesive class that avoided keeping disposable objects as members. Instead, I found ways to kept each call to the disposable object short lived. I challenge you to see if you really need the excelPackage to live for the lifetime of this class. I would try to keep it inside of a using block in the method you'd like to call.

Answer 2

ExcelPackage is a managed resource so you don't need any of that Dispose(bool disposing) or CG.SupressFinalize(this) hogwash. Simply call excelPackage.Dispose() once you are done using it and call it a day (after all that's what the using statement does too).

Only implement the IDisposable interface if you really need to move the disposing responsibility to the user of the class (you probably don't) and if you do your Dispose() method should simply be

public void Dispose()
{
    excelPackage.Dispose()
}

(You don't even need to check the reference for null first since you initialize it from the constructor so it should never be null and if it is you have a bug that needs to be fixed rather than swallowed.)

All that disposing boilerplate code is only needed if your class holds a reference to an unmanaged resource and in that case managing(!) that unmanaged resource should be the sole responsibility of the class.

Never should your class reference multiple unmanaged resources and never ever should it reference both managed and unmanaged resources.

Answer 3

I would like to thank everyone for their helpful comments and suggestions. I placed my ExcelPackage code back within the using block -- IDispose seems too complicated to me at this time, so I'm going to opt for the simpler option.

As for one of my earlier concerns about the using block blowing up with lots of code -- I haven't yet developed future code that the customers haven't yet determined -- I think I worked out a better approach for organizing and allowing for future growth: extracting blocks of code into their own void functions and calling them from within the using block.

The only public code other than the constructor is a public byte[] method. This is the Excel document that is sent back to the MVC website controller for the user to save where they choose.

I also chanced into a better means of type-checking for datetime properties when using Reflection: if (dataPropertyInfo.PropertyType == typeof(DateTime?)).

Here is the approach that I am going with for now.

using OfficeOpenXml;
using System;
using System.Collections.Generic;
using System.Globalization;
using System.Reflection;

// uses Nuget Epplus package

// User uses class constructor and the class does the rest
// 1: Add report information to document
// 2: Identify datetime columns and format column in Excel document as shortdate
// 3: TODO: Apply chosen theme (where 0 = plain format)
// 4: Export byte[] to the controller to be saved as a file

namespace WebApplication1.CentralFunctions
{
    public class EpplusExcelPackage<T>
    {
        private IEnumerable<T> data;
        private string reportName;
        private int chosenTheme;

        public EpplusExcelPackage(IEnumerable<T> Data, string ReportName, int ChosenTheme)
        {
            this.data = Data;
            this.reportName = ReportName;
            this.chosenTheme = ChosenTheme;

            // All ExcelPackage code is placed inside a "using block".
            // Formatting, reflection type-checking, and other future code
            // are placed in separate functions that are called from the using block
        }

        private void IdentifyDataTypes(ExcelWorksheet worksheet)
        {
            // Gather type info of T properties.
            PropertyInfo[] dataPropertiesInfos = typeof(T).GetProperties();

            // from: http://stackoverflow.com/questions/1180730/c-sharp-determine-a-nullable-property-datetime-type-when-using-reflection

            int i = 1;
            foreach (System.Reflection.PropertyInfo dataPropertyInfo in dataPropertiesInfos)
            {
                if (dataPropertyInfo.PropertyType == typeof(DateTime?))  // field is nullable in model; 
                {
                    worksheet.Column(i).Style.Numberformat.Format = DateTimeFormatInfo.CurrentInfo.ShortDatePattern;
                }
                i++;
            }
        }

        // byte[] sent to controller for user to save as a file
        public byte[] ExportToExcel()
        {
            using (ExcelPackage excelPackage = new ExcelPackage())
            {
                // create worksheet
                excelPackage.Workbook.Worksheets.Add("Sheet1");
                ExcelWorksheet worksheet = excelPackage.Workbook.Worksheets[1];
                worksheet.Name = "Sheet1";

                // add header information
                worksheet.Cells[1, 1].Value = "Report Name";
                worksheet.Cells[1, 2].Value = reportName;
                worksheet.Cells[2, 1].Value = "Timestamp";
                worksheet.Cells[2, 2].Value = DateTime.Now.ToString();

                // load data from collection
                worksheet.Cells[5, 1].LoadFromCollection(data, true);

                // Identify datetime columns in model and format accordingly
                IdentifyDataTypes(worksheet);

                // TODO: apply chosen theme               

                return excelPackage.GetAsByteArray();
            }

        }
    }
}