Read sales transactions from Excel

Question

This is a program to read and store the data from a Excel file (.xlsx) which is uploaded by users.

1. Any better approach to reduce the usage of if-else to return the error message?
2. How to design or construct a good coding pattern before I code? What kind of aspects I should pay more attention on ?

Excel file uploaded by user:

| Date       | XXX | XXX | ATM | Card-AMEX | Card-MASTER | Card-VISA | Cash | Total  | By    |

| 11/01/2015 | XXX | XXX | 0   | 100       | 50.20       | 0         | 0    | 150.20 | ADMIN |    
|            | XXX | XXX | 0   | 0         | 50.00       | 0         | 0    |  50.00 | ADMIN |    
| 12/01/2015 | XXX | XXX | 0   | 200       | 10.25       | 0         | 0    | 210.25 | ADMIN |

Code

public SalesFileParseResults Parse_Xlsx(string filePath)
{
        SalesFileParseResults res = new SalesFileParseResults();
        res.Success = true;

        if (File.Exists(filePath))
        {
            int EXPECTED_col = 15;
            int lnCnt = 0;
            string content = File.ReadAllText(filePath);
            try
            {
                // Note:
                // Add Reference: NOPI.dll, NOPI.OOXML.dll, NPOI.OpenXml4Net.dll
                // HSSFWorkbook: xls, XSSFWorkbook: xlsx

                XSSFWorkbook excel = new XSSFWorkbook(filePath);
                ISheet sheet = excel.GetSheetAt(0);
                if (sheet.LastRowNum > 0 && sheet.GetRow(0).LastCellNum > 0 && sheet.GetRow(0).LastCellNum <= EXPECTED_col)
                {
                    int startIndex = -1;
                    int endIndex = -1;
                    int ATM = -1;
                    int AMEX = -1;
                    int MASTER = -1;
                    int VISA = -1;
                    int CASH = -1;

                    for (int i = 0; i <= sheet.LastRowNum; i++)
                    {
                        if(sheet.GetRow(i).GetCell(0).ToString() == "Date") //may contain empty cell
                        {
                            startIndex = i + 1;
                            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;
                            }
                        }
                        else if(sheet.GetRow(i).GetCell(3).ToString() == "Total")
                        {
                            endIndex = i;
                        }
                    }

                    string format = "dd-MMM-yyyy"; //01-Jul-2015
                    if(startIndex > -1 && endIndex > -1)
                    {
                        for (int j = startIndex; j < endIndex; j++)
                        {
                            lnCnt++;
                            double sumATM = 0;
                            double sumAmex = 0;
                            double sumMaster = 0;
                            double sumVisa = 0;
                            double sumCash = 0;
                            DateTime dt;
                            if (DateTime.TryParseExact(sheet.GetRow(j).GetCell(0).ToString(), format, CultureInfo.InvariantCulture, DateTimeStyles.None, out dt))
                            {
                                sumATM = ATM < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "Others", sheet.GetRow(j).GetCell(ATM).ToString());
                                sumAmex = AMEX < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "CreditCardAmex", sheet.GetRow(j).GetCell(AMEX).ToString());
                                sumMaster = MASTER < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "CreditCardMaster", sheet.GetRow(j).GetCell(MASTER).ToString());
                                sumVisa = VISA < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "CreditCardVisa", sheet.GetRow(j).GetCell(VISA).ToString());
                                sumCash = CASH < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "Cash", sheet.GetRow(j).GetCell(CASH).ToString());

                                //Check next row is datetime?
                                bool isFound = false;
                                for (int k = j + 1; k <= endIndex; k++)
                                {
                                    DateTime dtNext;
                                    if (k == endIndex || DateTime.TryParseExact(sheet.GetRow(k).GetCell(0).ToString(), format, CultureInfo.InvariantCulture, DateTimeStyles.None, out dtNext))
                                    {
                                        isFound = true;
                                        j = k - 1;
                                        break;
                                    }
                                    else if (sheet.GetRow(k).GetCell(0).ToString() == "")
                                    {
                                        sumATM = ATM < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "Others", sheet.GetRow(k).GetCell(ATM).ToString());
                                        sumAmex += AMEX < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "CreditCardAmex", sheet.GetRow(k).GetCell(AMEX).ToString());
                                        sumMaster += MASTER < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "CreditCardMaster", sheet.GetRow(k).GetCell(MASTER).ToString());
                                        sumVisa += VISA < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "CreditCardVisa", sheet.GetRow(k).GetCell(VISA).ToString());
                                        sumCash += CASH < 0 ? 0 : Parse_double_SothysXlsx(res, lnCnt, "Cash", sheet.GetRow(k).GetCell(CASH).ToString());
                                    }
                                }
                                if (isFound)
                                {
                                    SalesData sd = new SalesData();
                                    sd.DateTime = dt;
                                    sd.CreditCardAmex = sumAmex;
                                    sd.CreditCardMaster = sumMaster;
                                    sd.CreditCardVisa = sumVisa;
                                    sd.Cash = sumCash;
                                    sd.Others = sumATM;
                                    res.SalesData.Add(sd);
                                }
                            }
                        }
                    }
                    else
                    {
                        res.Success = false;
                        res.Messages.Add("Line " + lnCnt + ": Invalid format.");
                    }
                }
                else
                {
                    res.Success = false;
                    if()
                    res.Messages.Add("Line " + lnCnt + ": Invalid format. Maximum: "+ EXPECTED_col +" columns.");
                }
            }
            catch (Exception ex)
            {
                res.Success = false;
                res.Messages.Add("Line " + lnCnt + ": " + ex.ToString());
            }

            if (res.SalesData.Count > 0)
            {
                res.Success = true;
            }
        }
        else
        {
            res.Success = false;
            res.Messages.Add("File not found.");
        }

        //debug
        string hi = "";
        foreach (SalesData item in res.SalesData)
        {
            hi += item.DateTime.ToString() + "\n" + item.Others + "\n" + item.CreditCardAmex + "\n" + item.CreditCardMaster + "\n" + item.CreditCardVisa + "\n" + item.Cash + "\n\n";
        }

        return res;
}

