2

This question already has an answer here:

I have been studying this subject quite a bit, and I am unsure of what's the best way. I still have a lot to go trough (books, blogs, stack exchange, etc), but I simply can't find a consensus.

Basically here is the situation: You have a function that is expected to return a result, but the function fails (parameter is bad, exception occurs, resource is blocked / unreachable, etc), should you throw an exception or return something?

I simply don't see a best way of doing this, here is how things look:

People seem to agree that returning null and coding for null (expecting null and checking for it) is bad practice and I wholeheartedly agree. But on the other side of things people don't seem to like "vexing exceptions", for example Int32.parseInt throws an exception instead of returning null, this function has a counterpart that does not do that (tryParse) which returns a boolean but has an output parameter:

bool result = Int32.TryParse(value, out number);
if (result)
{
    Console.WriteLine("Converted '{0}' to {1}.", value, number);         
}

I personally find this method a bit vexing, I studied threading under linux and most methods were methodA(in, in, out, in) which is horrible. With a proper IDE this issue is way smaller, but it still left a bad taste in my mouth.

And in some languages (like Java) this is not exactly easy to do, since it has no out descriptor (or whatever it's called) and it passes by value.

So it comes down to this: How would this be better handled? Add exceptions to when your code can't complete (if it isn't a number, I can't parse it, I can't return a number), or should you work around the issue. In C, C++, or C# I would probably always go with the bool method(in, out) approach, but in Java this seems like a really painful way of solving the issue, and I'm not sure if it's available in other languages or not.

marked as duplicate by GlenH7, durron597, Ixrec, gnat, Community Sep 22 '15 at 14:03

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

5

The modern line of thinking, which is considered as best practice by most publications and in most workplaces out there, is to never return result codes, and instead to always throw an exception if you cannot return a valid value. (But read more, there is a twist.)

The main reason why C# offers tryParse() is because in Microsoft Dot Net throwing an exception is outrageously slow, (because the DotNet runtime works in promiscuous collusion with the underlying Windows Operating System,) so you cannot really afford to have exceptions thrown and caught during normal flow of operations. By contrast, in Java throwing an exception and catching it is relatively inexpensive, (because it gets handled entirely within the VM without hitting the underlying operating system, whichever that may be,) so people do it all the time and are not afraid of it.

If you are really concerned about the performance overhead of exceptions, then you may "bend" the best practices to make them suit your needs.

Best practices dictate that any exceptional situation must be reported by means of an exception, while any non-exceptional situation must not be reported by exception. Thus, you can redefine, if you wish, what a non-exceptional situation is, as to include, for example, the case where you are unable to parse a given string. If you expect failures to be common, this is reasonable. So, in this scenario, it is permissible to indicate a parsing failure by some means other than an exception, because you have just redefined failure-to-parse as a non-exceptional situation.

So, in C#, besides the (admittedly awkward) try...() style of methods you can always return a nullable primitive (int?) or that "optional generic" suggested by Winston Ewert. In java, you can always return a boxed primitive, (Integer instead of int,) which can be null.

  • Performance is not the issue. DotNet does not have checked exceptions, so using exceptions as ordinary flow control is really breaking the whole idea of a structured programming. – JacquesB Sep 21 '15 at 18:48
  • @JacquesB I did not speak of using exceptions as ordinary flow control. Failure to parse can be considered as an exceptional situation, and in fact it is considered as such in the java world. In DotNet you don't even have the freedom to consider failure to parse as an exceptional situation, because if you do, each such failure would be extremely expensive. Imagine a recursive descent parser which uses int.Parse() within a try{} catch{} block when trying to determine if the next token is numeric: it would never complete parsing a document. – Mike Nakis Sep 21 '15 at 19:20
  • @JacquesB I did not speak of using exceptions as ordinary flow control. Allowing an exception to be thrown, and catching that exception, is not called using exceptions as ordinary flow control. It is called exception handling. – Mike Nakis Sep 21 '15 at 19:25
  • If you are using int.Parse() in a parser to check if the next token is numeric, then a parse error would definitely not be "exceptional", rather it would be the most common outcome. So handling that exception would be ordinary flow control. – JacquesB Sep 21 '15 at 19:39
  • The "modern line of thinking" has been thoroughly rejected by MacOS X and iOS software. There exception = programming error. Throw exceptions when you detect a programming bug. Never catch an exception, but fix the code. – gnasher729 Sep 22 '15 at 6:18
