9
\$\begingroup\$

I have a code here that counts the number of fruits from Column B to Column AF (31 days). I used a switch with cases from 1 to 31. I'd like my code to be simpler. 31 case statements is just too long.

 private void button1_Click(object sender, EventArgs e)
    {
        Microsoft.Office.Interop.Excel.Application OfficeExcel;
        Microsoft.Office.Interop.Excel._Workbook OfficeWorkBook;
        Microsoft.Office.Interop.Excel._Worksheet OfficeSheet;

        var dtpMonth = dateTimePicker1.Value.ToString("MMMM");
        var dtpYear = dateTimePicker1.Value.Year;
        var MonthYear = dtpMonth + " - " + dtpYear;
        var dtpDay = dateTimePicker1.Value.Day;
        try
        {
            OfficeExcel = new Microsoft.Office.Interop.Excel.Application();
            OfficeExcel.Visible = true;

            int appletotal = Convert.ToInt32(lblappletotal.Text);
            int bananatotal = Convert.ToInt32(lblbananatotal.Text);
            int orangetotal = Convert.ToInt32(lblorangetotal.Text);
            int grapestotal = Convert.ToInt32(lblgrapestotal.Text);

            switch (dateTimePicker1.Value.Day.ToString())
            { 
                case "1":
                     OfficeWorkBook = (Microsoft.Office.Interop.Excel._Workbook)(OfficeExcel.Workbooks.Add(""));
                     OfficeSheet = (Microsoft.Office.Interop.Excel._Worksheet)OfficeWorkBook.ActiveSheet;

                    OfficeSheet.Cells[3,1] = "apple";
                    OfficeSheet.Cells[4,1] = "banana";
                    OfficeSheet.Cells[5,1] = "orange";
                    OfficeSheet.Cells[6,1] = "grapes";

                    OfficeSheet.Cells[2, 2] = dtpDay + dtpMonth ;
                    OfficeSheet.Cells[3, 2] = appletotal; // variable
                    OfficeSheet.Cells[4, 2] = bananatotal;
                    OfficeSheet.Cells[5, 2] = orangetotal;
                    OfficeSheet.Cells[6, 2] = grapestotal;

                    OfficeExcel.Visible = true;



                    OfficeWorkBook.SaveAs("D:\\fruits\\" + MonthYear + ".xls", Microsoft.Office.Interop.Excel.XlFileFormat.xlExcel7, Type.Missing, Type.Missing,
                                    false, false, Microsoft.Office.Interop.Excel.XlSaveAsAccessMode.xlNoChange,
                                    Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing);


                     break;


                case "2":
                    OfficeWorkBook = OfficeExcel.Workbooks.Open("D:\\fruits\\" + MonthYear + ".xls");
                    OfficeSheet = (Excel.Worksheet)OfficeWorkBook.Worksheets.get_Item(1);

                    OfficeSheet.Cells[2, 3] = dtpDay + dtpMonth;
                    OfficeSheet.Cells[3, 3] = appletotal;
                    OfficeSheet.Cells[4, 3] = bananatotal;
                    OfficeSheet.Cells[5, 3] = orangetotal;
                    OfficeSheet.Cells[6, 3] = grapestotal;

                    OfficeExcel.Visible = true;
                    OfficeWorkBook.Save();


                    case "3":
                    OfficeWorkBook = OfficeExcel.Workbooks.Open("D:\\fruits\\" + MonthYear + ".xls");
                    OfficeSheet = (Excel.Worksheet)OfficeWorkBook.Worksheets.get_Item(1);

                    OfficeSheet.Cells[2, 4] = dtpDay + dtpMonth;
                    OfficeSheet.Cells[3, 4] = appletotal;
                    OfficeSheet.Cells[4, 4] = bananatotal;
                    OfficeSheet.Cells[5, 4] = orangetotal;
                    OfficeSheet.Cells[6, 4] = grapestotal;

                    OfficeExcel.Visible = true;
                    OfficeWorkBook.Save();

                    break;
                case "4":
                    OfficeWorkBook = OfficeExcel.Workbooks.Open("D:\\fruits\\" + MonthYear + ".xls");
                    OfficeSheet = (Excel.Worksheet)OfficeWorkBook.Worksheets.get_Item(1);

                    OfficeSheet.Cells[2, 5] = dtpDay + dtpMonth;
                    OfficeSheet.Cells[3, 5] = appletotal;
                    OfficeSheet.Cells[4, 5] = bananatotal;
                    OfficeSheet.Cells[5, 5] = orangetotal;
                    OfficeSheet.Cells[6, 5] = grapestotal;

                    OfficeExcel.Visible = true;
                    OfficeWorkBook.Save();

                    break;
                    .
                    .
                    .
                    so On.....
    }
\$\endgroup\$
1
  • \$\begingroup\$ @Heslacher. I edit it. \$\endgroup\$
    – jLaw
    Dec 18 '15 at 14:08
8
\$\begingroup\$
private void button1_Click(object sender, EventArgs e)

You have an UI control name button1. It's not a good name because you can't understand (in code) what its purpose is. Use a meaningful name. The same is also valid for dateTimePicker1.

int appletotal = Convert.ToInt32(lblappletotal.Text);

First of all you should use proper casing for your variables. In C#, usually, case for local variables is camelCase. Use it consistently: appleTotal and officeExcel.

Moreover OfficeExcel, OfficeWorkbook and so on are not so descriptive names. If you keep your function small you don't need more than excel, workbook, activeWorksheet and so on (that you're talking about Microsoft Office objects is obvious).

Microsoft.Office.Interop.Excel.Application OfficeExcel;
...
OfficeExcel = new Microsoft.Office.Interop.Excel.Application();

It's C#, you do not need to declare your variables at very beginning of your method. Declare them as late as possible (ideally together with their initialization):

var workbook = (Microsoft.Office.Interop.Excel._Workbook)(excel.Workbooks.Add(""));

COM objects should (must?) be disposed, they do not implement IDisposable then you have to use Marshal.FinalReleaseComObject(excel) (also for other objects). See also How to release COM handle in .NET.

int appletotal = Convert.ToInt32(lblappletotal.Text);

You do not perform any error checking, user may enter an invalid string and your application will simply crash.

int apples;
if (!Int32.TryParse(lblApples.Text, out apples)) {
    // Invalid input. Also check other TryParse overloads.
}
"D:\\fruits\\" + MonthYear + ".xls" ...

First of all do not hardcode constants! Won't you ever need to save to any other location? Even if it's written in stone use a const field for that:

private const string OutputFolderPath = @"d:\fruits";

Then you should never never compose paths by hand:

string outputFilePath = Path.Combine(OutputFolderPath, monthAndYear + ".xls");

You may also want to give it some dignity and create a ResolveOutputFileName().

OfficeSheet.Cells[2, 5] = dtpDay + dtpMonth;
OfficeSheet.Cells[3, 5] = appletotal;
OfficeSheet.Cells[4, 5] = bananatotal;
OfficeSheet.Cells[5, 5] = orangetotal;
OfficeSheet.Cells[6, 5] = grapestotal;

This block of code is repeated in each case. Refactor it out into a separate method (with also calculation for that variables) ExportEatenFruitInRow(int row, ...) (please pick a better and more meaningful name).

switch (dateTimePicker1.Value.Day.ToString())

You do not need to convert to string to perform a comparison:

switch (dateTimePicker1.Value.Day) {
    case 1:
    ...
}

However you're using dateTimePicker1.Value.Day simply as an offset for method you refactored out just now. You should simply delete entire switch/case calling that method:

ExportEatenFruitInRow(dateTimePicker1.Value.Day + 1, ...);
\$\endgroup\$
4
\$\begingroup\$

Do yourself a favor, and add this line at the top of the code file:

using Microsoft.Office.Interop.Excel;

Now these locals:

Microsoft.Office.Interop.Excel.Application OfficeExcel;
Microsoft.Office.Interop.Excel._Workbook OfficeWorkBook;
Microsoft.Office.Interop.Excel._Worksheet OfficeSheet;

Can be declared like this:

    Application OfficeExcel;
    _Workbook OfficeWorkBook;
    _Worksheet OfficeSheet;

Much less annoying isn't it! Now, _Workbook and _Worksheet don't feel very natural - see this answer on Stack Overflow about the differences between _Worksheet and Worksheet, but long story short, I'd declare them like this:

    Application excelApp;
    Workbook workbook;
    Worksheet worksheet;

The established naming convention for local variables is to use camelCase: notice that the Stack Exchange syntax highlighter no longer confuses them for class names! You don't need to prefix everything with "Office" either, besides it's wrong to prefix "Workbook" with "Office", since a "workbook" is an Excel object, Office knows nothing of workbooks and worksheets.

\$\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.