4
\$\begingroup\$

I have an Excel file and each row within the sheet represents an employee's data.

I am trying to find a cleaner way to map each row to a model object:

public IList<Employee> ReadEmployees(byte[] file, string companyId)
{
    var employees = new List<Employee>();
    using (var stream = new MemoryStream(file))
    {
        using (var package = new ExcelPackage(stream))
        {
            var worksheet = package.Workbook.Worksheets[0];
            for (var rowNumber = 3; rowNumber <= worksheet.Dimension.End.Row; rowNumber++)
            {
                //---- Trying to retrieve the row data in an elegant way----
                var employee = GetEmployeeRowSummary(worksheet, rowNumber, companyId);
                employees.Add(employee);
            }
        }
    }

    return employees;
}

I was kind of hoping to use something like auto-mapper but don't know if that would be the best way (maybe create a Custom Resolver).

Is there a better way of mapping excel rows to an model? The way I am doing it currently seems like a maintenance issue.

private static Employee GetEmployeeRowSummary(ExcelWorksheet worksheet, int rowNumber, string companyId)
{
    var employeeId = Guid.NewGuid().ToString();

    return new Employee
           {
               CompanyId = companyId,
               EmployeeId = employeeId,
               EmployeeNumber = worksheet.Cells[rowNumber, 1].GetValue<string>(),
               FirstName = worksheet.Cells[rowNumber, 2].GetValue<string>(),
               LastName = worksheet.Cells[rowNumber, 3].GetValue<string>(),
               BirthDate = worksheet.Cells[rowNumber, 4].GetValue<DateTime>(),
               AppointmentDate = worksheet.Cells[rowNumber, 5].GetValue<DateTime>(),
               CompanyRunFrequencyId = 9,
               JobTitle = worksheet.Cells[rowNumber, 8].GetValue<string>(),
               DateCreated = DateTime.Now,
               EmployeeAddress = new List<EmployeeAddress>
                                 {
                                    new EmployeeAddress
                                    {
                                        EmployeeId = employeeId,
                                        StreetNumber = worksheet.Cells[rowNumber, 9].GetValue<int>(),
                                        Suburb = worksheet.Cells[rowNumber, 11].GetValue<string>(),
                                        City = worksheet.Cells[rowNumber, 12].GetValue<string>(),
                                        PostalCode = worksheet.Cells[rowNumber, 13].GetValue<int>()
                                    }
                                 }
           };
}

\$\endgroup\$
  • \$\begingroup\$ Is this the code as you currently use it? Maintenance issues aside, does it work to your satisfaction? \$\endgroup\$ – Mast Nov 20 '19 at 13:21
  • 1
    \$\begingroup\$ Yes this is my current code, and working \$\endgroup\$ – R4nc1d Nov 20 '19 at 13:23
3
\$\begingroup\$

The indentation needs to be mentioned. Hopefully it's just an accidental tabs vs spaces issue that caused it to render this way, ...but I'm not seeing the Tab characters, so... the indentation needs to be mentioned, becauase this is not normal:

