Human-Debugger.net - Using Exception between two methods in the same class

Friday, May 12, 2006

Using Exception between two methods in the same class

In one of my code reviews I saw this piece of code (those two methods are in the same class), IMO, and I believe that most of you will agree with me, this usage of exception is unnecessary.

private void CheckPrivileges()
{
   // check user privileges
   // for authorized user won't do anything.

   // if not authorized:
   throw new UserNotPrivException("user name", "action not allowed");
}

public void DoSomething()
{
   try
   {
      CheckPrivileges();
      // Continue your logic for authorized users.

   }
   catch (UserNotPrivException)
   {
      // showing message box which
      // says that user has no permissions
      ShowMessageBox ("User X has no privileges for this place.");
   }
}

Why ?

The Exception mechanism is slow
A lot try&catch makes the code harder to read
Why throw exception if you won't use it? and also throwing it between two methods in the same class?

I would have done it like this:

private bool IsUserPrivileged()
{
   // check user privileges
   // for authorized user - return true
   // if not authorized - return false
}

public void DoSomething()
{
   if (!IsUserPrivileged())
   {
      // showing message box which
      // says that user has no permissions
      ShowMessageBox("User X has no privileges for this place.");
      return;
   }

   // Continue your logic for authorized users
}

If you want to check between those two (privileged or not) you can use the bool.
Moreover, when you are not really need the exception data.

Honestly,
I wrote this post before Tech-Ed, but didn't have enough time to publish it, in Tech-Ed, I talked with some other programmers that agreed with my opinion, but I still would like to hear your opinion,
what do you think about it?

Friday, May 12, 2006 11:10:06 AM (GMT Daylight Time, UTC+01:00)

Comments [5] - Trackback
.Net | Software Development

Friday, May 12, 2006 10:07:22 PM (GMT Daylight Time, UTC+01:00)

I agree, although there might be a reason to throw an exception, if you have an exception handling mechanism that logs all errors, and you want to know about the unauthorized access. If all you want to do is show a message - than definitely your way is the way to go.

Doron

Saturday, May 13, 2006 6:21:06 PM (GMT Daylight Time, UTC+01:00)

As I see it , it's not an error situation .
It's a check that you do in application ( as you would check 4==5 ) .

So that's a purpose of the CheckPriveleges/IsUserPriveleged method , and when you
call it , you know that this method returns you TRUE/FALSE .

So FALSE return value is not an error - it's one of the possible results of the check , and you aware that this is what you might get .

If for example you wanted to call some framework method like ReadFile or something ,
and you don't have the possibility to check the user permissions on the file ( or you don't want to do that at all ) , then the best way is to wrap the call
with a try-catch() statement .

Alex

Sunday, May 14, 2006 7:40:02 PM (GMT Daylight Time, UTC+01:00)

Thanks Doron and Alex, I agree with you both.
Moreover, I even changed the method name from CheckPrivileges to IsUserPrivileged which I think is a better design (readability).

Thanks both.

Shani Raba

Thursday, May 25, 2006 10:44:20 PM (GMT Daylight Time, UTC+01:00)

Hi,
I think that it's always (in this case of course) better to return a value without throwing exception, even in case that we want to log errors (or something else).
And in this case (that we want to log the error) we should use event, that we will raise it in the IsUserPrivilege function.
The event (something like OnUserNotPrivilege) will handle all the things that we want to do with the error (log, public…).
Good luck!

Avi Wortzel

Monday, May 29, 2006 10:21:43 PM (GMT Daylight Time, UTC+01:00)

Thanks Avi,
I am not sure i understood your point of view, but I'll post advanced usage of IsUserPrivileged (on the web page) & HandleVisibility.
So I hope we could here from you more at that point.

Shani Raba

Comments are closed.