Throwing exceptions at multiple points (refactoring)

Question

I'm writing a function that takes user input, runs a procedure in our database, and compares the values. Along the way, I need to check that we've received proper input and then that the query has returned an acceptable value.

    private void DoTheThing(int? userInput1, int? userInput2, int valuePassedIn)
    {
        if (userInput1 == null || userInput2 == null)
        {
            Exception ex = new Exception();
            ex.Data.Add("Message", "You screwed up.");
            throw ex;
        }

        var queryResult = 0; //execute a query using the non-null inputs

        if (queryResult == null) //or otherwise doesn't return an acceptable value
        {
            Exception ex = new Exception();
            ex.Data.Add("Message", "some other thing happened");
            throw ex;
        }
        else
        {
            //We're good, so do the thing
        }
    }

A quick note about this: I'm aware of the argument against exceptions as flow control and that I'd be better off checking the user's input before I even get this far. I won't get into all the details, but please accept that I'm kind of stuck writing the function this way.

That having been said, here's my question:

Given that the only differences between the 2 exceptions here is the message and the time at which they are thrown, how can I clean this code up to be both DRY and avoid running unnecessary code after determining that there will be a problem?

I thought about using a goto and placing the error code there, but that really only moves the problem around. If I move the exception code to the bottom and check for a message variable (or something similar), then I'm just running code that doesn't need to be run in the first place.

Answer 1

You might be better off creating a BadInputException class and a NullQueryResultException class. These do two different things and throwing a specific exception is better than throwing a generic Exception(...). In fact I think FXCop or Visual Studio's Code Analysis will give you a warning about throwing generic Exceptions.

It's not really all that much new code to write.

public class BadInputException : Exception 
{
    public BadInputException()
    {
        this.Data.Add("Message", "You screwed up.")
    }
}

Then instead of this:

Exception ex = new Exception();
ex.Data.Add("Message", "You screwed up.");
throw ex;

Do this:

throw new BadInputException();

Edit: moved the "You screwed up" message from the Message property to the Data collection to match what the OP wants.

Answer 2

I suggest not throwing Exception (which means something went wrong, no comments are available), but ArgumentNullException and InvalidOperationException classes. Another amendment is avoding arrow-head antipattern:

private void DoTheThing(int? userInput1, int? userInput2, int valuePassedIn)
{
    // What actually went wrong? An argument "userInput1" is null
    if (null == userInput1)
      throw new ArgumentNullException("userInput1"); 
    else if (null == userInput2)
      throw new ArgumentNullException("userInput2"); // ...or userInput2 is null

    var queryResult = executeSomeQuery(userInput1, userInput2, valuePassedIn);

    // What went wrong? We don't expect that null can be returned;
    // so the operation "executeSomeQuery" failed:
    // we've provided validated (not null) values and got unexpected return.
    // Let it have been known. 
    if (null == queryResult) 
      throw new InvalidOperationException(
        String.Format("Query ({0}, {1}, {2}) returned null when bla-bla-bla expected", 
          userInput1, userInput2, valuePassedIn));   

    // We're good, so do the thing
    // Note that's there's no "arrow-head antipattern": 
    // we're not within any "if" or "else" scope 
}

Edit: Since every *Exception is inherited from Exception you can put some info into Data:

  Exception ex = new ArgumentNullException("userInput1");
  ex.Data.Add("Some key", "Some value");
  throw ex;

but often Message is a far better place to explain what had heppened.

Answer 3

I would create a method:

private void CheckResult(bool cond, string msg, string info) {
    if (!cond)
        return;

    Exception ex = new Exception();
    ex.Data.Add(msg, info);
    throw ex;
}

and call

CheckResult(userInput1 == null || userInput2 == null, "Message", "You screwed up.");

and

CheckResult(queryResult == null, "Message", "You screwed up.");

Answer 4

I think the QuestionRefactoring Guard Clauses is helpful for you .

There are something about this in Replace Nested Conditional with Guard Clauses.

Hope it's useful.