16
votes
\$\begingroup\$

This falls straight into the same category with the recent "Is it better practice to return if false or execute an if statement if true?" question.

Often, while writing code, I find myself presented with two options. Either I can do this:

public void myMethod() throws Exception {
    // Some code
    validate(some_object_or_condition);
    // Some code
}

private void validate(Object arg) throws Exception {
    // Code that validates the argument and throws an exception if it's invalid
}

Or this:

public void myMethod() throws Exception {
    // Some code
    if( !isValid(some_object_or_condition) ) throw new Exception();
    // Some code
}

private boolean isValid(Object arg) {
    // Code that returns a boolean based on whether or not the argument is valid
}

Is either one preferable over the other? When I was still just learning Java the latter felt more "object oriented" and somehow more natural. However, with experience the former has started to feel better because then the code looks cleaner to me, but I'm afraid people might find it less readable.

Edit/Add: This time I ran into this problem when implementing a game (just for the fun of it) where the user would place words into a grid. The playWord method may fail because of the word not actually being a real word (yielding an InvalidWordException) or because the user is trying to place a valid word into an illegal position (yielding a WordPlacementException). If the method does not fail, it should return the score of the word as an integer. So in my case the playWord method first calls the method that validates the word and later the method that validates the location of the word, and these provoked this question here.

\$\endgroup\$

locked by Jamal Aug 8 '14 at 5:06

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site, so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. More info: help center.

Read more about locked posts here.

11
votes
\$\begingroup\$

There's quite a few questions about this on stackoverflow but I think my general rule tends to be leave exceptions for exceptional circumstances and booleans for where failure occurances is an accepted/known possible behaviour.

So I personally probably prefer your option 2 (interested to see what others think), where if anything the exception is thrown a bit higher up the chain. That way I could re-use the isValid logic elsewhere and deal with the failure without needing to wrap it in an exception handler.

Of course, I think this is probably a case by case basis and depends on the kind of logic being checked.

I'm sure I remember a really good Q&A on this somewhere but can't quite find it. However found a couple of discussions:

Throw exception or return boolean - SO

Throwing exceptions - SO

EDIT: To further comment after the edits, perhaps you could also consider the option of returning status codes (enums) instead of the exceptions if there are a number of options. This has the benefit of being more efficient however as some have mentioned on SO could expose unecessary workings if this is a API or assembly. Some links I found on SO about this with some good discussions were:

Exception or error code enum to use error codes or not

And there were more. I see this coming down to a few options.

  1. Throw exception
  2. Return true/false
  3. Return a status/error code enum

All probably have their place. Exceptions do occur the most overhead so if performance is an issue or the routine is called in many places perhaps this is not the best solution. I also saw mention that it was not necessarily a good idea to use exceptions to control the flow of application logic which it would seem to be doing in your case?

Basically my thoughts are to use return true/false, use a status error code, and then exceptions in that order.

An alternative suggestion might be something along the lines of (whether it's a decent alternative or not hopefully someone might comment on ???):

public int playWord(string w) {
    Word word = new Word(word);
    if (word.isValid()) {
       return word.getScore();
    } else {
       throw new InvalidDataException(word.GetErrorMessage());
    }
}

class Word {
   private String word;
   private String errorMessage = null;

   public Word(string w) {
       word = w;
   }

   public boolean IsValid() {
       // TODO: perform various checks calling private methods as required and if fail store against errorMessage
      return errorMessage == null;
   }

   public String GetErrorMessage() {
      return errorMessage;
   }

   public int getScore(string word) {
     // return the score of the word no matter what it is
   }
}
\$\endgroup\$
  • \$\begingroup\$ Hey, thanks for the links. I indeed didn't take a look at SO before posting this because SO is not generally very welcoming for these kinds of questions where a definite answer may not exist and where there's no broken code. The problem with leaving exceptions for exceptional circumstances is how to define when a circumstance is exceptional enough. I've edited my post to contain a practical example (or two, if you want to read it that way), could you please take a look at it again? \$\endgroup\$ – ZeroOne May 13 '12 at 22:03
6
votes
\$\begingroup\$

Returning a Boolean seems more natural to me. It gives you the option to handle this condition in different ways, depending on the situation. The IsValid method has only one very specific task, which is to determine if the object is valid. This follows SRP, the single responsibility principle, which states that a code entity (class, method, component) should have only one responsibility.

if(IsValid(obj))
    Save(obj);
else
    Messagebox.Show("Please enter a valid code");

In another context the same IsValid method can be reused without any changes

if(!IsValid(obj))
    throw new WhateverException();

Note: Wikipedia defines SRP on classes; however this principle can be applied to different levels of granularity.

\$\endgroup\$
  • \$\begingroup\$ The validate method also has the same single specific task, it just expresses the result in a different way. Actually, I think you could argue that myMethod follows the single response principle better when it calls the validate method! \$\endgroup\$ – ZeroOne May 13 '12 at 21:41
  • 1
    \$\begingroup\$ I am not a SRP purist; however, I would say that your method (which is OK) validates AND throws an exception if the test fails and therefore does two things. Of cause you can argue that this is just another way of returning a result, but it influences the flow of execution as well. I am just telling my personal preference; both solutions are OK. \$\endgroup\$ – Olivier Jacot-Descombes May 13 '12 at 21:56
  • \$\begingroup\$ Thanks for your opinion. :) For the sake of discussion I'd like to add that actually the playWord method originally implemented the validation code in itself and it used to throw an exception. In order to keep the code clean I then refactored the validation part into its own private method. So basically the exception-throwing validate() would just be a copy-paste creation, whereas with isValid() I'd actually need to add a little something. \$\endgroup\$ – ZeroOne May 15 '12 at 22:22
