Extracting data from .txt files and writing to Excel - follow-up

Question

After I got this answer, I edited my code after that answer, but the code still seems kind of slow. Can I make any other improvements? And why does the code seem to get slower with every minute? It seems to start great and continue slow. Am I missing something here?

using System;
using System.Windows.Forms;
using Microsoft.Office.Interop.Excel;
using System.IO;
using System.Linq;
using System.Diagnostics;

namespace Report
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }
        string GetLine(string fileName, int line)
        {
            using (var sr = new StreamReader(fileName))
            {
                for (int i = 1; i < line; i++)
                    sr.ReadLine();
                return sr.ReadLine();
            }
        }
        private void button1_Click(object sender, EventArgs e)
        {


            Stopwatch timer = new Stopwatch();
            timer.Start();       

            Microsoft.Office.Interop.Excel.Application excelapp = new Microsoft.Office.Interop.Excel.Application();
            excelapp.Visible = true;
            _Workbook workbook = (_Workbook)(excelapp.Workbooks.Open(textBox2.Text));
            _Worksheet worksheet = (_Worksheet)workbook.ActiveSheet;
            DateTime dt = DateTime.Now;

            foreach (string fileName in Directory.GetFiles(textBox1.Text, "*.txt"))
            {
                int row = 1, EmptyRow = 0;
                while (Convert.ToString(worksheet.Range["A" + row].Value) != null)
                {
                    row++;
                    EmptyRow = row;
                }
                var line2s = File.ReadLines(fileName).Skip(9).Take(1).ToArray();             
                string comp = line2s[0];

                string compare = comp.Substring(30, comp.Length - 30);
                if (compare != "Failed")
                {
                    continue;
                }
                else
                { 
                    string[] lines = File.ReadAllLines(fileName);

                    string serial = lines[3];
                    string data = lines[4];
                    string time = lines[5];
                    string operat = lines[6];
                    string result = lines[9];

                    (worksheet.Cells[EmptyRow, 1] as Microsoft.Office.Interop.Excel.Range).Value2 = serial.Substring(30, serial.Length - 30);
                    (worksheet.Cells[EmptyRow, 2] as Microsoft.Office.Interop.Excel.Range).Value2 = data.Substring(30, data.Length - 30);
                    (worksheet.Cells[EmptyRow, 3] as Microsoft.Office.Interop.Excel.Range).Value2 = time.Substring(30, time.Length - 30);
                    (worksheet.Cells[EmptyRow, 4] as Microsoft.Office.Interop.Excel.Range).Value2 = operat.Substring(30, operat.Length - 30);
                    (worksheet.Cells[EmptyRow, 5] as Microsoft.Office.Interop.Excel.Range).Value2 = result.Substring(30, result.Length - 30);


                    foreach (string line in lines)
                    {
                        if (line.Contains("FixtureCoverResistance:"))
                        {                          
                            (worksheet.Cells[EmptyRow, 6] as Microsoft.Office.Interop.Excel.Range).Value2 = line.Substring(31, line.Length - 31);
                        }

                        else if (line.Contains("FwProgrammingCheck:"))
                        {                           
                            (worksheet.Cells[EmptyRow, 7] as Microsoft.Office.Interop.Excel.Range).Value2 = line.Substring(31, line.Length - 31);
                        }


                        else if (line.Contains("Checksum ="))
                        {
                            (worksheet.Cells[EmptyRow, 8] as Microsoft.Office.Interop.Excel.Range).Value2 = line.Substring(11, line.Length - 11);

                        }

                        else if (line.Contains("FwEepromCheck:"))
                        {
                            (worksheet.Cells[EmptyRow, 9] as Microsoft.Office.Interop.Excel.Range).Value2 = line.Substring(31, line.Length - 31);
                        }



                        else if (line.Contains("FixtureCoverResistanceAfterProg:"))
                        {
                            (worksheet.Cells[EmptyRow, 11] as Microsoft.Office.Interop.Excel.Range).Value2 = line.Substring(33, line.Length - 33);
                        }
                    }
                    }


            }
            TimeSpan ts = timer.Elapsed;
            label2.Text = ts.ToString("mm\\:ss\\.ff");
            timer.Stop();
        }

        private void button2_Click(object sender, EventArgs e)
        {
            if (folderBrowserDialog1.ShowDialog() == DialogResult.OK)
            {
                textBox1.Text = folderBrowserDialog1.SelectedPath;
            }                  
        }

        private void button3_Click(object sender, EventArgs e)
        {
            if (openFileDialog1.ShowDialog() == DialogResult.OK)
            {
                textBox2.Text = openFileDialog1.FileName.ToString();
            }
        }
    }
}

Answer 1

I am not providing review from programming good practice or architecture perspective. I just want to provide you some recommendations to improve performance of your code you have asked.

1) Find first empty cell on worksheet

