3
\$\begingroup\$

I've made a method to extract Excel data out of an existing Excel file. It contains 3 tabs where info should be extracted from. The data should be stored to use local so it's always fast available. It is currently working but it seems like I have a lot duplicate code that could most likely be written a lot shorter.

I'm using 9 different Properties.settings (all of the type list), 3 for each column in each of the tabs. The tabs I'm using are MPU, AAUX and ACTRL. I'm thinking of making a single list, where type is a class that contains 9 properties, but how do I accomplice this? Or isn't it even a good idea to do that? All the tabs contain the same columns only with different values.

enter image description here

public void ExcelExtract(string filePath)
            {
                Microsoft.Office.Interop.Excel.Application excelApp = new Microsoft.Office.Interop.Excel.Application();
                Excel.Workbook excelWorkbook = excelApp.Workbooks.Open(filePath);
                excelSheets = excelWorkbook.Worksheets;
                excelWorksheet = (Excel.Worksheet)excelSheets.get_Item("MPU");

                Excel.Range eersteColumn = excelWorksheet.UsedRange.Columns[1];
                Excel.Range derdeColumn = excelWorksheet.UsedRange.Columns[3];
                Excel.Range vierdeColumn = excelWorksheet.UsedRange.Columns[4];
                Excel.Range vijfdeColumn = excelWorksheet.UsedRange.Columns[5];

                System.Array eersteColumnWaarden = (System.Array)eersteColumn.Cells.Value;
                System.Array derdeColumnWaarden = (System.Array)derdeColumn.Cells.Value;
                System.Array vierdeColumnWaarden = (System.Array)vierdeColumn.Cells.Value;
                System.Array vijfdeColumnWaarden = (System.Array)vijfdeColumn.Cells.Value;

                var MPU = new List<object>();
                foreach (var a in eersteColumnWaarden) { MPU.Add(a); }
                Properties.Settings.Default.listFoutcodeMPU = MPU.Select(o => o == null ? String.Empty : o.ToString()).ToList();
                Properties.Settings.Default.Save();

                MPU = new List<object>();
                foreach (var a in derdeColumnWaarden) { MPU.Add(a); }
            Properties.Settings.Default.listGraadMPU = MPU.Select(o => o == null ? String.Empty : o.ToString()).ToList();
            Properties.Settings.Default.Save();

            MPU = new List<object>();
            foreach (var a in vierdeColumnWaarden) { MPU.Add(a); }
            Properties.Settings.Default.listProbleemMPU = MPU.Select(o => o == null ? String.Empty : o.ToString()).ToList();
            Properties.Settings.Default.Save();

            MPU = new List<object>();
            foreach (var a in vijfdeColumnWaarden) { MPU.Add(a); }
            Properties.Settings.Default.listOplossingMPU = MPU.Select(o => o == null ? String.Empty : o.ToString()).ToList();
            Properties.Settings.Default.Save();


            excelWorksheet = (Excel.Worksheet)excelSheets.get_Item("AAUX");

            eersteColumn = excelWorksheet.UsedRange.Columns[1];
            derdeColumn = excelWorksheet.UsedRange.Columns[3];
            vierdeColumn = excelWorksheet.UsedRange.Columns[4];
            vijfdeColumn = excelWorksheet.UsedRange.Columns[5];

            eersteColumnWaarden = (System.Array)eersteColumn.Cells.Value;
            derdeColumnWaarden = (System.Array)derdeColumn.Cells.Value;
            vierdeColumnWaarden = (System.Array)vierdeColumn.Cells.Value;
            vijfdeColumnWaarden = (System.Array)vijfdeColumn.Cells.Value;

            var AAUX = new List<object>();
            foreach (var a in eersteColumnWaarden) { AAUX.Add(a); }
            Properties.Settings.Default.listFoutcodeAAUX = AAUX.Select(o => o == null ? String.Empty : o.ToString()).ToList();
            Properties.Settings.Default.Save();

            AAUX = new List<object>();
            foreach (var a in derdeColumnWaarden) { AAUX.Add(a); }
            Properties.Settings.Default.listGraadAAUX = AAUX.Select(o => o == null ? String.Empty : o.ToString()).ToList();
            Properties.Settings.Default.Save();

            AAUX = new List<object>();
            foreach (var a in vierdeColumnWaarden) { AAUX.Add(a); }
            Properties.Settings.Default.listProbleemAAUX = AAUX.Select(o => o == null ? String.Empty : o.ToString()).ToList();
            Properties.Settings.Default.Save();

            AAUX = new List<object>();
            foreach (var a in vijfdeColumnWaarden) { AAUX.Add(a); }
            Properties.Settings.Default.listOplossingAAUX = AAUX.Select(o => o == null ? String.Empty : o.ToString()).ToList();
            Properties.Settings.Default.Save();


            excelWorksheet = (Excel.Worksheet)excelSheets.get_Item("ACTRL");

            eersteColumn = excelWorksheet.UsedRange.Columns[1];
            derdeColumn = excelWorksheet.UsedRange.Columns[3];
            vierdeColumn = excelWorksheet.UsedRange.Columns[4];
            vijfdeColumn = excelWorksheet.UsedRange.Columns[5];

            eersteColumnWaarden = (System.Array)eersteColumn.Cells.Value;
            derdeColumnWaarden = (System.Array)derdeColumn.Cells.Value;
            vierdeColumnWaarden = (System.Array)vierdeColumn.Cells.Value;
            vijfdeColumnWaarden = (System.Array)vijfdeColumn.Cells.Value;

            var ACTRL = new List<object>();
            foreach (var a in eersteColumnWaarden) { ACTRL.Add(a); }
            Properties.Settings.Default.listFoutcodeCTRL = ACTRL.Select(o => o == null ? String.Empty : o.ToString()).ToList();
            Properties.Settings.Default.Save();

            ACTRL = new List<object>();
            foreach (var a in derdeColumnWaarden) { ACTRL.Add(a); }
            Properties.Settings.Default.listGraadCTRL = ACTRL.Select(o => o == null ? String.Empty : o.ToString()).ToList();
            Properties.Settings.Default.Save();

            ACTRL = new List<object>();
            foreach (var a in vierdeColumnWaarden) { ACTRL.Add(a); }
            Properties.Settings.Default.listProbleemCTRL = ACTRL.Select(o => o == null ? String.Empty : o.ToString()).ToList();
            Properties.Settings.Default.Save();

            ACTRL = new List<object>();
            foreach (var a in vijfdeColumnWaarden) { ACTRL.Add(a); }
            Properties.Settings.Default.listOplossingCTRL = ACTRL.Select(o => o == null ? String.Empty : o.ToString()).ToList();
            Properties.Settings.Default.Save();


            excelApp.Quit();

        }
