views:

179

answers:

7

I've designed a simple program that has a counter class and inside that counter class I have methods that increment the count, deincrement the count, and so on. I then present a menu to the user and have them enter their choice, ex. enter 1 for option one, etc. Well, I don't ever want the count to be negative, so to handle this I had my SubtractCount method of the counter class throw an ArguemetOutOfRangeException when count < 0.

I then had my switch statement catch that exception if it occurred. My question is: Is it bad to use an exception to make sure the count can never be negative? Should I do it differently? Or better yet, is there a better way to accomplish this?

Code snippet:

static void Driver()
{
    switch (userChoice)
    {
        case 1: // if user picks a 1, the count is deincremented
            try {

                myObjectOfCounterClass.SubtractCount();
            }
            catch (ArguemetOutOfRangeException ex)
            {
                console.writeLine(ex.message);
            }
         break;
         // case 2, case 3 etc.
     }

class Counter
{
    private int count;

    public void SubtractCount()
    {
        if (count < 0)
            throw new ArguementOutOfRangeException("The count can never be negative!");
        else
            count--;
    } 
+1  A: 

Exception handling can be resource intensive. If you plan to throw the exception for the sake of catching it and suppressing it then don't do it. Instead do something like return a bool from the method to indicate success to the caller.

public bool SubtractCount() { 
   if (count < 0)
        return false;
   count--;
   return true;
}

Caller can still handle the situation with similar logic but without exceptions

if (!myObjectOfCounterClass.SubtractCount())
   Console.writeLine("The count cannot be negative");

That's the gist.

Throw exceptions when you don't want to handle them, or if you can't handle them or you want the error propagated outside your immediate programming context. It's fun to use features such as exception handling but they can be overdone - I'm often guilty of doing the same with interesting features in the .NET framework.

John K
This isn't a very clear API design. What happens if the return value is false? It also violates Command/Query Separation.
Mark Seemann
I think that if the method were named TrySubtractCount then it would be perfectly clear!
RCIX
As for proper architecture I prefer @Mark Seemanns' answer - http://stackoverflow.com/questions/1802228/c-how-to-handle-this-situation/1802250#1802250
John K
+7  A: 

Using exceptions for control flow is not considered good practice. In your case, the Tester-Doer pattern would be a better fit.

You could change your Counter class to something like this:

class Counter
{
    private int count;

    public bool CanSubtractCount
    {
        get { return this.count >= 0; }
    }

    public void SubtractCount()
    {
        if (!this.CanSubtractCount)
            throw new InvalidOperationException("The count can never be negative!");
        else
            this.count--;
    }

You can now rewrite your client like this:

static void Driver()
{
    switch (userChoice)
    {
        case 1: // if user picks a 1, the count is deincremented
            if(myObjectOfCounterClass.CanSubtractCount)
            {
                 myObjectOfCounterClass.SubtractCount();
            }
            else
            {
                 // Write a message to the user?
            }
         break;
         // case 2, case 3 etc.
     }
}
Mark Seemann
A good answer, but an important side point is that ArgumentOutOfRangeException should not be thrown, I would rather recommend InvalidOperationException or something derived from that.
AdamRalph
@AdamRalph: Agreed - InvalidOperationException is the correct exception type to throw. Edited my answer :)
Mark Seemann
+1 for good architecture in the answer
John K
I suppose I don't even need the throw an exception, just need to make a method of the counter class that returns a bool: public bool CanSubtractCount() { if (count <= 0) return false; else return true; }
Alex
A: 

Actually, it seems that situation when count < 0 is not "exceptional". Use normal "if" for this, there is no need for exception.

Vitaliy Liptchinsky
+1  A: 

You shouldn't use exceptions for flow control, when you could just use flow control. You can check for less than zero and either (a) not act on it or (b) show a message or (c) close things right where they are at.

With the exception you're basically dropping everything on the floor as if a great catastrophe has occurred, when it could be handled a lot more gracefully, and even give a warning and continue.

Jeremy Morgan
+1  A: 

I think it's fine to throw an exception in the SubtractCount() method if the value is already zero.

Btw should if (count < 0) not be if (count == 0) ?

However, I would make the UI prevent the user from choosing subtract count if it is already zero, thus avoiding the exception case in the first place.

Btw - do not raise ArgumentOutOfRangeException. This should be used when an argument passed to a method is not within the acceptable range. Your method does not accept any arguments. A more appropriate exception type would be InvalidOperationException.

AdamRalph
No, count can never be negative, but it can be zero.
Alex
I understand that. This means you need to test the case where SubtractCount() is being called, and the count is already zero, i.e. if (count == 0) throw exception
AdamRalph
A: 

Well, first of all you should make sure that the counter actually can't be negative. Currently it will decrement to -1 and throw the exception if you try to decrement it to -2.

As an alternative to just catching the exception, you can expose the check as a CanDecrement property so that it can be used separately:

class Counter {

  private int count;

  public bool CanDecrement { get{ return count > 0; } }

  public void Decrement() {
    if (!CanDecrement) {
        throw new ArguementOutOfRangeException("The count can never be negative!");
    }
    count--;
  }

}

Usage:

if (myObjectOfCounterClass.CanDecrement) {
  myObjectOfCounterClass.Decrement();
} else {
  Console.WriteLine("The count can not be negative.");
}

This way you can use the property for code where you expect the condition to occur, and in code where it should not normally occur you can just call Decrement and let the exception be handled like any other unexpected error.

Guffa
A: 

Exceptions should be used only in exceptional circumstances, user clicking one more time on a decrease button is not exceptional you can handle it by simple control logic. You can just not decrease the counter if it reached zero in addition you should change you UI it should block decrease option for the user if counter reached zero so user never will have an option to decrease it further.

MichaelT