51

I've done my reading and understand what a Try/Catch block does and why it's important to use one. But I'm stuck on knowing when/where to use them. Any advice? I'll post a sample of my code below in hopes that someone has some time to make some recommendations for my example.

    public AMPFileEntity(string filename)
    {
        transferFileList tfl = new transferFileList();
        _AMPFlag = tfl.isAMPFile(filename);
        _requiresPGP = tfl.pgpRequired(filename);
        _filename = filename.ToUpper();
        _fullSourcePathAndFilename = ConfigurationSettings.AppSettings.Get("sourcePath") + _filename;
        _fullDestinationPathAndFilename = ConfigurationSettings.AppSettings.Get("FTPStagePath") + _filename;
        _hasBeenPGPdPathAndFilename = ConfigurationSettings.AppSettings.Get("originalsWhichHaveBeenPGPdPath");
    }


    public int processFile()
    {

        StringBuilder sb = new StringBuilder();
        sb.AppendLine(" ");
        sb.AppendLine("    --------------------------------");
        sb.AppendLine("     Filename: " + _filename);
        sb.AppendLine("     AMPFlag: " + _AMPFlag);
        sb.AppendLine("     Requires PGP: " + _requiresPGP);
        sb.AppendLine("    --------------------------------");
        sb.AppendLine(" ");

        string str = sb.ToString();
        UtilityLogger.LogToFile(str);
        if (_AMPFlag)
        {
            if (_requiresPGP == true)
            {
                encryptFile();
            }
            else
            {
                UtilityLogger.LogToFile("This file does not require encryption. Moving file to FTPStage directory.");
                if (File.Exists(_fullDestinationPathAndFilename))
                {
                    UtilityLogger.LogToFile(_fullDestinationPathAndFilename + " alreadyexists. Archiving that file.");
                    if (File.Exists(_fullDestinationPathAndFilename + "_archive"))
                    {
                        UtilityLogger.LogToFile(_fullDestinationPathAndFilename + "_archive already exists.  Overwriting it.");
                        File.Delete(_fullDestinationPathAndFilename + "_archive");
                    }
                    File.Move(_fullDestinationPathAndFilename, _fullDestinationPathAndFilename + "_archive");
                }
                File.Move(_fullSourcePathAndFilename, _fullDestinationPathAndFilename);
            }
        }
        else
        {
            UtilityLogger.LogToFile("This file is not an AMP transfer file. Skipping this file.");
        }

            return (0);
    }


    private int encryptFile()
    {

        UtilityLogger.LogToFile("This file requires encryption.  Starting encryption process.");


        // first check for an existing PGPd file in the destination dir.  if exists, archive it - otherwise this one won't save.  it doesn't overwrite.
        string pgpdFilename = _fullDestinationPathAndFilename + ".PGP";



        if(File.Exists(pgpdFilename))
        {
            UtilityLogger.LogToFile(pgpdFilename + " already exists in the FTPStage directory.  Archiving that file." );
            if(File.Exists(pgpdFilename + "_archive"))
            {
                UtilityLogger.LogToFile(pgpdFilename + "_archive already exists.  Overwriting it."); 
                File.Delete(pgpdFilename + "_archive");
            }
            File.Move(pgpdFilename, pgpdFilename + "_archive"); 
        }

        Process pProc = new Process();
        pProc.StartInfo.FileName = "pgp.exe";

        string strParams = @"--encrypt " + _fullSourcePathAndFilename + " --recipient infinata --output " + _fullDestinationPathAndFilename + ".PGP";

        UtilityLogger.LogToFile("Encrypting file.  Params: " + strParams);
        pProc.StartInfo.Arguments = strParams;
        pProc.StartInfo.UseShellExecute = false;
        pProc.StartInfo.RedirectStandardOutput = true;
        pProc.Start();
        pProc.WaitForExit();

        //now that it's been PGPd, save the orig in 'hasBeenPGPd' dir
        UtilityLogger.LogToFile("PGP encryption complete.  Moving original unencrypted file to " +  _hasBeenPGPdPathAndFilename); 
        if(File.Exists(_hasBeenPGPdPathAndFilename + _filename + "original_which_has_been_pgpd"))
        {
            UtilityLogger.LogToFile(_hasBeenPGPdPathAndFilename + _filename + "original_which_has_been_pgpd already exists.  Overwriting it.");
            File.Delete(_hasBeenPGPdPathAndFilename + _filename + "original_which_has_been_pgpd");
        }
            File.Move(_fullSourcePathAndFilename, _hasBeenPGPdPathAndFilename + _filename + "original_which_has_been_pgpd");

        return (0);

    }
}

}

dreftymac
  • 31,404
  • 26
  • 119
  • 182
