Readability
- Make use of
foreach
instead offor
whenever possible - Use
switch
instead ofif-else
ladder - Use variables to store and re-use. I have used
currentRow
forsheet.GetRow(i)
Before
for (int col = 1; col < sheet.GetRow(i).Cells.Count; col++)
{
if (sheet.GetRow(i).Cells[col].ToString() == "ATM") ATM = col;
else if (sheet.GetRow(i).Cells[col].ToString() == "Card-AMEX") AMEX = col;
else if (sheet.GetRow(i).Cells[col].ToString() == "Card-MASTER") MASTER = col;
else if (sheet.GetRow(i).Cells[col].ToString() == "Card-VISA") VISA = col;
else if (sheet.GetRow(i).Cells[col].ToString() == "Cash") CASH = col;
}
After
foreach (var cell in currentRow.Cells)
{
switch(cell.ToString())
{
case "ATM": ATM = col; break;
case "Card-AMEX": AMEX = col; break;
case "Card-MASTER": MASTER = col; break;
case "Card-VISA": VISA = col; break;
case "Cash": CASH = col; break;
}
}
- Use implicit typing for local variables when the type of the variable is obvious from the right side of the assignment, or when the precise type is not important.
- Use
CollectionInitializer
Before
SalesData sd = new SalesData();
sd.DateTime = dt;
sd.CreditCardAmex = sumAmex;
sd.CreditCardMaster = sumMaster;
sd.CreditCardVisa = sumVisa;
sd.Cash = sumCash;
sd.Others = sumATM;
After
var sd = new SalesData
{
DateTime = dt,
CreditCardAmex = sumAmex,
CreditCardMaster = sumMaster,
CreditCardVisa = sumVisa,
Cash = sumCash,
Others = sumATM
};
Use properties
instead of global variables. You can set default values to properties
like this in C# 6.0 or above.
public int ExpectedColumn { get; set; } = 15;
public int LineCount { get; set; } = 0;
public int StartIndex { get; set; } = -1;
public int EndIndex { get; set; } = -1;
public int ATM { get; set; } = -1;
public int AMEX { get; set; } = -1;
public int VISA { get; set; } = -1;
public int Cash { get; set; } = -1;
You don't need these variables to be declared outside score.
double sumATM = 0;
double sumAmex = 0;
double sumMaster = 0;
double sumVisa = 0;
double sumCash = 0;
You can easily terminate above lines because you are assigning default value in ternary operator itself.
double sumATM = ATM < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "Others", sheet.GetRow(j).GetCell(ATM).ToString());
double sumAmex = AMEX < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "CreditCardAmex", sheet.GetRow(j).GetCell(AMEX).ToString());
double sumMaster = MASTER < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "CreditCardMaster", sheet.GetRow(j).GetCell(MASTER).ToString());
double sumVisa = VISA < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "CreditCardVisa", sheet.GetRow(j).GetCell(VISA).ToString());
double sumCash = CASH < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "Cash", sheet.GetRow(j).GetCell(CASH).ToString());
You can use string.Empty
else if (sheet.GetRow(k).GetCell(0).ToString() == string.Empty)
Instead of
else if (sheet.GetRow(k).GetCell(0).ToString() == "")
Use String.Format
to organize code.
This will look more clear
var message = string.Format("Line {0}: Invalid Format. Maximum: {1} columns.", lnCnt, EXPECTED_col);
res.Messages.Add(message);
Than this
res.Messages.Add("Line " + lnCnt + ": Invalid format. Maximum: "+ EXPECTED_col +" columns.");
You can do negative checks first and return as suggested by Johan. This will results in much readable and less number of lines in code
public SalesFileParseResults Parse_Xlsx(string filePath)
{
if (!File.Exists(filePath))
return "File not found";
if (sheet.LastRowNum < 0 && sheet.GetRow(0).LastCellNum < 0 && sheet.GetRow(0).LastCellNum > EXPECTED_col)
return "Invalid format. Maximum: " + EXPECTED_col + " columns.";
for (int i = 0; i <= sheet.LastRowNum; i++)
{
// set the keyword index based on header
}
if (startIndex < -1 && endIndex < -1)
return "Invalid format";
for (int j = startIndex; j < endIndex; j++)
{
// grab data
}
if (isFound)
{
//add to List ...
res.SalesData.Add(sd);
}
return res;
}
As a side note:
- I feel you need to give more efforts on Naming Conventions. It will make this code more cleaner and readable.
- Also use constants in a logical classes to organize your application and error messages.