5

One possibility is to use an Optional generic. For example, see one from Guava:

http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/base/Optional.html

In this case, your function becomes

Optional<Int32> parseInt(String);

Now the calling code has to explicitly decide how it wants to handle the failure case. The Guava Optional generic has a variety of methods to make common use cases easy.

In some ways, this is like a null, in that you include the error state as a return value. However, this is explicit. If you try to pass a Optional where an Int32 is expected, you'll get a compiler error. At the point where Optional becomes an Int32, you know that you've got to think about handling the failure case.

2

On the one hand, it all depends on the contract of the procedure. Whatever you choose, you should make sure that what you expect of the caller (e.g., preconditions) and what the caller can expect from you (e.g., postconditions) are clear, including what the caller should expect on failure. If you put the burden of checking return values on the caller, make sure the caller knows.

I favor exceptions. Exceptions are beneficial because they make it difficult for client code to silently ignore the error. In Java, for instance, non-Runtime exceptions must be explicitly caught or declared as thrown; Runtime exceptions generally shouldn't be caught and are intended to terminate the program due to a bug. Exceptions also make it easier to separate the "real" logic of solving the problem from the "error handing" logic, which make the code easier to read.

What I wouldn't suggest is mixing using exceptions with using return codes – that'll confuse anyone using your API.

  • I honestly agree with everything you said, I'm simply too new at this to know if my instincts are right or not. Mandatory exception handling can be annoying, but they make sure everything works as expected. – Kalec Sep 21 '15 at 17:33
1

You should throw an Exception.

People like the TryParse because otherwise you need an IsParsable function which probably does the same thing under the hood.

Personally I try to define my functions such that common exceptions can be handled within the function itself. eg

//returns empty list when no objects are found
IEnumerable<Objects> Repository.GetObjects(string search)

The thing to avoid is a situation where throwing an exception will force the calling code to use the exception as part of the logic ie.

Try {
    This();
}
catch() {
    That();
}

As this is both expensive and prone to error when unexpected exceptions are thrown.

1

In general, throw an exception, do not implement a work around.

The basic principle is to write the method such that it is clear what the result will be from calling the function, and for the caller to expect that the function will succeed in its purpose.

The Int.TryParse() stretches this a little (but not beyond breaking point) in that it is clearly implied that the return from this function returns true/false on whether the string can be parsed into an integer. The return value of false implies that the function successfully determined that the string parameter was not a correctly formatted Integer.

The key, is that it is not instead of exceptions. This function will still throw exceptions (e.g. ThreadAbortException, InvalidArgException etc...). Exceptions are not being hidden by a return value of 'false'.

When to use the Try Implementation?

I would typically use the Int.Parse() in normal use.

The exception to this, is if I was writing the actual parsing function for a larger data set. Perhaps a configuration file, or validating user data entered in a UI. In these situations the actual purpose of the code is to do a series of validation checks on the data, and I am at the right point in the code to implement different actions based on data formatting.

If my only other option was to do:

try
{
   ...
}
catch(ParseException ex)
{
   //  do this instead....
}

and I would swallow the exception, then using the TryParse() makes sense.

1

It depends, but if you are in doubt, throw an exception.

The one thing you should never do, its to return an error indicator value which is the same type as a legitimate result. Like having IndexOf() returning -1 if the substring is not found. This is bad because you easily forget to check for this special value, so you might hide errors without handling them, which leads to hard-to-find bugs. Returning null also have this issue because null is a valid value for any object type, so it leads to the same problem.

The TryParse() pattern is somewhat better, since it is explicit in that it returns a flag which indicates if the operation succeeded. You can of course decide to ignore the return value, but you are not likely to do it by mistake. Furthermore, the TryParse()-method has the counterpart Parse(), which thrown an exception if parsing fails. So if your code is not able to recover form a parse error you just use the Parse() version anyway. If TryParse() didn't have the Parse() counterpart, you would run the risk of people ignoring the error code and hiding errors, but this is not likely when it is simply easier to use Parse().

