1

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;
}
3

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;
    }
}
  • Hi John, accepted your answer, also, what is wrong with my answer code below? It is voted to be deleted, want to understand why, just started programming – CarSpeed87 Sep 24 '18 at 0:01
  • While this somewhat cleans up the code, it ignores the main reason for not using exception handling logic: performance. You're still using exception handling, albeit with better separated logic. You're not actually addressing any of the underlying concerns which leads people to consider execssive exception handling as bad practice. Secondly, there are many arguments against using catch without any particular exception type. When would you ever want to catch literally every exception, yet also not care about the thrown exception or its message? – Flater Sep 24 '18 at 6:59
  • Also, you could achieve the same result without doing the boolean return value: catch { DoSomethingComplicatedToHandleAProblem(); }. The logic is still abstracted in this case. I see little purpose for adding an additional layer of boolean return values. First of all, it only works for methods which otherwhise have no return value, and secondly, it's not necessary when you can just handle the exception when you catch it (why defer it?) – Flater Sep 24 '18 at 7:07
  • Thanks @Flater for your feedback. I am answering the OP's specific question "I was told catch statements should not be utilized to handle other business logic. Where should I relocate these statements?" under the assumption that the design decision to use exceptions is a given. The code I provided is the simplest possible example to convey the flow of control, and would require simple modifications for the data flows you are describing. (FWIW I would probably lean toward using TryParse here.) – John Wu Sep 24 '18 at 22:00
  • @JohnWu You're still performing business logic after catching an exception. You've just shuffled it around a bit so that it's harder to spot. Obfuscating code is not an improvement. The feedback given to OP was not just a stylistic suggestion, there are important technical considerations here, which your answer leaves untouched. – Flater Sep 25 '18 at 3:37
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.

  • would the logging library, print out a simple text file for the Customer, showing which lines cannot be parsed? The customer wants a simple report, I just started programming, and heard logging library gives too much complicated information, I want simple report for customer, additionally, anyone you prefer? I will just stick with log4net, seems to be wide used, Thanks- – CarSpeed87 Sep 23 '18 at 2:06
  • 1
    Logging framework give you many information including stack trace and etc but you can choose which information should be logged. but in your case you should just log as simple : File.AppendAllText(path, line); – Mojtaba Tajik Sep 23 '18 at 3:34
1

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.

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.