3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ @200_success Why you changed to title to Read sales transactions from Excel instead of How to reduce if-else usage for multiple conditions and return error messages ? This question is about how to use IF-ELSE in the proper way. So I think How to reduce if-else usage for multiple conditions and return error messages? is more suitable. \$\endgroup\$ – sky91 Dec 2 '15 at 6:32
  • \$\begingroup\$ It is the site policy. Feel free to ask in our chat room if you have questions about the policy. \$\endgroup\$ – 200_success Dec 2 '15 at 6:34
6
\$\begingroup\$

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.
\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I added summary section. You may have a look. \$\endgroup\$ – sky91 Nov 26 '15 at 14:05
  • \$\begingroup\$ @sky91 updated answer. \$\endgroup\$ – Johan Nov 26 '15 at 14:40

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.