views:

114

answers:

9

I want to write a method in Java that verifies that some conditions hold on some data, and acknowledges that the data is valid or produces an appropriate error message otherwise.

The problem is that we cannot return more than one thing from a method, so I'm wondering what the best solution is (in terms of readability and maintainability).

First solution. Easy, but we cannot know what exactly made the check fail:

boolean verifyLimits1(Set<Integer> values, int maxValue) {
    for (Integer value : values) {
        if (value > maxValue) {
            return false; // Out of limits
        }
    }
    return true; // All values are OK
}

Second solution. We have the message, but we are using exceptions in a way that we shouldn't (besides, it should probably be a domain-specific checked exception, too much overhead IMO):

void verifyLimits2(Set<Integer> values, int maxValue) {
    for (Integer value : values) {
        if (value > maxValue) {
            throw new IllegalArgumentException("The value " + value + " exceeds the maximum value");
        }
    }
}

Third solution. We have a detailed message, but the contract is not clean: we make the client check whether the String is empty (for which he needs to read the javadoc).

String verifyLimits3(Set<Integer> values, int maxValue) {
    StringBuilder builder = new StringBuilder();
    for (Integer value : values) {
        if (value > maxValue) {
            builder.append("The value " + value + " exceeds the maximum value/n");
        }
    }
    return builder.toString();
}

Which solution would you recommend? Or is there a better one (hopefully!)?

(Note: I made up this little example, my real use case concerns complex conditions on heterogeneous data, so don't focus on this concrete example and propose Collections.max(values) > maxValue ? "Out of range." : "All fine." :-).)

+3  A: 

The solution is simple: create a custom VerificationResult class. It can have a boolean status flag and a String message field, among other things you may want to add. Instead of returning either a String or a boolean, return a VerificationResult.

Also, depending on context, throwing an exception may actually end up being the right thing to do. This has to be considered on a case-by-case basis based on concrete scenarios, though.


Alternative solution: a last error query

Another option you can use is to have the verification return a boolean, and have a separate method e.g. String whatWentWrongLastTime() that a user can query in case false is returned. You'd have to be very careful with any concurrency issues etc. that may overwrite the "last" verification error.

This is the approach taken by e.g. java.util.Scanner, which does NOT throw any IOException (except for the constructors). To query if something "went wrong", you can query its ioException() method, which returns the last IOException, or null if there wasn't any.

polygenelubricants
I'd really like not to create a new object for this. I'll use this as a last resort, though.
espinchi
You won't be able to do anything without creating a new object, since everything in java is an object. Assuming you meant a new class, creating a new class will be a lot less overhead than some of the other hacks that you would require to return multiple values.
Falmarri
@espinchi - you're using an OO language, defining new classes is absolutely normal. In fact, it shouldn't be your last resort but your first idea ;)
Andreas_D
Sure I meant a new class.I like the `VerificationResult` (aka `Validation` in Daniel Bleisteiner proposal). The problem is that such a generic class should go into our commons/core package, which I'd rather not extend too much.
espinchi
The last error query is interesting, but definitely too dangerous
espinchi
A: 

Create your own custom unchecked exception that extends from RuntimeException.

Jeroen Rosenberg
An unchecked exception is not a good idea, because if we verify values we *expect* that verification might fail. Custom exception is an idea but make it a checked one.
Andreas_D
Producing an error message is not an _exceptional_ condition for this method, and should not be the reason for a program termination. That's why I'm reluctant to use a unchecked runtime exception.
espinchi
A: 

You can use simple Key-Value, by using HashMap, of course with predefined keys. Return the HashMap for further processing.

djakapm
The same arguments against a generic `Pair<L,R>` in Java (which I may or may not agree with) can be used against this suggestion as well. A `Map<String,Object>` as a heterogenous bag of stuff is a poor substitute for a simple POJO.
polygenelubricants
Agreed, may be creating Status object as mentioned by Andreas_D is better than the Key-Value solution.
djakapm
+6  A: 

If you need more than a single value you should return a simple class instance instead. Here is an example of what we use in some cases:

public class Validation {
    private String          text    = null;
    private ValidationType  type    = ValidationType.OK;

    public Validation(String text, ValidationType type) {
        super();
        this.text = text;
        this.type = type;
    }
    public String getText() {
        return text;
    }
    public ValidationType getType() {
        return type;
    }
}

