1

In our code base, I see a lot of code like this

var error = ValidatePhoneNumber(userId, phoneNumber);

//if validation fails, return error
if(!string.IsNullOrEmpty(error))
{
    return error;
}

If I were writing this, I would have just had ValidatePhoneNumber(userId, phoneNumber) throw the error and call the method inside of a try/catch block. The only reason I can think of for this kind of practice is that throwing/catching errors can become expensive. Would concerns of performance really necessitate this kind of error handling? Or would a try/catch pattern be more useful in this situation?

I've seen posts like this, and I can see that there are some minor performance differences in including exceptions, but are they large enough to concern most applications?

(I work mostly in the .NET framework, if that makes any difference to the answer)

  • 3
    Exception = an act/case which should not happened. – Fabio May 03 '17 at 22:14
  • Not an answer, but this particular example may well not be an exception- it's the validation failing because the content isn't valid. –  May 03 '17 at 22:15
  • 1
    @Fabio Some systems use Exceptions as best practice for handling any sort of error. **cough** Microsoft **cough** See https://msdn.microsoft.com/en-us/library/ms173163.aspx for example – Peter M May 03 '17 at 22:20
  • @PeterM, from your link: _Exceptions should not be used to change the flow of a program as part of ordinary execution. Exceptions should only be used to report and handle error conditions._ – Fabio May 03 '17 at 22:22
  • @Fabio So then it comes down to `Is the phone number expected to be valid at this point, or is it an error that isn't`? There is not enough context at this point – Peter M May 03 '17 at 22:24
  • @PeterM - Validation of phone number is responsibility of application - users will input invalid numbers all the time - this not exception. – Fabio May 03 '17 at 22:27
  • @Fabio Validating the user input at that level is not an exception. But if you have passed that input to a separate and independent module that expects validated phone numbers then from *that* modules point of view, not having a valid phone number *is* an exception. – Peter M May 03 '17 at 22:31
  • @PeterM - correct, but you shouldn't "handle" those exceptions in "internal modules", because if it happens - your validation logic not working. If exception thrown - it should go up to the stack - cancel "transactions", log it, inform user. – Fabio May 03 '17 at 22:36
  • @Fabio Except that it might not be your validation logic that is in error. The user may have entered a valid phone number, but the number was corrupted by the ORM when a different module tried to read it from the database. – Peter M May 03 '17 at 22:41
  • @PeterM - again - this scenario is exception - you don't need to handle all possible real exceptions in your internal modules. Internal modules just throw exception - which will be handled on higher layer. The question was clearly about - why exceptions shouldn't be used as validation flow control. – Fabio May 03 '17 at 22:48
  • @Fabio: exceptions as flow control can be appropriate in the specific case of handling error situations, as you can have your primary path through an operation assuming the ideal case, and exception throwing can bail out at the right point and handling the exception can clean up. The alternative is arrowheads of status codes and whatnot which can be a nightmare. – whatsisname May 03 '17 at 23:06
  • Agree, there are exceptions which can be handled, by retrying again, for example. But we talking about "validation" exception. Which are really not expected to happened, because validation is made early. – Fabio May 03 '17 at 23:10
  • How many phone numbers are you validating that you think this might be critical for performance? – gnasher729 May 04 '17 at 00:21
  • Possible duplicate of [Clean readable code vs fast hard to read code. When to cross the line?](https://softwareengineering.stackexchange.com/questions/89620/clean-readable-code-vs-fast-hard-to-read-code-when-to-cross-the-line) – gnat May 04 '17 at 08:12
  • see also: [Return magic value, throw exception or return false on failure?](https://softwareengineering.stackexchange.com/q/159096/31260) – gnat May 04 '17 at 08:13
  • @gnat Good links. I should point out that functions like `isValidPhoneNumber()` don't surprise me, even in contexts where exceptions are thrown. – candied_orange May 04 '17 at 18:44

5 Answers5

9

Performance is not a reason to do any of this.

If you are returning errors, rather than throwing them, I hope you at least do it consistently.

The reason why has nothing to do with performance, language, or program correctness. It has to do with confusing humans.

If you offer some functions/methods to me that I can use to solve problems don't confuse me by having some of them return errors and some throw exceptions. Pick one style and stick with it!

That doesn't mean this code is necessarily wrong. It had just better not be in a context where functions are expected to throw. It had better be returning to a context where functions are expected to return errors.

I can't tell if this code is good or bad since I only have this one function to go on. But it certainly raises my eyebrows. What I need to see is what the other methods/functions are doing. If this is doing things different from the others around it then it's going to be a nasty surprise.

If you think the reason we aren't supposed to use exceptions for flow control is because grabbing a stack trace is too expensive then you've never heard of Knuth.

The reason you're not supposed to use exceptions for flow control is because they're almost as hard to follow as goto. So use them for when things go wrong. Use them to move error handling logic away from the "sunny day" logic. Don't use them because you think they're a fun way to branch your code. Don't amaze me with your clever ways of using them. I have enough to think about.

candied_orange
  • 102,279
  • 24
  • 197
  • 315
3

As C# programming guide stays: Things to Avoid When Throwing Exceptions

Exceptions should not be used to change the flow of a program as part of ordinary execution. Exceptions should only be used to report and handle error conditions.

Validation is part of "ordinary" execution logic in your application.
If you wrap your validation method in try .. catch block, what type of exception you will going to catch, Exception? If so, then what happens if validation method thorw exception not related to validation?
Using exceptions as validation logic can be misleading for other developers or readers of your code (most of the time programmers read code).

I am pretty sure that validation can be done in more readable way then using exceptions.

var phoneNumber = ValidatePhoneNumber(string number);
var outputText = phoneNumber.ToText();

Where

public interface IPhoneNumber
{
    string ToText();
}

public class ValidPhoneNumber
{
    public ValidPhoneNumber(string number) { _number = number } 
    public string ToText()
    {
        return $"Phone: {number}";
    }
}

public class InvalidPhoneNumber
{
    public ValidPhoneNumber(string errorMessage) { _message = errorMessage} 
    public string ToText()
    {
        return _message;
    }
}

And validate method

public IPhoneNumber ValidatePhoneNumber(string rawNumber)
{
    // do your validation logic get error message if possible

    if (IsValid)
    {
        return new ValidPhoneNumber(validNumber);
    }
    else
    {
        return new InvalidPhoneNumber(errorMessage);
    }
}

With approach above, by introduced InvalidPhoneNumber you hide "implementation" details about validation and just use "invalid" implementation of interface.

Fabio
  • 3,086
  • 1
  • 17
  • 25
  • "Validation is part of "ordinary" execution logic" as an absolute statement is not always true. "Validation" can encompass a wide variety of checks depending on your applications, and for some situations a validation very rarely fails, such as checking data received from other components, not just stuff inputted by an operator. In those situations using exceptions can be appropriate. – whatsisname May 03 '17 at 22:58
  • One step further and you end up with a well-known `Result` class, which either wraps a value of type `V`, or an error of type `E` (usually a string). The great [Railway-Based Programming presentation](https://www.slideshare.net/slideshow/embed_code/32242318) explains it in detail. – 9000 May 04 '17 at 02:57
  • Why should exceptions only be used for "out-of-ordinary" situations - is it because of performance? – user253751 May 04 '17 at 03:00
  • 3
    @immibis: You might be interested in [Martin Fowler's opinion on this topic](https://martinfowler.com/articles/replaceThrowWithNotification.html). He favors the 'exceptions are for exceptional situations' approach, too. – 9000 May 04 '17 at 04:12
  • @immibis -simply, because of the same reason as you don't use `string` for storing integers - _use right tool for the job_. – Fabio May 04 '17 at 06:04
  • @Fabio And the only reasoning for that is so you can log/report more than one cause of failure? – user253751 May 04 '17 at 08:51
0

Exceptions, as the name says, should be used for exceptional circumstances.

Validating user input and finding it at fault is in no way exceptional. At least not with the users I know. They make mistakes all the time. So handling them is the normal thing the program has to do.

Now if your phone number came from a super secure database and should by all means be valid, then checking it again and finding it invalid should be an exception.

So this really is not a technical decision. Ask yourself if the error condition is normal, or an exception.

Lets take another example: the method IsMyPizzaDoneYet() returns a bool. Simply because there is a very real chance you were impatient and it's not done yet. Nothing to see, move along. It might as well throw a HolyShitTheOvenIsOnFireException if the oven is on fire. Because that's an exception.


To not confuse people, if the point of a method is throwing an exception, just name it so. Building on the example above, where suddenly finding that your database is inconsistent and lacks a valid phone number despite the fact that you checked them when users are created:

public void SendTextMessageToKnownUser(int userID, string message)
{
   var phone = GetPhoneNumberFromSecureDatabase(userID);

   ThrowIfNotValid(phone);

   phone.Send(message);
}
nvoigt
  • 7,271
  • 3
  • 22
  • 26
0

To highlight the performance aspect of the question:


My obligatory reference: Fix it you should: Are you aware that you have thrown over 40,000 exceptions in the last 3 hours?

Executive Summary: In .NET, throwing an exception is expensive. Don't do it on regular hot code paths.


The takeaway is still the same, exceptions should be exceptional, so if you validate input, using an exception might be a performance problem if the method to validate the input isn't used in the GUI, but suddenly is used to validate, e.g., an input csv file with 1M records. Better not use exceptions for every wrong record there.

That being said, my take would be roughly:

void ValidatePhoneNumberFromInteractiveContext(...) {
  ValidatePhoneNumber_Throws(userId, phoneNumber);
  // The interactive caller knows how to format and display
  // exceptions
}

/or/

ErrorObject ValidatePhoneNumberFromBatchContext(...) {
  return ValidatePhoneNumber_WithError(userId, phoneNumber);
}

Do note that the first approach only really works properly if your exception contains sufficient information to build up a decent "message" at the catch site.

Martin Ba
  • 7,578
  • 7
  • 34
  • 56
0

Is the product you are working on derived form work that was done in C, or was designed to work with C.

If so, then it is not for performance reasons why exceptions are not being used.

In the example that you give, it doesn't really matter how errors are handled, just that they are handled consistently.

If the person telling you this is your technical lead or anyone you don't lead, don't cotradict him. Just realize it is for other reasons, you will eventually need their help.

Robert Baron
  • 1,132
  • 7
  • 10