1

I have method which check for a recorded image name in a database. If there is such I try to load the image with the recorded path. If there isn't I load a default image.

First I had my whole method in a try-catch block where catch(Exception ex) and no matter what was the exception I just returned Error loading image :

    if (File.Exists(imgPath + "\\" + imageName))
    {
        try
        { 
            using (var temp = new Bitmap(imgPath + "\\" + imageName))
            {
                pictureBox1.Image = new Bitmap(temp);
            }

            if (pictureBox1.Image.Width > defaultPicBoxWidth)
            {
                pictureBox1.Width = defaultPicBoxWidth;
            }
        }
        catch (Exception ex)
        {
            logger.Error(ex.ToString());
            MessageBox.Show("Error loading image!", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
        }
    }

Then I figured out that sometimes I may have a record in the database but for some reason the file might be missing so I add check for that :

if (imageName != null && !File.Exists(imgPath + "\\" + imageName))

Now I need a proper message for this case too. I get to the conclusion that I can either - use several try-catch blocks to handle those parts or throw an exception and handle the exception from where the method is called.

I chose the second option and now the whole code is :

    if (imageName != null && !File.Exists(imgPath + "\\" + imageName))
    {
        throw new FileNotFoundException(); 
    }

    if (File.Exists(imgPath + "\\" + imageName))
    {
        //try
        //{ 
            using (var temp = new Bitmap(imgPath + "\\" + imageName))
            {
                pictureBox1.Image = new Bitmap(temp);
            }

            if (pictureBox1.Image.Width > defaultPicBoxWidth)
            {
                pictureBox1.Width = defaultPicBoxWidth;
            }
        //}
        //catch (Exception ex)
        //{
        //    logger.Error(ex.ToString());
        //    MessageBox.Show("Error loading image!", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
        //}
    }

And this is where I call the method :

try
                {
                    //The name of the method described above
                    LoadSavedOrDefaultImage(imageInfo, entity.Picture, txtCode.Text, imageLocation);
                }
                catch (FileNotFoundException ex)
                {
                    logger.Error(ex.ToString());
                    MessageBox.Show("Error loading image! The file wasn't found.", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
                }
                catch (Exception ex)
                {
                    LogErrorAndShowMessage(ex, Resources.ERROR_LOAD);
                }

I like this better but I can't say exactly why. The general question is - is this correct way to handle exceptions. And the more specific one - in my exact case where is the better place to put the try-catch blocks? It makes sense to me to be in the body of the method itself because that way I won't need to write those try-catch block everywhere I call the method for me it's even more encapsulated this way. Also now there are two try-catch blocks but if in the future the logic changes I might want to throw more different exceptions, which is another reason to keep the exception handling in the method itself rather where it's called but on the other hand... I want to read your opinion.

Cody Gray - on strike
  • 239,200
  • 50
  • 490
  • 574
Leron
  • 9,546
  • 35
  • 156
  • 257

2 Answers2

3
  1. In response to your general question, "where is the best place to handle exceptions", the answer is a simple one: always handle exceptions closest to the point where they are raised.

    That is where you will have the most information necessary to handle the exception, and it also keeps down code dependencies. If you let the exceptions bubble up too many levels, that higher-level code will have to know the implementation details of the lower-level code, which increases coupling and destroys abstraction.

    And that brings us to another important point: if you don't have enough information available to handle the exception, or you don't know what to do, you should not handle the exception at all. Instead, you should let it bubble up until it eventually reaches code that can handle it, or failing that, a global exception handler that displays an error message and/or writes to a log file and/or sends a dump back to the developer(s) before ending the application gracefully. Exceptions are not like Pokemon; you aren't supposed to catch 'em all. Only catch the ones you know how to deal with. (Related reading on global exception handling.)

  2. In response to your specific question, "how do I handle the case where a file/record/object is not found", the answer is also relatively straightforward: always handle the exception.

    Checking that the object exists first seems like a good idea at first blush. It seems like defensive programming, not attempting to do something you know will fail. And we all know that defensive programming is a best practice.

    So what's the problem? A relatively subtle one, known as a race condition. See, just because you ensure that the object exists before you try to access it, the object could still disappear between the time that you verify its existence and the time that you try to access it. In pseudo-code:

    if (!File.Exists(myFile))
    {
        MessageBox("Sorry buddy, that's a no-go.");
    }
    else
    {
        // Uh-oh! The file got deleted!
        File.Open(myFile);  // fails
    }
    

    Now, of course, you're probably saying to yourself that this sounds like it would be extremely rare. How often is the object really doing to disappear (or you lose the ability to access it) in between executing two lines of code? Well, it's actually not that unlikely. Consider the case where the file is located on a network drive and the network cable suddenly gets unplugged. But even if it is extremely rare, well that's why it's called an exception: it's an exception[ally rare] condition.

    So the proper solution here is to just handle the exception, because even if you program defensively, you'll still need to handle the exception to handle race conditions. And we all know that unnecessary code duplication is bad.

Community
  • 1
  • 1
Cody Gray - on strike
  • 239,200
  • 50
  • 490
  • 574
1

You are going well with your second option. If you have parts in your code which are very unlikely to cause any exceptions (if at all), then you shouldn't embrace them in try-catch.
In my eyes, there are only few times where there is only "one way" to handle an exception. It is significant that you have those parts of your code be under guard which could cause your program to malfunction or even crash.
Additionally, using several try-catch-blocks to catch different exceptions is good, too. Sometimes, there are exceptions that you want to trait equally and not further distinguish them. And yet on other occurences, there are exceptions you don't expect at all, which you can then at least find using catch(Exception ex) { ... }

Maybe a little look-up on MSDN helps you.

bash.d
  • 13,029
  • 3
  • 29
  • 42