This uses a simple Enumeration for the type:

public enum ValidationType {
    OK, HINT, ERROR;
}

A validator method could look like this:

public Validation validateSomething() {
    if (condition) {
        return new Validation("msg.key", ValidationType.ERROR);
    }
    return new Validation(null, ValidationType.OK);
}

That's it.

Daniel Bleisteiner
I suggest adding a `public final static Validation OK = new Validation(null, ValidationType.OK)` so we don't have to create Validation objects each time validation is a success.
Andreas_D
Have a look at http://pastebin.com/0cAYvNhV, which is a bit more generic. I believe that's the solution I'll take.
espinchi
+3  A: 

IllegalArgumentException is the way to go if it really means that: You make some demands to the caller of the method (the contract) but they are ignored. In this case an IAE is appropriate.

If that doesn't reflect your use case, I'd use one of the solutions of the others.

musiKk
+1  A: 

In this case, the method returning 'false' looks like a business logic result rather than a real Exception. So verifyLimits should return a result anyway rather than throwing an Exception when 'false'.

 class VerifyLimitsResult{
       //Ignore get, set methods
        Integer maxValue;
        Integer value;

        public VerifyLimitsResult(Integer maxValue, Integer value) {
           this.maxValue = maxValue;
           this.value = value;
        }

        public boolean isOK(){
           return value==null;
        }

        public String getValidationInfo(){
           if(isOK()){
              return "Fine";
           }else{
              return "The value " + value + " exceeds the maximum value/n"
           }
        }
 }
....
VerifyLimitsResult verifyLimits4(Set<Integer> values, int maxValue) {

         for (Integer value : values) {
             if (value > maxValue) {
                   return new VerifyLimitsResult(maxValue, value);  
            }
        }
         return new VerifyLimitsResult(maxValue, null);  
}
卢声远 Shengyuan Lu
A: 

I would vote for the second solution (either using IllegalArgumentException or defining a specific one).

Generally good practice is ensuring that any return value from a method can safely be ignored (because some day somebody will forget to check it anyway) and, in cases when ignoring a return value is unsafe, it's always better to throw/catch an exception.

Vanya
+3  A: 

Another approach - use a Status object:

 public class Status {
   public final static Status OK = new Status("OK");
   private String message;
   public Status(String message) { this.message = message; }
   public String getMessage() { return message; }
 }

To Verify, either return Status.OK if the input is valid or create a new Status message.

 public Status validate(Integer input, int maxValue){
   if (input > maxValue) {
     return new Status(
         String.format("%s value out of limits (maxValue=%s)", input, maxValue);
   }

   return Status.OK;
 }

Using the verifier is simple as that:

 Status status = validate(i, 512);
 if (status != Status.OK) {
   // handle the error
 }
Andreas_D
I'd add an `isOK()` method to `Status` rather than relying on a magical constant. (Or maybe an `isError()` method so that you can use a positive test in that `if`.)
Donal Fellows
Also using an `enum` for `Status` could be appropriate.
Johannes Wachter
I propose http://pastebin.com/0cAYvNhV, which is generic enough to cover some more use cases (eg, when the validation result needs to be understood by the client code)
espinchi
@Johannes Wachter - Thought about enums with a message field and a pair of getter/setter. But then, if you do something like `Status.ERROR.setMessage("Validation failed");`, the message is set global, because there's only one instance of `Status.ERROR` in the application.
Andreas_D
@Andreas_D: You're right, if the message needs to be customizeable, it makes no sense to put it into an enum. Would only work well for a defined set that you can put into the enum like: ERROR_EXCEEDS_MAX, ERROR_...
Johannes Wachter
+1  A: 

I think the best solution is to create your own exception that holds as much error description information as you want. It should not be a RuntimeException subclass; you want callers to have to deal with a failure to validate, because too many programmers fail to put in error handling. By making failure a checked exception, you force them (you?) to put at least something in, and code review can relatively easily pick up if they're being stupid about it. I know it's bureaucratic, but it improves code quality in the long run.

Once you've done that, consider whether you need to return a value on successful validation or not. Only return a value if that value contains information other than “oh, I've got here now” (which is obvious from the program flow). If you do need to return a result, and it needs to be a complex result, by all means use a custom class instance to hold it! To not do that is just refusing to use the facilities that the language gives you.

Donal Fellows