Your while loop way to find first empty row is inefficient. Getting value from cell via Interop is slow.

Instead of this block:

while (Convert.ToString(worksheet.Range["A" + row].Value) != null)
{
   row++;
   EmptyRow = row;
}

Use single line built in Excel function:

int emptyRow = worksheet.Cells[worksheet.Cells.Rows.Count, "A"].End[-4162].Row + 1;

Constant -4162 is xlUp constant and you can find it in Excel documentation or in object browser in VBE.

2) Use temporary string[] array

Writing to worksheet cells one by one is inefficient. Work with string array and fill final array at once into worksheet.

You can initialize array:

string[,] rangeArray = new string[1, 11];    

Then instead of writing values to cells:

(worksheet.Cells[EmptyRow, 1] as Range).Value2 = serial.Substring(30, serial.Length - 30);
(worksheet.Cells[EmptyRow, 2] as Range).Value2 = data.Substring(30, data.Length - 30);

store them in array, which is very fast operation:

rangeArray[0,0] = serial.Substring(30, serial.Length - 30);
rangeArray[0,1] = data.Substring(30, data.Length - 30);

Finally fill array in worksheet:

Range c1 = (Range)worksheet.Cells[emptyRow, 1];
Range c2 = (Range)worksheet.Cells[emptyRow, 11];
Range range = worksheet.Range[c1, c2];
range.Value = rangeArray;

3) Still not satisfied with performance?

If you are still not satisfied with performance, avoid of using Office.Interop. You can parse your text files in C# and store the output in CSV file. The final CSV file you can easily open in Excel. There are also alternatives like ExcelLibrary. This should be also faster than Interop.

Answer 2

You're mixing UI and back-end code. There is no reason for the code to be in button1_Click; it should be in a class of its own, expose a method that receives the values of textBox1.Text and textBox2.Text as a parameter, and report back the elapsed time.

---

Why define DateTime dt = DateTime.Now; when you don't use it?

---

Don't call File.ReadLines(fileName) twice. Do it once, store the result in a variable and use that variable.

---

EmptyRow should be camelCase.

Also, this kind of initialization isn't typical for C#: int row = 1, EmptyRow = 0;.

---

Your entire method is 100 lines long and contains two foreach blocks. Split this up into smaller methods; worksheet can be a variable at the class level so you don't need to pass it around.

---

Seeing that this functionality is repeated several times, consider making it into a method with the necessary parameters:

(worksheet.Cells[EmptyRow, 6] as Microsoft.Office.Interop.Excel.Range).Value2 =

---

I note that you keep the Excel file open and then parse one or more text files. Have you tried closing and opening the Excel file for each text file you parse?

Also, have you considered other ways to open and edit Excel files besides Microsoft.Office.Interop, like OpenXml?

Answer 3

In addition to @BCdotWEB has suggested, a few improvements you could implement

• button1_Click() is doing so much, it contains a stopwatch, opening , reading and writing to an excel application why not split this into other methods.
• I'm not such if this winform or wpf but you should follow a nice architecture - MVVM or MVC or MVP to structure your code
• Yes, using qualified names is good but it seems to be very long why not use var e.g var excelapp = new Microsoft.Office.Interop.Excel.Application();
• excelapp should be camelCase as well e.g excelApp
• I suggest you should read your data into a .Net DataTable and you can read from it using a reader which should improve your performance

Answer 4

I'd like to show you yet another idea that IMO is the most generic one where you declaratively can configure the columns.

---

You start by creating a custom attribute. Whether you use indexes or letters or whatever it's your choice. This is just an example.

[AttributeUsage(AttributeTargets.Property)]
class ColumnAttribute : Attribute
{
    public ColumnAttribute(int index)
    {
        Index = index;
    }
    public int Index { get; }
}

Next you create a new type for the content of the file. Each property that is mapped to a column in your excel sheet you decorate with the above attribute:

class DataRow
{
    public static DataRow From(string[] lines)
    {
        return new DataRow
        {
            Serial = lines[3]
        };
    }

    [Column(1)]
    public string Serial { get; set; }
}

Later before you start the import you get the column indicies:

var columns  = typeof(DataRow)
    .GetProperties()        
    .Select(x => new
    {
        x.Name, 
        Column = x.GetCustomAttribute<ColumnAttribute>()?.Index 
    })
    .Where(x => x.Column.HasValue)
    .ToArray();

You load the file

var dataRow = DataRow.From(lines);

and you import it to excel

foreach (var property in columns)
{
    var cell = (worksheet.Cells[EmptyRow, property.Column.Value] as Microsoft.Office.Interop.Excel.Range);
    cell.Value2 = property.GetValue(dataRow);
}

---

 if (line.Contains("FixtureCoverResistance:"))

You can create another attribute to map these values as well and read all settings from the attributes. Also the magic numbers like 31 or 11 etc. The import method can be completely generic and the only place where you change the configuration are the attributes.