\$\endgroup\$
2
\$\begingroup\$

This section is duplicated the most:

var cellValues = new List<object>();
foreach (var a in eersteColumnWaarden) { cellValues.Add(a); }
Properties.Settings.Default.listFoutcodeMPU = cellValues.Select(o => o == null ? String.Empty : o.ToString()).ToList();

I think all it is doing is getting the cells from the columns. You need to extract to it's own method:

public List<string> GetCellValues(Array cells)
{
    var cellValues = new List<object>();
    foreach (var a in cells)
    {
        cellValues.Add(a);
    }
    return cellValues.Select(o => o == null ? String.Empty : o.ToString()).ToList();
} 

These two lines also get duplicated:

Range eersteColumn = excelWorksheet.UsedRange.Columns[1];
System.Array eersteColumnWaarden = (System.Array)eersteColumn.Cells.Value;

You can create another method for that like this:

public Array GetCells(Worksheet worksheet, int columnNumber)
{
    Range column = worksheet.UsedRange.Columns[columnNumber];
    return (Array)column.Cells.Value;
}

Actually, you can even make GetCellValues better by calling GetCells directly:

public List<string> GetCellValues(Worksheet worksheet, int columnNumber)
{
    var cellValues = new List<object>();
    foreach (var a in GetCells(worksheet, columnNumber))
    {
        cellValues.Add(a);
    }
    return cellValues.Select(o => o == null ? String.Empty : o.ToString()).ToList();
}

Now, you can simplify the ExcelExtract method so that instead of doing this:

var cellValues = new List<object>();
foreach (var a in eersteColumnWaarden) { cellValues.Add(a); }
Properties.Settings.Default.listFoutcodeMPU = cellValues.Select(o => o == null ? String.Empty : o.ToString()).ToList();
Properties.Settings.Default.Save();

You just need this for each column:

Properties.Settings.Default.listFoutcodeMPU = GetCellValues(excelWorksheet, 1);
Properties.Settings.Default.Save();

This is how it should look like after all those refactorings:

class ExcelReader
{
    private List<string> GetCellValues(Worksheet worksheet, int columnNumber)
    {
        var cellValues = new List<object>();
        foreach (var a in GetCells(worksheet, columnNumber))
        {
            cellValues.Add(a);
        }
        return cellValues.Select(o => o == null ? String.Empty : o.ToString()).ToList();
    }

    private Array GetCells(Worksheet worksheet, int columnNumber)
    {
        Range column = worksheet.UsedRange.Columns[columnNumber];
        return (Array)column.Cells.Value;
    }


