Skip to main content
5 of 7
deleted 3 characters in body
SiD
  • 597
  • 2
  • 8

Readability

  • Make use of foreach instead of for whenever possible
  • Use switch instead of if-else ladder
  • Use variables to store and re-use. I have used currentRow for sheet.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.
SiD
  • 597
  • 2
  • 8