3
\$\begingroup\$

Intro: Today I have decided to make an Excel automation task with C#. This is probably the first time I am doing something like this, thus the problems are plenty.

The task: Pretty much, the idea is the following - I have 4 excel files in folder strPath. I have to loop through all of them and make a file called Report.xlsx in the same folder, with the information from those files.

The information, that I need is anything, below row 9. Thus, the first row to copy is row number 10. That is why, the first file I loop for is saved as Report, and the bMakeOnce value is changed. After the first file is looped and saved As, I start entering into the else condition. There I locate the last used row of the Excel files and I try to copy the range into the sheetReport.

The questions: Pretty much the code works as expected. However, I was thinking of some improvements, as far as probably there should be a way to make the things neater. E.g., some fancy try-catch-finally or similar. And some good practises, e.g. what would make to a separate function?

using System;
using System.IO;
using Excel = Microsoft.Office.Interop.Excel;

class MainClass
{
    static void Main()
    {
        string strPath = Path.GetFullPath(Path.Combine(Directory.GetCurrentDirectory(), @"..\..\..\"));
        string[] strFiles = Directory.GetFiles(strPath);
        Excel.Application excel = null;
        bool bMakeOnce = true;
        string strReportName = "Report.xlsx";
        int intFirstLine = 10;
        int intLastColumn = 50;
        int lastRow;
        int lastRowReport;
        int intTotalRows;

        Excel.Workbook wkbReport = null;
        string strWkbReportPath;
        int n = 0;

        excel = new Excel.Application();
        excel.Visible = true;

        foreach (string strFile in strFiles)
        {
            if (strFile.Contains(strReportName))
            {
                Console.WriteLine(strReportName + " is deleted.");
                File.Delete(strFile);
            }
        }

        foreach (string strFile in strFiles)
        {
            if (strFile.Contains(strReportName))
            {
                continue;
            }
            Console.WriteLine(strFile);
            Excel.Workbook wkb = null;
            Excel.Worksheet sheet = null;
            Excel.Worksheet sheetReport = null;
            Excel.Range rngLastReport = null;
            Excel.Range rngToCopy = null;

            wkb = Open(excel, strFile);
            if (bMakeOnce)
            {
                bMakeOnce = false;
                strWkbReportPath = wkb.Path + "\\" + strReportName;
                wkb.SaveAs(strWkbReportPath);
                wkb.Close();
                wkbReport = Open(excel, strWkbReportPath);
            }
            else
            {   
                sheetReport = wkbReport.Worksheets[1];
                sheet = wkb.Worksheets[1];

                //lastRow = sheet.Cells[1, 3].get_End(Excel.XlDirection.xlUp).Row;

                intTotalRows = sheet.Rows.Count;
                lastRow = sheet.Cells[intTotalRows, 1].End(Excel.XlDirection.xlUp).Row;
                lastRowReport = sheetReport.Cells[intTotalRows, 1].End(Excel.XlDirection.xlUp).Row;

                //lastRowReport = sheetReport.Cells[intTotalRows, 1].get_End(Excel.XlDirection.xlUp).Row;
                //lastRowReport = sheetReport.Cells[intTotalRows, intTotalRows.End[Excel.XlDirection.xlUp]].Row;
                n++;

                rngToCopy = sheet.Range[sheet.Cells[intFirstLine,1],sheet.Cells[lastRow, intLastColumn]];
                int size = rngToCopy.Rows.Count;
                rngLastReport = sheetReport.Range[sheetReport.Cells[lastRowReport+1, 1], sheetReport.Cells[lastRowReport + 1+size, intLastColumn]];

                rngToCopy.Copy(rngLastReport);
                wkb.Close(false);
            }
        }
        wkbReport.Close(true);
        excel.Quit();
        Console.WriteLine("Finished!");
    }

    public static Excel.Workbook Open(Excel.Application excelInstance, string fileName, bool readOnly = false, bool editable = true, bool updateLinks = true)
    {
        Excel.Workbook book = excelInstance.Workbooks.Open(
            fileName, updateLinks, readOnly,
            Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing,
            Type.Missing, editable, Type.Missing, Type.Missing, Type.Missing,
            Type.Missing, Type.Missing);
        return book;
    }
    //public static Excel.Workbook OpenBook(Excel.Application excelInstance, string fileName, bool readOnly = false, bool editable = true, bool updateLinks = true)
    //{
    //    Excel.Workbook book = excelInstance.Workbooks.Open(
    //        fileName, updateLinks, readOnly,
    //        Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing,
    //        Type.Missing, editable, Type.Missing, Type.Missing, Type.Missing,
    //        Type.Missing, Type.Missing);
    //    return book;
    //}
}
\$\endgroup\$
2
\$\begingroup\$
string strReportName = "Report.xlsx";
int intFirstLine = 10;
int intLastColumn = 50;

You don't use magic strings or numbers. I like it ;-)


bool bMakeOnce = true;
string strReportName = "Report.xlsx";
int intFirstLine = 10;

We don't use hungarian notation in C#. The variables names should be respectively

var makeOnce = false;
var reportFileName = "..";
var firstLine = 10;

and so on...


strWkbReportPath = wkb.Path + "\\" + strReportName;

You used the Path.Combine method once you can use it here too

var reportFullName = Path.Combine(path, reportFileName);

foreach (string strFile in strFiles)
{
    if (strFile.Contains(strReportName))
    {
        Console.WriteLine(strReportName + " is deleted.");
        File.Delete(strFile);
    }
}

You should separate this loop from the other one. This means instead of getting all files and deleting reports and then skipping those file names in the second loop, you first delete the reports and then process the remaining files.

This can be done by specifying a search pattern for the GetFiles method

var reportName = "Report.xlsx";
var reportFileNames = Directory.GetFiles(path, @"*{reportName}");

foreach (string reportFileName in reportFileNames)
{
    Console.WriteLine(@"{reportFileName} deleted.");
    File.Delete(reportFileName);
}

This means it'll get all files whose names end with reportName.


In order to process the remaining files you only get *.xlsx files for the next loop:

var dataFileNames = Directory.GetFiles(path, "*.xlsx");

foreach (string dataFileName in dataFileNames)
{
    ...
}

and you no longer need this condition

if (strFile.Contains(strReportName))
{
    continue;
}

bool bMakeOnce = true;

This variable is not necessary. You can make the same decition based on the report. Create if it's null otherwise skip it.

Excel.Workbook wkb = null;
Excel.Worksheet sheet = null;
Excel.Worksheet sheetReport = null;
Excel.Range rngLastReport = null;
Excel.Range rngToCopy = null;

Usually it's better to define the variables nearest to their usage. You don't need them globaly but only inse the else block so define them where you actually start using them.


In general the second loop could become

Excel.Workbook report = null;
string strWkbReportPath;
int n = 0;

excel = new Excel.Application();
excel.Visible = true;    

try
{
    var dataFileNames = Directory.GetFiles(path, "*.xlsx");

    foreach (string dataFileName in dataFileNames)
    {

        ..

        if (report == null)
        {
            var reportFullName = Path.Combine(path, reportFileName);
            wkb.SaveAs(reportFullName);
            wkb.Close();
            report = Open(excel, reportFullName);
        }
        else
        {

            var sheetReport = report.Worksheets[1];
            var sheet = wkb.Worksheets[1];
            ..

        }
    }
}
finally
{
    if (report != null)
    {
        report.Close(true);
    }
}

If you want to be sure that each workbook is properly closed you can add another try/catch for the data files inside the loop.


Now that everything is nice and clean the only thing that remains is to split the Main up in to smaller pieces.

Define constants outside the Main

const string reportName = "Report.xlsx";
const int firstLine = 10;
const int lastColumn = 50;

and encapsulate deleting old reports and creating new ones:

static void Main()
{
    ..

    DeleteReports();
    CreateReport();
}

static void DeleteReports()
{
    .. 
}

static void CreateReport()
{
    ..
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the review. The ideas are good, indeed. Path.Combine is a good idea everywhere, indeed. Concerning the loops and the story about them, I am not sure whether you got my idea. Initially, I probably have one or more files, which are named with "report". I want to delete them. Later, when I delete them, I create on the same folder new report file. Thus, this report file is part of the collection foreach (string strFile in strFiI was thinking about some other way to access the files, and not with the Public static Excel.Workbook Open., because it seems a bit strange/old. \$\endgroup\$ – Vityata Jan 20 '17 at 15:52
  • \$\begingroup\$ @Vityata I think I get the idea ;-) You get all files from the folder strPath, then you delete the report files and process the remaining ones while skipping the deleted files. This makes it uncecessarility complex and couples both loops. What I suggest is that you first delete the old reports and then process the data files in two separate, independant steps. The report files are deleted so you no longer have to skip them. \$\endgroup\$ – t3chb0t Jan 20 '17 at 15:56
  • \$\begingroup\$ After deleting the reports, the first file that I find is savedAs a report. Thus, the bMakeOnce is flagged. And then, it also enters in the same collection and I loop over him. \$\endgroup\$ – Vityata Jan 20 '17 at 16:01
  • \$\begingroup\$ I know my code is really bad, but yesterday I was a bit tired and a bit unhappy that it was not going as fast as it would be with VBA. \$\endgroup\$ – Vityata Jan 20 '17 at 16:03

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.