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

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