7

I am focusing on learning better design and wondered if this code is good or an anti-pattern? The function Validate() returns true if the data is correct, else it will throw an exception with a message to display in a popup.

Another way could have been to return a string and treat empty string as validated, but I don't see that as much cleaner.

try
{
    if (await Validate(data))
    {
        //logic
        success = true
    }
}
catch (Exception e)
{
   message = e.Message;
   success = false;
}

return new GenericResponse(success, data, message);
7
  • What will it mean when Validate returns false? Will this be a program error?
    – Doc Brown
    Sep 28, 2021 at 9:17
  • The function did never return false but threw an exception in that case. But i liked the approach using out instead I then discovered that async cant use out, my solution was to pass Active<string> instead and just have an implicit function that assigns the error message
    – proeng
    Sep 28, 2021 at 9:55
  • 2
    So you say Validate should never return false, and when it would, it would be a program error, yes? If that's correct, you could simply make Validate a void function.
    – Doc Brown
    Sep 28, 2021 at 10:53
  • 1
    @proeng, if it's async (and presumably we are talking C# here), it could return Task<(bool, string)>. As before, when false, the string contains the error message.
    – David Arno
    Sep 28, 2021 at 20:25
  • 1
    @DavidArno: or simply a Task<string> , with the convention of using null or the empty string if no error occured.
    – Doc Brown
    Sep 29, 2021 at 8:17

2 Answers 2

5

Either returning true or throwing an exception is an unusual way of indicating success or failure. It's not an anti-pattern, but it's not good practice either. Until I read the description of what happens properly, my immediate thought was that you weren't handling Validate returning false. It therefore breaks the principle of least astonishment.

A better approach would be to use the try pattern, eg for a language that uses out parameters you might have something like:

bool TryValidate(SomeType data, out string failureMessage);

where the return value is true for success and false for failure. When false, failureMessage contains the reason it fails (and it's undefined when the return is true).

Depending on the language you are using, you could use a tuple or even an Either<bool, string> union to return the data. Whichever method is used though, the principle of the pattern is true/false with a message when false is returned.

4
  • Thank you, that seems like a better approach. I was using try catch cause the logic did calls that required exception handling, so i thought might aswell piggyback. But i see now this way is more clear.
    – proeng
    Sep 28, 2021 at 7:43
  • IMO using exceptions for flow control is a bad practice and should be considered as an anti pattern. Sep 28, 2021 at 18:22
  • 1
    @DakshinamurthyKarra, the moment you put a try/catch in your code, you are using exceptions for control flow.
    – David Arno
    Sep 29, 2021 at 7:26
  • 1
    @DavidArno that way we can write code without conditionals. Seriously, an exception path is different from normal flow of the program. Mixing them up is a bad idea. YMMV. Sep 29, 2021 at 18:04
1

This is the very worst kind of Exception "swallowing".

try
{
   . . . 
catch (Exception e)
{
   // Do nothing useful 
}

The calling code knows that the function failed - it returned false - but can do nothing whatever about that failure.
If it had been thrown an Exception, it might have been able to take some more useful, corrective action. That's how Structured Exception Handling is supposed to work.
But throwing an Exception for each and every Validation error is expensive.

You know that the function failed - it returned false, which made the application behave a certain way - but have no idea why it did so and, therefore, no idea how to go about fixing it!
You should, at the very least, capture and record the Exception rather than making it "magically" disappear.

Another way could have been to return a string and treat empty string as validated, but I don't see that much cleaner.

Actually, it is and it's a very common model in Validation.
You ask your method to validate something and it returns you a message indicating what, if anything, is wrong with it. That's a far more "useful" value than just "Yes or No".

2
  • 1
    The example code in the question very clearly shows that the exception message is captured and returned. Your comments about not returning such information being bad is correct but it is unrelated to this question.
    – David Arno
    Sep 28, 2021 at 20:41
  • I do think your comment where helpful in showing why my first approach is not good design. I was throwing the messages using exception, but i learned now it is expensive and not intuitive. So having a function that return true or false, aswell as the message seems clearer.
    – proeng
    Sep 29, 2021 at 11:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.