So even if you implement a Try...-pattern method, you should have a counterpart which throws on error. Which leads back to the point that you as as default should implement the version which throws on error.

  • Umm, I disagree. Look at PHP: lots of people having issues exactly because it doesn't return -1 for not found, but another type, which equals to zero. Boom. Rarely people forget to check if it returned less than zero, unless they are badly expecting the needle to always be found. And that's just bad coding, not a problem with the return value. I view methods returning multiple types as ugly and bad design. – Sami Kuhmonen Sep 21 '15 at 19:21
  • @Sami: Obviously returning 0 is just as bad as -1 – JacquesB Sep 21 '15 at 19:50
  • No, they return false, which equals to zero if you don't know how to compare types. Which is a lot worse than returning -1. -1 is a clear indication of not found, no need to confuse with several types. But with .NET of course it would be possible to use Nullable<> – Sami Kuhmonen Sep 21 '15 at 20:14
  • @SamiKuhmonen: If the language will silently coerce any value to an integer then the only safe way to indicate "not found" is to throw an exception. – JacquesB Sep 21 '15 at 20:32
  • The thing to remember is, you can disagree with an answer without the hubris of being so damn sure that it is a bad answer, as to downvote it. – Mike Nakis Sep 21 '15 at 22:33
0

I just completed a project where I was wrapping practically ALL my methods that do this sort of work in a "message" type class, which (in C#) has a concrete boolean for success/fail, along with error/success messaging string collections. There's a generic <T> version that inherits from the non-generic version, so a method might return a plain TransactionResult (which just signals if a data call failed or succeeded, with extra messaging as needed), or a TransactionResult<string> which is the former, plus a string field which holds the return value (in this case, a string).

Now I can just inspect the returned object's 'Success' field, without having to know that "-1" means "string not found". I could even wrap my "ResultObject" property with a backing private field and check it for null when setting (if I was so inclined). No more error handling for common stuff, and overall its been quite easy to implement.

public class TransactionResult
    {
        public bool Success { get; set; }
        public ValidationResult[] TransactionErrors { get; private set; }
        public string[] SuccessMessages { get; private set; } // in case you need to signal a partial success, or a custom result message

        public TransactionResult()
        {
            TransactionErrors = new ValidationResult[0];
            SuccessMessages = new string[0];
            Success = true; // Easier to let it be 'true' by default, and 'false' only when you add an error or set it manually!
        }

        public void AddTransactionError(string errorMessage, string[] memberNames = null)
        {
            var newError = new ValidationResult(errorMessage, memberNames ?? new[] { "" });
            TransactionErrors = TransactionErrors.Concat(new[] { newError }).ToArray();
            Success = false;
        }

        public void MergeWithErrorsFrom(TransactionResult transaction)
        {
            if (transaction == null || !transaction.TransactionErrors.Any())
                return;

            SuccessMessages = SuccessMessages.Concat(transaction.SuccessMessages).ToArray();
            TransactionErrors = TransactionErrors.Concat(transaction.TransactionErrors).ToArray();
            if (TransactionErrors.Any())
                Success = false;
        }

        public void AddSuccessMessage(string message)
        {
            SuccessMessages = SuccessMessages.Concat(new[] { message }).ToArray();
        }
    }

    public class TransactionResult<T> : TransactionResult
    {
        public T ResultObject { get; set; }

        public void MergeWithResultsFrom(TransactionResult<T> transaction)
        {
            MergeWithErrorsFrom(transaction);
            ResultObject = transaction.ResultObject;
        }
    }

The above class uses the System.ComponentModel.DataAnnotations.ValidationResult class as its sorta backing "error" class, since I'm working in MVC4, which means my results can bind to my UI pretty much automatically, but you could roll your own version of that error as well. I wouldn't mind working in a language where this sort of structure was automatically part of the language/framework instead of having to roll my own, but that's pretty opinionated.

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