Link to home
Start Free TrialLog in
Avatar of rwheeler23
rwheeler23Flag for United States of America

asked on

What is the proper use of throw in a try catch segment using C#?

I am constantly looking for ways to improve my code. Today I am working an error trapping.  As an example, please look at the C# code below. I want this method to return either true or false depending on success. I am confused about the use of throw. My question is inside this try catch, do i need to include a return false prior to the throw? Do I need the throw at all? Will return false work just as well as the throw ex? Is there any reason to use one or the other?


public bool GetDatabaseNames()
{
    try
    {
        Log.Write("DEBUG: GetDatabaseNames", false);

        /* Get company database name */
        Controller.Instance.Model.GPCompanyDatabase = Dynamics.Globals.IntercompanyId.Value;

        /* Get GP system database name */
        Controller.Instance.Model.GPSystemDatabase = Dynamics.Globals.SystemDatabaseName.Value;
        return true;
    }
    catch (Exception ex)
    {
        string eMsg = "GP.Class.Library - Controller 002: ERROR" + ex.Message;
        if (Model.StackTraceWanted) eMsg += "\n" + ex.StackTrace;
        MessageBox.Show(eMsg);
        throw ex;
    }
}

Open in new window

Avatar of Éric Moreau
Éric Moreau
Flag of Canada image

to throw an exception to the caller. you should not call "throw ex" in your sample, just "throw". Otherwise, you break the chain and it is harder to debug
you need to throw the exception if your current method did not fully handle the error and you want the caller to know it
You shouldn't really be using a catch for program logic. Your method is expecting a boolean return type, so you should return either true or false, depending on your business needs. You should only use the catch block to deal with unexpected problems (i.e. Exceptions).

In your catch block, you're not really handling the Exception. You're just showing an arbitrary message (plus the stack trace) which probably isn't very helpful as it's likely to mean nothing to the end-user. It would make more sense if you logged it and then re-threw it.

Looking at your code, I'm guessing the only problems you're likely to encounter are null references, in which case you should probably check for them in logic - if something is null, then return false, otherwise return true.

You might also want to consider renaming your method - GetDatabaseNames() implies that your method would get (return) the database names, which it doesn't!
Avatar of rwheeler23

ASKER

The StackTraceWanted is a debugging tool. During development it is set to True so anything that causes the catch to fire will display the error message so I know where the error occurred. The GetDatabaseName is a True/False flag indicating whether the routine was able to obtain the database names which are gotten using SQL reflection. I am currently patching several projects together to eventually create a template for future projects. I will incorporate your advice into the final product.
I understand. The point I made about your current catch block, regardless of StackTraceWanted, is that it's pretty useless as it stands. It just gives the user a meaningless error. Showing of the MessageBox is NOT dependent on whether StackTraceWanted is set or not - that just dictates the content of the message (include the stacktrace or don't).

A more robust option for this kind of thing is proper logging. Using something like the nLog framework can help with this - you can then set what level of logging you want (Debug during development / Errors during production etc).
A general point.
try...catch error handling uses rather more CPU time and resorces than just a simple return.  As mentioned earlier only use try...catch when there is code that really could give problems unexpectedly, otherwise test variables for correctness before attempting to use.
*NO POINTS*

Basically what Andy and Chris are saying is that your method could be condensed to something like this:
bool AreDatabasesInitialized()
{
    Log.Write($"DEBUG: {nameof(AreDatabasesInitialized)}", false);

    if (Controller?.Instance?.Model != null)
    {
        Controller.Instance.Model.GPCompanyDatabase = Dynamics?.Globals?.IntercomanyId?.Value;
        Controller.Instance.Model.GPSystemDatabase = Dynamics?.Globals?.SystemDatabaseName?.Value;
    }

    return Controller?.Instance?.Model?.GPCompanyDatabase != null && Controller?.Instance?.Model?.GPSystemDatabase != null;
}

Open in new window

-saige-
That is much more understandable. Especially if you don't look at the code for six months.
This question needs an answer!
Become an EE member today
7 DAY FREE TRIAL
Members can start a 7-Day Free trial then enjoy unlimited access to the platform.
View membership options
or
Learn why we charge membership fees
We get it - no one likes a content blocker. Take one extra minute and find out why we block content.