How to handle error in the calling method

Question

I don't have much experience with programming in a good OO way but I want to learn and today occurs a situation where I think I can improve my skills on error handling if I'm correct on my observations.

I have an event which code is logically separated in two parts :

private void btnSave_Click(object sender, EventArgs e)
{
    IList<AppConfig> AppConfigs = AppConfigService.All().Where(a => (a.IsActive == true) && (a.ConfigProperty == "MaterialImages")).ToList();
    string ImageNameFilter = txtCode.Text;

    if (!ucFileExplorer.IsSavedImage(AppConfigs, ImageNameFilter))
    {
        MessageBox.Show("Error");
        return;
    }

    if (Save())
    {
        LoadForm<Materials>();
    }
}

The second part is just calling the Save() method, but my question is about the first one. There I call a method which should copy an image selected with a OpenFileDialog and the whole logic is in my user control that I make for file browsing cause I use it in many forms. The method that actually do the coping is this:

public bool IsSavedImage(IList<AppConfig> AppConfigs, string ImageNameFilter)
{
    bool IsCopied = false;
    try
    {
        string imgPath = AppConfigs[0].ConfigValue.ToString();
        File.Copy(selectedFile, imgPath + "\\" + ImageNameFilter + slectedFileName + ".jpeg");
        IsCopied = true;
    }
    catch (IOException copyError)
    {
        MessageBox.Show("Error saving image!", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
        return false;
    }
    return IsCopied;
}

And here start my questions. I think I need the try-catch clause here, because this is where the actual execution takes place and makes it easier to return correct status (true or false). But I think the actual copyError should be handled in the calling method (my btn Click event in this case). So I have two questions, what is the right way to handle this exception, and if I want to pass it to the place where the method is called how can I do it?

Answer 1

I agree with dutzu that the actual error checking should be done in the business layer. Further to that, I would suggest that your UI component or form should expose an ErrorHandler event which you should raise whenever there is an error condition.

This way, if there is an error handler attached to the error event handler, that will be called and that error handler can take the appropriate action, if need be.

You can define an ErrorEventHandler delegate such as:

public void ErrorEventHandler(object sender, ErrorEventArgs e);

And an ErrorEventArgs class such as:

public class ErrorEventArgs : EventArgs
{
    // error info exposed via readonly properties
}

You can create an error event in your UI component/form like so:

public class UIForm
{
    public event ErrorEventHandler ErrorOccurred;

    protected void RaiseErrorEvent(ErrorEventArgs e)
    {
        ErrorEventHandler eh = ErrorOccurred;

        if (eh != null)
        {
            eh.Invoke(this, e);
        }
    }

    private void btnSave_Click(object sender, EventArgs e)
    {
        IList<AppConfig> AppConfigs = AppConfigService.All().Where(a => (a.IsActive == true) && (a.ConfigProperty == "MaterialImages")).ToList();
        string ImageNameFilter = txtCode.Text;

        try
        {
            if (!ucFileExplorer.IsSavedImage(AppConfigs, ImageNameFilter))
            {
                RaiseErrorEvent(new ErrorEventArgs());
                return;
            }

            if (Save())
            {
                LoadForm<Materials>();
            }
        }
        catch (Exception exception)
        {
            // convert exception to an ErrorEventArgs e here
            RaiseErrorEvent(e);
        }
    }
}

Answer 2

what is the right way to handle this exception

It really depends on your application; in general, you handle the exception where you need (or can!) handle it. For example, in the place where you can collect more information, and/or react to the error (display an error message, log, retry...)

and if I want to pass it to the place where the method is called how can I do it?

You just handle the exception at an higher level: exceptions are handy because they are propagated to the caller until someone catches them. Just insert the try... catch block in the calling function(s) (be careful to consider all the executions paths, or exceptions that will not find any function handling them will be uncaught, terminating the program).

Generally speaking, I remember four main ways of handling errors in an application: return values (error codes or booleans), exceptions, GetLastError-style (store an error code somewhere, have the programmer call a function to retrieve it), callbacks (provide a function to be called in case of error).

For a while, exceptions where said to be THE OO way of handling error. But you now have also event handlers (a special case of callbacks, after all) in well designed frameworks.

In general, a good design should choose one and stick to that, eventually "transforming" one error type to the other (for example, catching exception from thrid-party code and making them into a callback, as in Umar Farooq Khawaja answer)

Answer 3

I wouldn't catch the exception this way, as this is currently not consistent: your method catches only IOException exceptions, so it might throw other exceptions that won't ever be caught (not caught in IsSavedImage nor in calling method), such as UnauthorizedAccessException for instance.

The other issue I see is that you always display a message box in case of IOException exception. I would remove this part, as one day you might want to handle exceptions in different ways, and you don't want to change how it is currently handled in a core/business function.

For instance something like:

public bool SaveImage(IList<AppConfig> AppConfigs, string ImageNameFilter)
{
    string imgPath = AppConfigs[0].ConfigValue.ToString();
    File.Copy(selectedFile, imgPath + "\\" + ImageNameFilter + slectedFileName + ".jpeg");
}

...

try
{
    xxx.SaveImage(....);
}
catch (UnauthorizedAccessException)
{
    // Message to user that the access is denied?
    ...
}
catch (any exception you can handle properly)
{

}
catch
{
    // Couln't be handled properly, so displays generic error message
    ...
}

Answer 4

Do not show an error message dialog in the save function. Rename it to "SaveImage" with void return type and let the exception raise. No "if" in the caller but a try-catch block that shows the error message dialog in case an exception happened.

Answer 5

you can define a event in the user-control and fire it whenver you get a exception
and then subscribe to this exception event from the main parent and handle it accordingly.

Something like this.

public Action<Exception> OnExceptionInUserControl;
public bool IsSavedImage(IList<AppConfig> AppConfigs, string ImageNameFilter)
{
    bool IsCopied = false;
    try
    {
        string imgPath = AppConfigs[0].ConfigValue.ToString();
        File.Copy(selectedFile, imgPath + "\\" + ImageNameFilter + slectedFileName + ".jpeg");
        IsCopied = true;
    }
    catch (IOException copyError)
    {
        OnExceptionInUserControl(copyError);
        return false;
    }
    return IsCopied;
}

in the mainWindow

userControl.OnExceptionInUserControl += new Action<Exception>(ExceptionEventHandler);