fieldingmellish
  • 1,473
  • 3
  • 16
  • 21
  • 3
    Search here for 'try catch c#': http://stackoverflow.com/questions/523875/where-to-put-try-catch http://stackoverflow.com/questions/751744/thoughts-on-try-catch-blocks http://stackoverflow.com/questions/505471/how-often-should-i-use-try-and-catch-in-c and so no... – Lazarus Nov 12 '09 at 15:23
  • There are tons of questions about exception handling on this site! – Philip Wallace Nov 12 '09 at 15:27
  • I know there are a million places where I could read up on exception handling. Indeed I mentioned in my post that I have understand the concept. I'm stuck at implementation right now and the purpose of my post was to hopefully get someone to show me how to apply it to my particular circumstance, my specific code example. – fieldingmellish Nov 12 '09 at 15:30
  • 3
    Good article from eric lippert on what to catch: [Fabulous Adventures In Coding: Vexing Exceptions](http://blogs.msdn.com/ericlippert/archive/2008/09/10/vexing-exceptions.aspx) – David_001 Nov 12 '09 at 15:29

4 Answers4

114

The basic rule of thumb for catching exceptions is to catch exceptions if and only if you have a meaningful way of handling them.

Don't catch an exception if you're only going to log the exception and throw it up the stack. It serves no meaning and clutters code.

Do catch an exception when you are expecting a failure in a specific part of your code, and if you have a fallback for it.

Of course you always have the case of checked exceptions which require you to use try/catch blocks, in which case you have no other choice. Even with a checked exception, make sure you log properly and handle as cleanly as possible.

Yuval Adam
  • 161,610
  • 92
  • 305
  • 395
  • 7
    In my opinion, logging every exception is a good idea, as long as you do it high up in the call stack and add a "throw;" statement right after. – Manu Nov 12 '09 at 15:25
  • 3
    @Manu - that is not always the way to go. Imagine you are a sys admin, and you open a log file only to find hundreds of useless traces all saying "exception in XXX". This has a big impact on TCO, and usually does not have any benefit. Log in the most appropriate place so that a sys admin has a meaningful trace to work with. – Yuval Adam Nov 12 '09 at 15:29
  • 2
    IIS keeps a log of all exceptions thrown, and there are plenty of solutions out there (i.e. ELMAH) that will log all exceptions without cluttering your code. – Russ Bradberry Nov 13 '09 at 00:54
  • 1
    Catching an exception at the source lets you supply metadata to logs that is very useful in determining why the exception occurred in the first place. Handling is only one reason to catch; the other (arguably more important) reason is forensics and debugging, which a stack trace will very often be of limited help with. – Jeremy Holovacs Oct 03 '19 at 12:22
9

Like some others have said, you want to use try-catch blocks around code that can throw an Exception AND code that you are prepared to deal with.

Regarding your particular examples, File.Delete can throw a number of exceptions, for example, IOException, UnauthorizedAccessException. What would you want your application to do in those situations? If you try to delete the file but someone somewhere else is using it, you will get an IOException.

try
{    
    File.Delete(pgpdFilename + "_archive")
}
catch(IOException)
{
    UtilityLogger.LogToFile("File is in use, could not overwrite.");
   //do something else meaningful to your application
   //perhaps save it under a different name or something
}

Also, keep in mind that if this does fail, then the File.Move you do outside of your if block next will also fail (again to an IOException - since the file was not deleted it is still there which will cause the move to fail).

Matteo Ragni
  • 2,837
  • 1
  • 20
  • 34
Ryan Elkins
  • 5,731
  • 13
  • 41
  • 67
4

I was taught to use try/catch/finally for any methods/classes where multiple errors could occur and that you can actually handle. Database transactions, FileSystem I/O, streaming, etc. Core logic usually doesn't require try/catch/finally.

The great part about try/catch/finally is that you can have multiple catches so that you can create a series of exception handlers to deal with very specific error or use a general exception to catch whatever errors you don't see coming.

In your case, you're using File.Exists which is good, but their maybe another problem with the disk that may throw another error that File.Exists cannot handle. Yes, it's a boolean method, but say the File is locked and what happens if you try to write to it? With the catch, you can plan for a rare scenario, but without try/catch/finally, you may be exposing the code to completely unforeseen conditions.

osij2is
  • 1,546
  • 3
  • 12
  • 21
  • Some of what is said here isn't true - it's always useful to attempt to catch exceptions for FileIO, for instance - it allows the developer to notify the user that, for instance, there may not be enough space on the required drive to complete the save operation, or the file doesn't exist (if the dev hasn't coded checks in properly ... sadly which I *have* seen). – Paul May 25 '17 at 08:21
  • @Paul if you can code it as an exception you can also code it as a requirement in the beginning of the function. So the question is not as much one of if you have to use exceptions, but if there is no other alternative. Exceptions don't always make for cleaner code either, since you are moving the handling part away from the error. – Michiel van der Blonk Nov 30 '17 at 21:18
-2

The other guys have given quite a number of good pointers and references.

My input is a short one:
When to use it is one thing, equally or more importanly is how to use it properly.

PS: "it" is refeings to "trying-catching exceptions".

o.k.w
  • 25,490
  • 6
  • 66
  • 63