Question About Where To Position Try And Catch statements

Question

I've used try and catch statements as an easy way to keep my code running without things crashing (I would wrap everything in a big try). Recently, I've wanted to start using try and catch statements more correctly. Here as an example I have questions about:

public class Ninja{
    Ninja(){
    }

    public void ThrowShirikin(int numberOfShirikins){
        try{
            if(numberOfShirikins == 0){
                throw new System.ArgumentException("Invalid number of shirikins");
            }

            //Throw shirikin
        }
        catch(ArgumentException e){
            MessageBox.Show(e.Message);
        }
    }
}

In the above Ninja class, the entire contents of my ThrowShirikin method is contained in a try loop. Since there is only one opportunity for an input error (in this case, when numberOfShirikins == 0), shouldn't only the lines of code that check for this be contained in the try loop? See below:

public class Ninja{
    Ninja(){
    }

    public void ThrowShirikin(int numberOfShirikins){
        bool errorsExist = false;
        try{
            if(numberOfShirikins == 0){
                errorsExist = true;
                throw new System.ArgumentException("Invalid number of shirikins");
            }
        }
        catch(ArgumentException e){
            MessageBox.Show(e.Message);
        }

        if(!errorsExist){
            //Throw shirikin
        }
    }
}

^But what I have here seems a bit clunky. Any suggestions and input on how I'm understanding the use of try catch statements? Thanks!

Edit:

Or I could do something like this so the //Throw shirikin code never executes if there is an invalid value for numberOfShirikins?:

public class Ninja{
    Ninja(){
    }

    public void ThrowShirikin(int numberOfShirikins){
        try{
            if(numberOfShirikins == 0){
                throw new System.ArgumentException("Invalid number of shirikins");
                return;
            }
        }
        catch(ArgumentException e){
            MessageBox.Show(e.Message);
        }

        //Throw shirikin
    }
}

Answer 1

When you throw exceptions like ArgumentException you shouldn't catch them in your method. Give this work to client for your method.

Answer 2

It looks like the try/catch only exists to catch an exception you are choosing to create and throw, and then you catch it just to pop a message box to the user. Then you choose to either continue or not based off the error condition.

Eliminate the try and the catch. Eliminate the message box. Do throw an exception for an invalid argument. Let the caller of the function determine if they want to catch and how they want to handle. Your Ninja class should not be making these decisions beyond identifying what is valid and what is not.

if (numberOfShirikins == 0)
    throw new ArgumentException("...");

// rest of your shirikin throwing code

Answer 3

You should never catch exceptions in this way. Only catch exceptions that you can actually handle.

public class Ninja{
    Ninja(){
    }

    public void ThrowShirikin(int numberOfShirikins){
            if(numberOfShirikins <= 0){
                throw new System.ArgumentException("Invalid number of shirikins");
            }
        //Throw shirikin
    }
}

I suggest that you start programming without ever using try/catch. Then fix any exceptions you see, don't hide them.

Answer 4

There is no real reason to catch exceptions in the same method like that. Go without:

public class Ninja{
    Ninja(){
    }

    public void ThrowShirikin(int numberOfShirikins){
        if(numberOfShirikins == 0){
            MessageBox.Show(e.Message);
            return;
        }

        //...
    }
}

Answer 5

A method should not be capturing exceptions that it throws itself - it's fine to generate them, but understand that they are expensive and that normal flow control is preferred (it's easier to reason about and such).

Further, in the above example, errorsExist will never be set to true - that should be put in your catch block rather than directly under where you're throwing an exception. That terminates the normal program flow.

Restated, method would look like this.

public void ThrowShirikin(int numberOfShirikins) {
    if(numberOfShirikins == 0){
        throw new System.ArgumentException("Invalid number of shirikins", "numberOfShirikins");
    }

    //Throw shirikin
}

It would be up to the class calling the method to decide what to do with the exception.

Answer 6

Neither is a good idea. There is no sense in throwing an exception and catching it in the same method and to display a forms dialog from that class.

Only classes that should contain GUI code are the ones in your GUI.

In your case, I wouldn't use an exception either, I would use a boolean return value:

public bool ThrowShirikin(int numberOfShirikins){
            if(numberOfShirikins == 0){
                return false;
            }
            //throw
            return true;
    }

Exceptions should be used for exceptional cases, not the general, which this would be.

Also, be aware that the errorsExist = true; code in your second way will never be called as it is below a throw statement.

Answer 7

When you throw an exception, you're basically saying: "This is an exceptional situation that I don't know how to handle (or another method higher up would have a better idea of how this should be handled)"

So what the method you just posted is saying then is: "I don't know what to do when I'm asked to throw 0 shurikens. Actually I lied, I'm going to show a message box"

Either skip the exception all together and show the message (not ideal, but better than throwing than what you have), or just throw the exception and catch it higher up in the UI code. You may want to create your own exception type if you want to handle this specific one specially.

Answer 8

IMO - an Exception is really an exceptional condition in your code - i.e. conditions which cannot be anticipated and/or cases where the error can error in truly exceptional conditions.

If the code snippet you have shown above, have an numberOfShirikins == 0 is not an exceptional condition, it is a normal argument check

So, I would re-factor the code to:

public class Ninja{
    Ninja(){
    }

    public void ThrowShirikin(int numberOfShirikins){
        bool errorsExist = false;
        try{
            if(numberOfShirikins == 0){ showUserMessage("Invalid number of shirikins", MessageType.Information); }
        }
        catch(ArgumentException e){
            showUserMessage(e.Message, MessageType.Exception, e);
        }

        if(!errorsExist){
            //Throw shirikin
        }
    }
   private showUserMessage(string message, MessageType type, Exception ex = null){
        MessageBox.Show(message);
   }

}

Once this is done, you will find that there is really no value addition done to catching the exception, so we might be able to write a generic exception handler to show exception & let your code flow through normally.

Now, my code would look like:

public class Ninja{
        Ninja(){
        }

        public void ThrowShirikin(int numberOfShirikins){
                if(numberOfShirikins == 0){ showUserMessage("Invalid number of shirikins", MessageType.Information); return; }
                else{ /* do something here */ }
        }

On the other hand, if the numberOfShirikins == 0 is an exceptional condition, then I will write a custom Exception with the code as:

public class Ninja{
        Ninja(){
        }

        public void ThrowShirikin(int numberOfShirikins){
                if(numberOfShirikins == 0){ throw NumberOfShirikinsException(); }
                else{ /* do something here */ }
        }

Answer 9

If you can avoid to throw exceptions, you should. They cost about 1000-4000 clock cycles.