Problem
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.....
}
Solution
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, ...);
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.