tags:

views:

293

answers:

6

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

    }
}

}

+2  A: 

Exceptions and Exception Handling (C# Programming Guide)

Exception Handling (C# Programming Guide)

Philip Wallace
+1. Good link. Recommended reading.
David Stratton
+11  A: 

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 A
+1. Exactly what I was about to post. Almost word-for-word.
David Stratton
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
@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 A
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
A: 

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  A: 

Good article from eric lippert on what to catch: Fabulous Adventures In Coding: Vexing Exceptions

David_001
A: 

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
A: 

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

For you particular examples, File.Delete can throw a number of exceptions, including IOException, UnauthorizedAccessException as well as others. 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).

Ryan Elkins