3
votes
\$\begingroup\$

The answer is it depends, or both.

  1. If you are the user of that method, and know this is an infrequent occurrence, throw exceptions.
  2. If you are user of the method, and know it is a frequent error, use booleans.
  3. If you don't know the frequence because you are not the only user, have a Try pattern. I.e.

public void MyMethod throws ArgumentException { }

And

public bool TryMyMethod
{
}
\$\endgroup\$
  • \$\begingroup\$ These are nice rules of thumb, but the problem with this approach is how to define "frequent" and "infrequent". I've edited my post to contain an example, could you please take a look at it again? \$\endgroup\$ – ZeroOne May 13 '12 at 22:05
  • \$\begingroup\$ @ZeroOne that's an IsValid imo. User input will frequently be invalid, doesn't make sense to inccur the overhead of an exception to validate it. \$\endgroup\$ – M Afifi May 14 '12 at 8:31
1
vote
\$\begingroup\$

Return the boolean (or better, return a meaningful object as suggested by Michael Anderson). If the validation failure is invalid, the calling method can choose to throw an exception.

That way, isValid() can throw an exception if there is an error while validating (perhaps loading a dictionary fails). This separates normal flow from exception processing.

\$\endgroup\$
1
vote
\$\begingroup\$

It is interesting that everyone is saying boolean. While booleans can be useful exceptions are extremely powerful and offer you a lot of flexibility. Booleans leaves a hole for people who aren't paying attention to not validate that the function worked. With exceptions a single function can have multiple reasons for failure and each exception will explain that failure. Sometimes one failure is acceptable while another is not. This way you can catch one failure and just throw the others up the chain.

public void validate(String top, String bottom) {
  if(top == "") {
    throw EmptyTopException("You forgot to put something on the top");
  }
  if(bottom == "") {
    throw EmptyBottomException("You forgot to put something on the bottom");
  }
}

public void main(String args[]) {
  try {
    validate("spire", "foundation");
  }
  catch(EmptyTopException ex) {
     System.out.println("The top was empty we will have a shorter tower");
  }
  catch(EmptyBottomException ex) {
     throw new Exception("There was no foundation this tower is not physically possible", ex);
  }
}

It can get a little big with a lot of try catches but as you can see you can fail for two different reasons here. one which doesn't really make a difference and another that will tell the user that the input can not be accepted.

\$\endgroup\$
0
votes
\$\begingroup\$

There's another option that is used more frequently in some other languages, which is to return the exception. This makes some sense when failure is not really exceptional and can be ignored in some cases.

public void myMethod() throws Exception {
    // Some code
    Exception e = validate(some_object_or_condition);
    if(e!=null) throw e;
    // Some code
}

private Exception validate(Object arg) {
   if(...) { 
       return new Exception(...);
   }
   ...
   return null;      
}

Now admittedly in many cases this looks pretty cumbersome, but where it really shines is when you start to need inversion of control or callbacks (where you pass the return values and exceptions to the callbacks rather than return them directly.)

When inversion of control is used this often looks like

public void myMethod() {
    Validator validator = getValidator();
    ErrorHandler eh = getErrorHandler();
    validator.validate(some_object_or_condition, eh);
}

public abstract class ErrorHandler {
  public void process(Exception e);
}

public void myMethod() throws Exception {
    // Some code
    Exception e = validate(some_object_or_condition);
    if(e!=null) throw e;
    // Some code
}

class Validator 
   private void validate(Object arg, XYZ xyz) {
   if(...) {
       xyz.process( new Exception(...) );
       return;
   }
   ...
   xyz.process(null);      
}

Again this can be uglier than the pure throw, or boolean return style, but in some cases it can result in more testable and code that can work better in a multi-threaded environment.

\$\endgroup\$
  • \$\begingroup\$ Wouldn't that be a misuse of a class? Repurposing a class just because it's convenient. I suggest that you could return something else (a Validation object, perhaps), and provide a facility to turn that into an Exception (throw new ValidationException(validation)). An example of what you suggest is Spring's BindException: I think Spring's Errors interface is reasonable, but the decision to have BindException implement Errors is one of Spring's odder ones. \$\endgroup\$ – Paul Hicks Feb 5 '14 at 3:31
  • \$\begingroup\$ One reason for returning an exception, rather than your own class, is that the type heirachy is already there and users are likely to be familiar with it. Also it them makes it easy if you decide later that it is "fatal" and you can throw it. Javascript captures the call stack at the point an exception is constructed, rather than (or in addition to) when it is thrown - solving another issue that often occurs with using your own custom error reporting class. However I'm not sure what java does on that front. \$\endgroup\$ – Michael Anderson Feb 5 '14 at 7:39
  • \$\begingroup\$ Yes, it does make things easier to do it that way. But the class isn't a kind-of Exception. Encapsulation is appropriate, but not inheritance, IMO. \$\endgroup\$ – Paul Hicks Feb 5 '14 at 8:33

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