Re: Where to use Try/Catch block? [modified] - C# Discussion Boards

Where to use Try/Catch block? [modified]

14-Sep-11 2:14

Have a look at the documentation for CultureInfo[^] and related classes.

Adam_Dev13-Sep-11 22:21

13-Sep-11 22:21

Hi
First of all please forgive me, I seem to be having a bit of a mental block! I've been trying to improve my exception handling and error logging.

I have a function ProcessPerson(). Within that function I call a ValidateUser() function. And within that function I call the GetAdmin() function. Would the following be acceptable or if not, how should it be done?

I have omitted unimportant code, so:

//Class 1:
public void ProcessPerson(Request arg)
{
    try
    {
        if(Class2.ValidateUser(arg.Username, arg.Role)
        {
            //Do something
        }
        else
        {
            //log error
        }
    }
    catch(ArgumentNullException ex)
    {
        //log error
    }
    catch(Exception ex)
    {
        //log error
    }
}

//Class 2:
public static bool ValidateUser(string Username, string Role)
{
    try
    {
        using(MyClass myClass = new MyClass())
        {
            if(myClass.GetAdmin(Username, Role)
                return true;
            else
                return false;
        }
    }
    catch
    {
        throw;
    }
}

// MyClass
public bool GetAdmin(string Username, string Role)
{
    // should I do this?
    if(String.IsNullOrWhitespace(Username) || String.IsNullOrWhitespace(Role))
    {
        throw new ArgumentNullException("e.g. Username or Role cannot be null");
    }
    
    try
    {
        using(MyClassAdapter myClassAdapter = new MyClassAdapter ())
        {
            myClassAdapter.GetAdmin(Username, Role); 
        }
    }
    catch(ArgumentNullException ex)
    {
        throw;
    }
    catch(Exception ex)
    {
        throw;
    }
}

Appreciate any help or advice.

Thanks

modified on Wednesday, September 14, 2011 5:01 AM

BobJanova13-Sep-11 22:40

Re: Where to use Try/Catch block? [modified]

13-Sep-11 22:40

try { ... } catch { throw; }

... is by definition pointless. Are you logging within that catch block? I don't think you should do that, though, because it will result in exceptions getting double-logged.

I would say you should trap exceptions at the point where it makes sense to handle them. You can make an argument for that being in ValidateUser (any exception should be 'invalid' i.e. return false), or at the top level call which might produce one (at which time you log the error). But don't catch it in both places.

You should only throw an ArgumentNullException if the argument is actually null – if you want to fail on empty too, it should be an ArgumentException. But 'guards' at the top of a method like that are fine.

Adam_Dev13-Sep-11 23:01

13-Sep-11 23:01

Thanks BobJanova.

The only reason I was using "try { ... } catch { throw; }" was to make sure the original exception was passed on to the top calling method that would handle the exception and log where the exception occurred.

Regarding ValidateUser, you're right, any exception should be invalied (return false) but I know you're not mean to return anything within the catch block correct?

What would you say is best, preempting an error by having a guard at the top or letting the error happen and catching it? I know it's swings and roundabouts really but it feels that if I know the arguments might be null or empty, I should test for it first?

Is this better?:

//Class 1:
public void ProcessPerson(Request arg)
{
    try
    {
        if(Class2.ValidateUser(arg.Username, arg.Role)
        {
            //Do something
        }
        else
        {
            //log error
        }
    }
    catch(Exception ex)
    {
        //log the error
    }
}
 
//Class 2:
public static bool ValidateUser(string Username, string Role)
{
    using(MyClass myClass = new MyClass())
    {
        if(myClass.GetAdmin(Username, Role)
            return true;
        else
            return false;
    }
}
 
// MyClass
public bool GetAdmin(string Username, string Role)
{
    try
    {   
        if (String.IsNullOrWhiteSpace(UserName) || String.IsNullOrWhiteSpace(Role))
                    throw new ArgumentException("Username/Role cannot be empty or null");

        using(MyClassAdapter myClassAdapter = new MyClassAdapter())
        {
            myClassAdapter.GetAdmin(Username, Role); 
        }
        return true;
    }
    catch(ArgumentException ex)
    {
        //log the error
    }
    catch(Exception ex)
    {
        //log the error
    }

    return false;
}

If not, are you able to show me what would be best please?

Thanks a lot for your help.

modified on Wednesday, September 14, 2011 5:14 AM

BobJanova14-Sep-11 2:09

14-Sep-11 2:09

I don't think there's anything particularly wrong with returning from a catch. The rule of thumb is that (as Pete says below) you should catch an exception where you intend to deal with it, and if you consider it dealt with you can return from the function. In this case, the calling code will not know that there was an exception in some lower layer. If you think the calling code should be informed, don't catch the exception and let the calling code deal with it.

What you've done in GetAdmin makes no sense. You're catching the exception you're throwing and doing nothing with it! Don't use exceptions to control the flow of code. If an empty or null user name/role is an error (i.e. in normal operation it should never happen), then throw the exception; if you want it to be valid input, but not an admin, then return false.

The only reason I was using "try { ... } catch { throw; }" was to make sure the original exception was passed on to the top calling method that would handle the exception and log where the exception occurred.

If you don't catch an exception, it automatically propagates up the stack. Handle it where it makes sense to do so, and once only. (Not always true, but generally you should not rethrow an exception.)

Since you included it originally I'm inclined to believe that the null or empty username is an error and GetAdmin, at least, should throw in that case. That means that, logically, it's invalid input to ValidateUser as well, I think, so I'd catch the error at the request level:

// Class1
public void ProcessPerson(Request arg)
{
    try
    {
        if(Class2.ValidateUser(arg.Username, arg.Role)
        {
            //Do something
        }
        else
        {
            // log failure to authenticate
        }
    }
    catch(Exception ex)
    {
        //log the error and create a nice message to the user
    }
}
 
//Class 2:
public static bool ValidateUser(string Username, string Role)
{
    using(MyClass myClass = new MyClass())
    {
        // if ( x ) return true else return false is redundant
        return myClass.GetAdmin(Username, Role);
    }
}
 
// MyClass
public bool GetAdmin(string Username, string Role)
{
    if (String.IsNullOrWhiteSpace(UserName) || String.IsNullOrWhiteSpace(Role))
                throw new ArgumentException("Username/Role cannot be empty or null");

    using(MyClassAdapter myClassAdapter = new MyClassAdapter())
    {
        return myClassAdapter.GetAdmin(Username, Role); 
    }
}

It also seems like there are far too many layers involved in the authentication process. You have Class1 (which receives the request), Class2, MyClass and MyClassAdapter all involved in finding out this simple piece of information (whether a user/role combination is admin). What I typically do is at login time I populate a single UserInfo object which contains the role and user-specific access overrides, and then I can just authenticate like:

if(user != null && user.HasAccess(AccessTypes.Admin)) { ... }

(setting user to null if no user is logged in). HasAccess would look a bit like:

public bool HasAccess(AccessTypes access){
 return access == access & (Access | role.Access & ~RemovedAccess);
}

I'm not sure if you can do that with enums but logically that is what AccessTypes would be, and UserInfo.Access, UserInfo.RemovedAccess and Role.Access would all be of that type. You might have to cast to int or uint to get the bit matching to work.

Re: Where to use Try/Catch block? [modified]

Pete O'Hanlon13-Sep-11 22:57

Pete O'Hanlon

13-Sep-11 22:57

To be honest, the catch statements in your ValidateUser and GetAdmin methods are completely redundant. In general, if you find yourself using catch/throw without doing anything in there, you have poor code. You should only catch exceptions if you plan to do something with them, e.g. reattempt to acquire a transient resource that you couldn't get and which triggered the exception.

Forgive your enemies - it messes with their heads

My blog | My articles | MoXAML PowerToys | Mole 2010 - debugging made easier - my favourite utility

Adam_Dev13-Sep-11 23:10

13-Sep-11 23:10

Thanks Pete.

Would my above post be any better? Or closer to better coding?

I'm just confused really, I think I've read too many articles on exceptions (or maybe not enough infact!). Some have said to deal with exceptions in the method that was called, others have said to let exceptions propagate to the top calling method.

(Or an alternative to the GetUser() function?)

// MyClass
public bool GetAdmin(string Username, string Role)
{
    try
    {   
        if (String.IsNullOrWhiteSpace(UserName) || String.IsNullOrWhiteSpace(Role))
        {
            LogEntry.New("Username/Role cannot be empty or null");
            return false;
        }

        using(MyClassAdapter myClassAdapter = new MyClassAdapter())
        {
            myClassAdapter.GetAdmin(Username, Role); 
        }
        return true;
    }
    catch(Exception ex)
    {
        //log the error
    }
 
    return false;
}

modified on Wednesday, September 14, 2011 5:27 AM

Pete O'Hanlon13-Sep-11 23:50

Pete O'Hanlon

13-Sep-11 23:50

OK, the simple way to think of this is to only catch an exception at the point at which you intend to do something with it. In the case you have here, you are logging the fact that something went wrong, but does it make sense to allow processing to continue? That's the question you have to ask whenever you are attempting to place exception handling in your code.

For instance, suppose you have some code to write something over TCP, and there's a failure at one point which throws an exception, do you want to attempt to reacquire a connection or abort the processing of that message at that point? The answer to that question indicates whereabouts you want to place your exception handling - if you want to reacquire, you'd handle the exception at a lower level than you would if you just wanted to abort the whole thing and tell the user that there was a problem.

Forgive your enemies - it messes with their heads

My blog | My articles | MoXAML PowerToys | Mole 2010 - debugging made easier - my favourite utility

Adam_Dev14-Sep-11 0:26

14-Sep-11 0:26

Thanks Pete, your advice is really helpful.

Ideally when an error occurs I want to log what happened and exit. I guess I could just have one single Try/Catch block in the top calling function and catch either a custom exception and then the general Exception or maybe just the general exception? Would this be acceptable? It just doesn't seem like a clean exit, as opposed to returning false after catching an error, but that may be a misconception?

Thanks.

Luc Pattyn14-Sep-11 2:11

Luc Pattyn

14-Sep-11 2:11

You could have all your methods contain try-catch constructs, and return a success/failure result somehow (that is what the Win32 API has done for as long as Windows exists); the disadvantage is you have to provide all the code to do that, *and* your callers have to explicitly check each and every return value.

Or you could take full advantage of the Exception mechanism, which basically says: if something goes wrong, an Exception will be thrown, and some piece of code is bound to catch it, either in the method itself, or one of the callers higher up in the calling chain; if there are no catchers, then the app will exit. You get all this for free.

So yes, a single try-catch at the top level would allow you to log the exception and exit, although that would be an ungraceful exit (and an app I would not like to use). As others have said, use a local try-catch if and when your catch block is able to resolve the problem, and leave the exception as is if you can't resolve it at that level.

And yes, input checking and throwing explicitly on bad input values is the normal thing to do.

And yes, having a catch block that just contains throw is pointless; having a catch block that contains logging and throw normally results in multiple logging. What I sometimes do is catch an exception, and throw a different exception (a "smarter" one, one that fits the usage domain of the method) while putting the original exception in the new one's InnerException. So a "File Not Found" situation deep inside a simulator program may end up as a "Simulation failed" exception, where the tree of inner exceptions ends up explaining why or how it failed.

FWIW: I would recommend you study the topic in more detail in a book on .NET in general, or on C#; an introductory book should contain a number of examples of all of the above. And I do mean a physical book, not an eBook or some web course.

Smile | :)

Luc Pattyn [My Articles] Nil Volentibus Arduum

BobJanova14-Sep-11 2:22

14-Sep-11 2:22

Generally an excellent post. I'd just mention that it's good to be careful about your use of catch-throw different, because it makes debugging quite a bit harder if your IDE is set up to trip on where the exception was thrown, because you get stopped in the context of the catch handler and don't have access to the state at the point of the initial failure. It can be good but don't overdo it (as the folks who wrote the app I'm working on seem to have done).

Adam_Dev14-Sep-11 2:33

14-Sep-11 2:33

Thanks Luc, your reply was really useful. I think I have a goodish idea of how to proceed (I hope). I'll make sure to get a good introductory book to give me a better understanding.

I should point out that when I said that I'd log the exception and then exit, I meant log the exception and then return a message to the user. Would that still count as an ungraceful exit if I had a single try/catch block at the top level out of interest?

Thanks