public IList<Employee> ReadEmployees(byte[] file, string companyId)
        {

Expected:

public IList<Employee> ReadEmployees(byte[] file, string companyId)
{

Just like you have here:

            using (var stream = new MemoryStream(file))
            {

And here:

                    for (var rowNumber = 3; rowNumber <= worksheet.Dimension.End.Row; rowNumber++)
                    {

Now, with that cleared, the GetEmployeeRowSummary method looks much cleaner already:

private static Employee GetEmployeeRowSummary(ExcelWorksheet worksheet, int rowNumber, string companyId)

{
    var employeeId = Guid.NewGuid().ToString();
    return new Employee
    {
        CompanyId = companyId,
        EmployeeId = employeeId,
        EmployeeNumber = worksheet.Cells[rowNumber, 1].GetValue<string>(),
        FirstName = worksheet.Cells[rowNumber, 2].GetValue<string>(),
        LastName = worksheet.Cells[rowNumber, 3].GetValue<string>(),
        BirthDate = worksheet.Cells[rowNumber, 4].GetValue<DateTime>(),
        AppointmentDate = worksheet.Cells[rowNumber, 5].GetValue<DateTime>(),
        CompanyRunFrequencyId = 9,
        JobTitle = worksheet.Cells[rowNumber, 8].GetValue<string>(),
        DateCreated = DateTime.Now,
        EmployeeAddress = new List<EmployeeAddress>
        {
            new EmployeeAddress
            {
                EmployeeId = employeeId,
                StreetNumber = worksheet.Cells[rowNumber, 9].GetValue<int>(),
                Suburb = worksheet.Cells[rowNumber, 11].GetValue<string>(),
                City = worksheet.Cells[rowNumber, 12].GetValue<string>(),
                PostalCode = worksheet.Cells[rowNumber, 13].GetValue<int>()
            }
       }
   };
}

Consistent indentation is fundamental for clean, readable code.

But readability isn't the biggest problem here: the repeated calls to worksheet.Cells and {Range?}.GetValue are.

I'm not sure what ExcelPackage is, but it's definitely not VSTO, so maybe this isn't as much of a problem, but invoking worksheet.Cells[...].GetValue<...>() is spawning a RCW (Runtime Callable Wrapper - a .NET wrapper object to access a COM object) whose reference isn't captured, and thus could be leaking the object reference. If you were using VSTO, there would very likely be a "ghost" EXCEL.EXE process lingering in Task Manager forever, well after your application happily terminated.

In any case, working directly with worksheet cells is THE slowest thing you can do, whether in VBA or C# through COM interop, whether it be through VSTO or any other library.

Again I don't know what ExcelPackage is nor what this API has to offer, but in the native Excel object model Range.Value, when the Range object represents multiple cells, gives you a 2D variant array (object[,] in .NET) - if all you're ever interested in is the values that are in the cells (as seems to be the case), then you don't need to deal with worksheets and cells and .GetValue<T>().

So instead of looping through rows:

for (var rowNumber = 3; rowNumber <= worksheet.Dimension.End.Row; rowNumber++)

Grab the values array (not sure what the syntax would be with this API - if your library doesn't allow you to grab a 2D array of values, drop that library and use VSTO):

var values = worksheet.Range(worksheet.Cells[3, 1], worksheet.Cells[lastRow, 13]).Value;

The idea is to get a Range object that encompasses all the cells you're interested in. Maybe that's everything from row 3 to worksheet.Dimension.End.Row, but even without knowing the slightest thing about this particular API I would fear that worksheet.Dimension.End.Row yields something like 1,048,576 - which is very very very unlikely the actual number of rows you need to care about.

With the Excel object model, you'd do something like sheet.Range("A" & sheet.Rows.Count).End(xlUp).Row to get the last interesting row (aka lastRow), and use that row number to make the "interesting range" from which to read the values.

Now that you have a 2D array that contains all the values, GetEmployeeRowSummary doesn't make much sense anymore: we don't care for a rowNumber, and we don't need a ExcelWorksheet reference anymore.

In fact all we need is a "row slice" of the 2D array, by copying the row to a new array (this code looks like it):

public T[] GetRow(T[,] matrix, int rowNumber)
{
    return Enumerable.Range(0, matrix.GetLength(1))
            .Select(x => matrix[rowNumber, x])
            .ToArray();
}

private static Employee ReadEmployee(object[] row, string companyId)
{
    var id = Guid.NewGuid().ToString();
    return new Employee
    {
        CompanyId = companyId,
        EmployeeId = id,
        EmployeeNumber = (string)row[1],
        FirstName = (string)row[2],
        LastName = (string)row[3],
        BirthDate = (DateTime)row[4],
        //...
    };
}

As for mapping the properties automatically, I'm not a fan. Something, somewhere needs to do the mapping, and I'd much rather it be explicit in the code. Automatic mapping would need more metadata than just column indices - you'd likely need column names and data types, so instead of copying the cells to an array you'd be copying the cells to some DataTable, or have the mapper understand what an Excel Range or ListObject is... all of which sound like quite a lot of effort for an error-prone way to just list the properties at the left of an = operator. If your explicit casts blow up, you'll know why. If the automagic mapping blows up, pray the library throws meaningful exceptions.

\$\endgroup\$
  • 1
    \$\begingroup\$ Hi thanks for your reply, going to go through it now, but just to start the indentation was just a pasting issue. And the ExcelPackage is from a library called EPPlus, nuget.org/packages/EPPlus \$\endgroup\$ – R4nc1d Nov 21 '19 at 18:24
  • \$\begingroup\$ Ah, so there's no COM interop going on then, the package works directly with the OpenXML... which has pros ...and cons. Everything one knows about the Excel object model goes out the window, basically. \$\endgroup\$ – Mathieu Guindon Nov 21 '19 at 19:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.