Catch statements with other Business Logic in Parsing Files

Question

I am writing a simple program to parse a text file and place into Generic List.

Sample Text:

1,Joe,CA,58,2
2,Matt,TX,63,5

Sometimes, there may be an error, with missing data in the file

1,Joe,CA,58   // missing one number
2,Matt,TX,63,5

I wrote a catch statement to handle such errors.

My Software Principle Question:

I was told catch statements should not be utilized to handle other business logic. In this catch statement, I am then creating an error folder, and a file with data which caused issue. Where should I relocate these statements? See catch statement below.

public class CustomerData
{
    public int CustomerId { get; set; }
    public string CustomerName { get; set; }
    public string CustomerState { get; set; }
    public int ProductId { get; set; }
    public int QuantityBought { get; set; }
}

public List<CustomerData> GetCustomer(string filename)
{
    List<CustomerData> customerdata = new List<CustomerData>();
    string CustomerBase = filename;

    String fileToLoad = String.Format(CustomerBase);
    using (StreamReader r = new StreamReader(fileToLoad))
    {
        string line;
        while ((line = r.ReadLine()) != null)
        {
            string[] parts = line.Split(',');
            // Skip the column names row
            if (parts[0] == "id") continue;

            try
            {
                CustomerData dbp = new CustomerData
                { 
                    CustomerId = Convert.ToInt32(parts[0]),
                    CustomerName = parts[1],
                    CustomerState = parts[2],
                    ProductId = Convert.ToInt32(parts[3]),
                    QuantityBought = Convert.ToInt32(parts[4]),
                };
                customerdata.Add(dbp);
            }
            catch
            {
                Console.WriteLine("Parse Error!");

                string ErrorFolderPath = @"C:\Users\Desktop\Parsefile\ErrorFile";
                string ErrorFile = System.IO.Path.Combine(ErrorFolderPath, Path.GetFileName(filename));


                bool FolderExists = System.IO.Directory.Exists(ErrorFolderPath);
                if (!FolderExists)
                    System.IO.Directory.CreateDirectory(ErrorFolderPath);

                bool ErrorFileExists = System.IO.File.Exists(ErrorFile);
                if (!ErrorFileExists)
                    System.IO.File.Create(ErrorFile);

                using (TextWriter tw = new StreamWriter(ErrorFile))
                {
                    tw.WriteLine(line);
                }

            }
        }
    }
    return customerdata;
}

Answer 1

You don't want to do anything too complicated within a catch block because any exceptions in a catch block will bubble up and the flow of control starts to get confusing (especially if you nest try blocks inside a catch). Instead, save only what you need to trigger your exception logic, and put that logic in an if block that depends on the flag.

Your example is a little long but here is a simpler representation of it:

void DoSomething()
{
    try
    {
        DoSomethingErrorProne();
    }
    catch
    {
        DoSomethingComplicatedToHandleAProblem();  //bad
    }
}

And here's how you'd extract the logic:

public void DoSomething()
{
    var ok = TryDoSomethingInternal();
    if (!ok) DoSomethingComplicatedToHandleAProblem();
}

private bool TryDoSomethingInternal();
{
    try
    {
        DoSomethingErrorProne();
        return true;
    }
    catch
    {
        return false;
    }
}

Answer 2

First question would be "why aren't you using a logging library?" That would take care of everything you are doing in the catch statement.

Essentially your code boils down to this pseudocode:

try
{
    CustomerData dbp = new CustomerData
    { 
        CustomerId = Convert.ToInt32(parts[0]),
        CustomerName = parts[1],
        CustomerState = parts[2],
        ProductId = Convert.ToInt32(parts[3]),
        QuantityBought = Convert.ToInt32(parts[4]),
    };
    customerdata.Add(dbp);
}
catch (Exception e)
{
    WriteErrorLine(line);
}

That's acceptable. Particularly when you aren't getting errors every line in a million line file. In other words the handling is for something exceptional.

---

Typically the advice about not using exceptions to handle application logic rely on exceptions that will happen on just about every line. To demonstrate why it's bad, let's just say for example that CustomerState was sometimes a two letter state code, sometimes the full state, and sometimes an integer id. Let's also say that you needed the state to evaluate to an enum. This is where things can get messy. Now it's not an exceptional state to have a parsing error. Just to keep things tidy you might have that parsing happen in a function.

private State ParseState(string raw)
{
    try
    {
         return (State) Enum.Parse(typeof(State), raw);
    }
    catch (ArgumentException) {}

    try
    {
         return abbreviations[raw];
    }
    catch (KeyNotFoundException) {}

    try
    {
         return (State) int.Parse(raw);
    }
    catch (ParseException) {}

    throw IllegalArgumentException("Could not parse state string");
}

