Ensuring specific columns in an ExcelWorksheet format as ShortDate

For a while, I have worked with the Nuget Epplus package to export MVC models to fresh Excel documents. While the code I show below works correctly, I have a feeling that either I can make this code better or perhaps that there is a coding pattern of which I am unaware that could simplify what I am trying to do.

The main issues are that Epplus does not:

• read MVC model annotations (e.g., [DisplayFormat(DataFormatString="{0:d}")])

• format date or time values as dates or times, instead formatting them as integers

To address these issues, I created a class EpplusExcelPackage which has evolved as I have learned more about C#. A recent purchase of ReSharper has helped me to further improve the code.

In order to have the correct output, as you can see from the code shown below, I have to use reflection and several different looping methods to get the model size and properties and format accordingly.

What I'm really hoping is that someone sees something obvious, such as a coding approach or a design pattern that would simplify the process. Any ideas?

Steps:

-
In a Controller ActionResult, create a new instance of a custom class that accepts the IEnumerable to be exported along with other string parameters.

var exportIenumerable = new EpplusExcelPackage(data.AsEnumerable(), 
    /* string params for Excel heading */);
byte[] export = exportIenumerable.ExportToExcel();

-
Use the EpplusExcelPackage custom class to -- along with creating the new Excel document, identify any DateTime fields and format them accordingly. Otherwise, those fields would export as integers. Here is the full code, as there will inevitably be questions about the code if I were to show only the highlights.

That being said, I believe the important area is this method. I have to use reflection to loop through the IEnumerable in order to identify the DateTime columns and format appropriately.

```
// Without this, DateTimes are formatted as integers
private void For

Readability

private void FormatDateTimeAsDate(ExcelWorksheet worksheet)
{
  for (var j = 0; j < _modelProperties.Length; j++)
  {
      if (_modelProperties[j].PropertyType == _typeNullDatetime
              && DateTimeFormatInfo.CurrentInfo != null)
          worksheet.Column(j + ColumnStart).Style.Numberformat.Format =
              DateTimeFormatInfo.CurrentInfo.ShortDatePattern;

      if (_modelProperties[j].PropertyType != _typeDateTime) continue;
      if (DateTimeFormatInfo.CurrentInfo != null)
          worksheet.Column(j + ColumnStart).Style.Numberformat.Format =
              DateTimeFormatInfo.CurrentInfo.ShortDatePattern;
  }
}

Because you ommited the {} for each if it super hard to understand your code not mentioning is very error prone.

You should always use {}.

This for example is terrible:

private int GetRowCount()
{
  var count = 0;
  IEnumerator enumerator;
  if (_dataToExport == null) return count;
  using (enumerator = _dataToExport.GetEnumerator())
      while (enumerator.MoveNext())
          count++;
  return count;
}

for variable

We usually start with the i. When I see a j I immediately think it's the second nested loop just to discover it's the only loop. You can also use more meaningful names like row, x or column but the default first index is normally an i.

DRY

Don't Repeat Yourself.

In the above method you do the same thing twice. Try to merge the conditions and give them a meaningful name rather then putting a long expression into an if:

private void FormatDateTimeAsDate(ExcelWorksheet worksheet)
{
    for (var i = 0; i < _modelProperties.Length; i++)
    {
        var canSetFormat = 
            DateTimeFormatInfo.CurrentInfo != null &&
            (_modelProperties[i].PropertyType == _typeNullDatetime || _modelProperties[i].PropertyType == _typeDateTime);

        if (canSetFormat) 
        {
            worksheet.Column(i + ColumnStart).Style.Numberformat.Format = DateTimeFormatInfo.CurrentInfo.ShortDatePattern;
        }               
    }
}

(I hope I got the condition right)

By the way, I doubt this DateTimeFormatInfo.CurrentInfo is ever null so no need to check this.

ctor

public EpplusExcelPackage(IEnumerable dataToExport, string reportName, string userName)
{
  _dataToExport = dataToExport;
  _reportName = reportName;
  _userName = userName;
  _rowCount = GetRowCount();
}

If you checked the parameters here and prevented nulls, you'd save a lot of work later where check them for nulls anyway.

IEnumerable

private int GetRowCount()
{
  var count = 0;
  IEnumerator enumerator;
  if (_dataToExport == null) return count;
  using (enumerator = _dataToExport.GetEnumerator())
      while (enumerator.MoveNext())
          count++;
  return count;
}

You shouldn't check the _dataToExport for null. You should prevent it to be a null at all and check the parameter passed to the constructor.

If you however still want to do it then consider this:

private int GetRowCount()
{
    return _dataToExport == null ? -1 : _dataToExport.Count();  
}

There is convention that collections/ienumerables shouldn't be null but rather emtpy.

-1 usually indicates there is something not right like when the collection is null in this case. saying the number of items is 0 let me thing the collection exists. Compare to String.IndexOf.

StackExchange Code Review Q#139569, answer score: 3