Implementing IDisposable for an Excel class

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 : IDisposable
{
private IEnumerable data;
private byte[] response;
private string worksheetTitle;
private ExcelPackage excelPackage = null;
private ExcelWorksheet worksheet;

public EpplusExcelPackage(IEn

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 Data)
    {
        this.data = Data;
        this.worksheetTitle = null;
        excelPackage = new ExcelPackage();
        CreateAndLoadWorksheet();
    }

     public EpplusExcelPackage(IEnumerable 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 Data)
        :this(Data, null)
    {
    }

    public EpplusExcelPackage(IEnumerable 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.

StackExchange Code Review Q#115159, answer score: 10