Is it bad practice to have a `try-catch` inside a method?

Question

class Program
    {
        public static void Main(string[] args)
        {
            try 
            {
                checkifStringNull(string check)
                Clean();
            }
            catch(ArgumentNullException msg)
            {
                Console.WriteLine(msg.Message);
            }


        }
        public static int Clean()
        {
            List<string> errorLog = new List<string>();

            foreach (DirectoryInfo dir in directories)
            {
                try
                {
                        dir.Delete(true);
                }

                catch(System.UnauthorizedAccessException msg)
                {
                        code = 5;
                        errorLog.Add(String.Concat(dir.FullName," ", code ," ", msg.Message));
                        Console.WriteLine("Error Removing the directory: {0}", dir.FullName);

                }
                catch(System.IO.IOException msg)
                {
                        code = 5;
                        errorLog.Add(String.Concat(dir.FullName," ", code ," ", msg.Message));
                        Console.WriteLine("Error Removing the directory: {0}", dir.FullName);

                }
            }
        }

        public static void checkifStringNull(string check)
        {
            if(string.IsNullOrWhiteSpace(check))
            {
                throw new ArgumentNullException(String.Format("String can't be null"));
            }

        }

I've got a method that deletes the sub directories for a specific path. Is it okay if I have my try catch inside the method? The reason is I want direct access to the variable(directory) inside the loop that threw the exception and for me it's more organized. If I were to remove the code from the method and place it inside main, it would look like this:

public static void Main(string[] args)
{

    try 
    {
        checkifStringNull(string check)
        List<string> errorLog = new List<string>();

    foreach (DirectoryInfo dir in directories)
    {
        try
        {
                dir.Delete(true);
        }

        catch(System.UnauthorizedAccessException msg)
        {
                code = 5;
                errorLog.Add(String.Concat(dir.FullName," ", code ," ", msg.Message));
                Console.WriteLine("Error Removing the directory: {0}", dir.FullName);

        }
        catch(System.IO.IOException msg)
        {
                code = 5;
                errorLog.Add(String.Concat(dir.FullName," ", code ," ", msg.Message));
                Console.WriteLine("Error Removing the directory: {0}", dir.FullName);

        }
    }

    }
    catch(ArgumentNullException msg)
    {
        Console.WriteLine(msg.Message);
    }


}

I have a try-catch embedded inside another try-catch, is that considered bad practice or inefficient?

Answer 1

There's nothing wrong with having try-catch in a method, and there's nothing wrong with having try-catch inside another try-catch.

The only thing that matters, is whose job it is to handle exceptions.

Sometimes you have a method whose only job is attempt to perform a task, and not deal with unusual conditions. That method shouldn't catch exceptions.

Other times you have a method which you want to deal with many or even all exceptional cases, and not return control otherwise. This method would catch exceptions, and might have a lot of code to do so, possibly with nested try-catchblocks.

Real life works the same way. Sometimes you want someone to try and get you coffee, but if it doesn't work out (the place is closed, the line was really long) they should just come back and you'll deal with it. Other times you want them to go out for coffee, and not come back empty handed. Both are perfectly legitimate; it just depends on what you want to happen. It's completely a case by case basis.

A lot of beginners (and beyond!) make the mistake, of thinking they should catch all exceptions and never throw one. That would be like a babysitter, thinking it was their job to handle any unusual situation that comes up. Probably true for spilled soda. But when the curtains are on fire, you might prefer the exception be handled by someone else.

Answer 2

Generally - no issue, you can throw or catch anywhere and everywhere if there is a reason to do that and you're actually doing something in a catch that other catches aren't doing or if the throw is actually needed, usually to interrupt the current flow

In your example you have a loop that deletes folders. If one certain folder doesn't delete you may still want to continue deleting, so you can check for IOException or UnauthorizedAccessException (no need of System. prefix, put using System; above the class)

That, however can't be said for your ArgumentNullException - if you have a condition you can just check the condition and avoid throwing an exception. Throwing an exception is not very high-performance operation and should generally be avoided when there is a low-cost alternative, such as if:

if (!string.IsNullOrEmpty(theString)){
  doSomeWork();
} else {
  didNotDoTheWork();// such as Console.WriteLine("Error: theString is empty");
}

Another mild issue with your sample is that you're catching two different exceptions but doing the exact same thing. Instead you can do this:

try
{
  dir.Delete(true);
} 
catch(Exception x)
{
  if (x is UnauthorizedAccessException || x is IOException){
    Console.WriteLine("Error Removing the directory: {0}", dir.FullName);
  }
  else throw;
} 

with C# 6.0 you (VS 2015) you can also do:

try
{
  dir.Delete(true);
} 
catch(Exception x) when (x is UnauthorizedAccessException || x is IOException)
{ 
  Console.WriteLine("Error Removing the directory: {0}", dir.FullName); 
}