Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Question About Where To Position Try And Catch statements

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
    }
}
like image 313
sooprise Avatar asked Feb 14 '11 20:02

sooprise


2 Answers

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

like image 140
whyleee Avatar answered Sep 21 '22 19:09

whyleee


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
like image 28
Anthony Pegram Avatar answered Sep 20 '22 19:09

Anthony Pegram