    public void ExcelExtract(string filePath)
    {
        var excelApp = new Application();
        try
        {
            var excelWorkbook = excelApp.Workbooks.Open(filePath);
            var excelSheets = excelWorkbook.Worksheets;

            var excelWorksheet = (Worksheet)excelSheets["MPU"];
            Properties.Settings.Default.listFoutcodeMPU = GetCellValues(excelWorksheet, 1);
            Properties.Settings.Default.Save();
            Properties.Settings.Default.listGraadMPU = GetCellValues(excelWorksheet, 3);
            Properties.Settings.Default.Save();
            Properties.Settings.Default.listProbleemMPU = GetCellValues(excelWorksheet, 4);
            Properties.Settings.Default.Save();
            Properties.Settings.Default.listOplossingMPU = GetCellValues(excelWorksheet, 5);
            Properties.Settings.Default.Save();


            excelWorksheet = (Worksheet) excelSheets["AAUX"];
            Properties.Settings.Default.listFoutcodeAAUX = GetCellValues(excelWorksheet, 1);
            Properties.Settings.Default.Save();
            Properties.Settings.Default.listGraadAAUX = GetCellValues(excelWorksheet, 3);
            Properties.Settings.Default.Save();
            Properties.Settings.Default.listProbleemAAUX = GetCellValues(excelWorksheet, 4);
            Properties.Settings.Default.Save();
            Properties.Settings.Default.listOplossingAAUX = GetCellValues(excelWorksheet, 5);
            Properties.Settings.Default.Save();


            excelWorksheet = (Worksheet) excelSheets["ACTRL"];
            Properties.Settings.Default.listFoutcodeCTRL = GetCellValues(excelWorksheet, 1);
            Properties.Settings.Default.Save();
            Properties.Settings.Default.listGraadCTRL = GetCellValues(excelWorksheet, 3);
            Properties.Settings.Default.Save();
            Properties.Settings.Default.listProbleemCTRL = GetCellValues(excelWorksheet, 4);
            Properties.Settings.Default.Save();
            Properties.Settings.Default.listOplossingCTRL = GetCellValues(excelWorksheet, 5);
            Properties.Settings.Default.Save();
        }
        finally 
        {
            excelApp.Quit();   
        }
    }
}

There are still a few issues that you need to fix:

  • Handle exceptions - like what will happen if the file doesn't exist? Or if the worksheet doesn't exist?
  • Why do you need to call Save() after setting the list? Can it be called only once at the end the method?
\$\endgroup\$
1
\$\begingroup\$

excelSheets and excelWorksheet seem to be defined outside of this method. Is there a point to that?


No need to abbreviate: excelApp.


What's the point of eersteColumn, derdeColumn, vierdeColumn and vijfdeColumn considering that all you do is cast them to (System.Array) next?

I also would advise against naming variables in Dutch. IMHO code should be in English, otherwise you end up with this odd mixture which makes your code hard to read.


MPU doesn't follow the naming rules: Do capitalize only the first character of acronyms with three or more characters, except the first word of a camel-cased identifier.


WRT listFoutcodeMPU = MPU.Select(o => o == null ? String.Empty : o.ToString()).ToList();: does it makes sense to have an empty string as one of those values? To me it seems odd that an empty string would be in that collection, and I suspect you filter those out at a later stage. Don't do that, filter them out here.


Far too much of your code is obviously copy-pasted and then altered through search-replace. Once you find yourself doing so, you need to move that code to a method:

private List<string> GetErrorCodes(IEnumerable<object> values)
{
    return values.Where(x => x != null).Select(o => o.ToString()).ToList();
}

Note that I've replaced your Array with IEnumerable<object>. Why are you even casting to an Array when the next thing you do is loop through that Array to create a List<object>(), which you then loop through to fill a value in Properties.Settings.Default.?


Note that the repeated code is also happening at a larger scale, since it looks like you do the same to the values of "MPU" as you do with the values of "AAUX" and "ACTRL". Once you find yourself copy-pasting code, realize that you're probably doing it wrong and what you should be doing is move code to a method.

\$\endgroup\$
0
\$\begingroup\$

Two quick points:

First: Make sure you're cleaning up your Interop objects properly. It's not easy. See here for more. The first answer has good advice: never use two dots with COM objects. Many of your lines violate this, for example excelWorksheet.UsedRange.Columns[1]. Keep everything in a variable that you can later release.

var usedRange = excelWorksheet.UsedRange;
var columns = usedRange.Columns;
var column1 = columns[1];
//Do work
Marshal.ReleaseComObject(column1);
Marshal.ReleaseComObject(columns);
Marshal.ReleaseComObject(usedRange);

Release your objects this way Marshal.ReleaseComObject(object);


Second: I find it best to keep all of my Excel library code hidden in a Facade. In this way your isolating the complicated code that accesses the Excel library from the code that's generating the spreadsheet. This makes it easy to switch libraries in the future. You never know when your situation will change and you'll want to use EPPlus instead of Excel.Interop for Excel work. Or Aspose. Or NPOI. The Excel Interop library requires the computer to have Excel running on it. If you deploy your code to a server that doesn't have Excel, you'll have to dig through your code to replace your Interop calls with 3rd party calls. Don't forget that while Interop references are 1-based, many popular Excel libraries used 0-based references. It's much easier to abstract this away in a Facade.

\$\endgroup\$

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.