Now, imagine also if you logged each one of those exceptions were logged and your files switched how they represented the state line by line. There's a lot of exceptions being thrown, in the worst case 4 exceptions for each line. Filling in the stack trace for those exceptions takes a good chunk of time. If this were called only once in a while it might not be so bad, but when parsing a file with even just thousands of lines the impact of those exceptions compound.

In this case it would be both more correct and perform better to do some tests on the string passed in to see which path was most likely and then use the TryParse or TryGetValue variants of methods that don't throw exceptions.

Answer 3

I was told catch statements should not be utilized to handle other business logic.

There is a lot of confusion on this point. People often refer to what other people said, but there are cases where others make blanket statements which are much broader than they need to be.

I don't like oversimplified rules (that omit any justification for why the rule is in place), I much prefer understanding the exact reason. So let me explain the reason why you should avoid using exceptions as much as you can.

---

For exception handling, this boils down to performance. Throwing an exception and catching it is rather expensive (performance wise). You are much better off using an intended return value as opposed to an exception.

As a simple example, I created a little algorithm that plays Tic Tac Toe. In some fringe cases, the AI refuses to make a move, and therefore resigns the game. I implemented this as a BotResignsException. It worked very well when I initially tested this (playing manual games against the AI), but it crumbled once I started simulating bulk games (> 1 million games). The performance hit was massive.
I ended up rewriting the code to handle resignation without an exception, and the runtime of the application came down to just 14% of the original.

Intentional code paths are significantly more performant than clever exception catch logic.

Performance aside, there is also a good practice argument here. We've stopped using goto statements because they lead to spaghetti code where the logical flow is incredibly hard to follow. Clever exception catching is effectively using the same goto approach, albeit under a different name.

However, that doesn't mean that you should never catch exceptions.

So why do we still use catch logic?

Because some exceptions are either unexpected or unavoidable. Some examples:

• Unexpected - The database server is down. We did not foresee this, so we couldn't prepare for it accordingly. Even if we were to write a manual check to see if the database server is online before we send our query, it's still possible that the server goes offline between our check and our actual query, so we can't actually prevent this problem from occurring. At this point, the minor performance hit of handling the exception is not as much of a problem, the priority is handling this exception gracefully and informing the user/admin of the situation.
• Unavoidable - Divide by zero exception. If you have a double Divide(double a, double b) method, it's impossible to return a value from it (i.e. a double) when b is zero, because whoever called the method cannot differentiate a correct number from an error number. The only option is to make it clear that you could not do the expected task (dividing by zero) is to raise an exception, because you specifically do not want to return any number.
• Avoidable - IndexOf() is a good example of a method which can avoid throwing an exception. Indexes are inherently postitive numbers. Therefore, when IndexOf() returns a negative value, you know something is wrong. This is why IndexOf() returns -1 when it can't find the specified element, as opposed to throwing a ElementNotFoundException. It's functionally equivalent, but returning -1 is considerably more performant.

In this catch statement, I am then creating an error folder, and a file with data which caused issue. Where should I relocate these statements?

The first question you should ask yourself is how expected this error is.

If it's a commonly occurring error, you should not be using an exception. You should be checking for this erroneous state manually, and if the state is found to be erroneous, handle it accordingly.

You can get away with using an exception, if (and only if):

• Invalid data states are not common, they are rare fringe cases.
• There are many unforeseen possible errors (many reasons why a text line could not be parsed correctly), and you intend to catch all of them - not just your one specific issue. In other words, trying to write manual validation logic for every possible error state has become an exercise in futility because of the sheer amount of possible issues with the data.
• The performance hit of using an exception is not as important as being able to handle unexpected issues gracefully.

In most cases, you should be handling this manually. There are only few cases where all the criteria are met where it is acceptable to handle exceptions.

In either case, I suggest abstracting your catch logic. In your example:

catch
{
    new ErrorLogger(filepath).WriteError(line);
}

This is an oversimplified example. The core idea is that you can hide the error handling logic, as it makes the rest of the method (the "happy" part of the method) longer and harder to parse. Abstracting the error handling logic improves both your readability and your separation of concerns.

---

Unrelated to the core of my answer;

• NEVER use catch (this is called Pokémon exception handling). I can't think of a single cases where you somehow want to catch every possible exception, yet are completely disinterested in what the exception actually is.
• Try to avoid using catch(Exception) (this is still Pokémon exception handling) unless you know you want to catch literally everything that could ever go wrong. Spoiler alert: in my 10 years of professional programming (i.e. using good practice), I've come across a single situation where I truly wanted to catch everything: To prevent exceptions from leaking to the consumer of my API.
• Try to always use catch(ASpecificExceptionType) so that you don't catch things you're not interested in handling.