Summary

public SalesFileParseResults Parse_Xlsx(string filePath)
{
    if (File.Exists(filePath))
    {
        if(sheet.LastRowNum > 0 && sheet.GetRow(0).LastCellNum > 0 && sheet.GetRow(0).LastCellNum <= EXPECTED_col)
        {
            for (int i = 0; i <= sheet.LastRowNum; i++)
            {
                // set the keyword index based on header
            }

            if(startIndex > -1 && endIndex > -1) //make sure both index are set
            {
                for (int j = startIndex; j < endIndex; j++)
                {
                    // grab data
                }

                if(isFound)
                {
                    //add to List ...
                    res.SalesData.Add(sd);
                }
            }
            else
            {
                return "Invalid format"
            }
        }
        else
        {
            return "Invalid format. Maximum: "+ EXPECTED_col +" columns.");
        }
    }              
    else
    {
        return "File not found"
    }

    return res;
}

As you can see, there are multiple nesting if-else because I need to verify some condition. Is there any method or better code design to do something similar as I doing now? I read some suggestions:

• create new method to replace to a part of the code
• Switch case
• create class as validator

But I do not know how to implement with the suggestions above.

Answer 1

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.

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.

Answer 2

The first thing that comes to mind when seeing your code is changing this

 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;

To the following (it might seem like more lines of code, but its way more readable and doesn't repeat sheet.GetRow(i).Cells[col].ToString() 5 times.

switch(sheet.GetRow(i).Cells[col].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;
}

Also, try not to repeat doing the same thing over and over again.

ie. you call sheet.GetRow(k) so many times. Instead create a variable and call it once and use that variable. Same concept as above and as a bonus, you will get a performance increase.

Edit

As per the question, you want a way to clean up all the if statements (the nesting)

Instead of

if(A == B)
{
    //50 lines of code
}

do

if(A != B)
{
     //return/exit/...
}

//50 lines of code

The other option to get "rid" of all the complex if logic, is to break your code blocks into their own functions/methods. I personally don't like code blocks bigger than 15 lines (incl spacing/bracers), but of course this is my opinion.

Visual studio has a really nice extension, called CodeMaid which gives you a value based on the complexity of the method. This will also teach you to create smaller, more readable, code blocks.