Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a class that has a method which takes a user as an argument and performs several validation checks on the user and returns a boolean to indicate if theyare valid.

I want to be able to get error messages back from the validation method along with the boolean return value.

I was thinking of doing it by passing a StringBuffer as an argument along with the user and in the validation code appending error messages to the buffer so that the caller gets the boolean returned and then can look at the StringBuffer for error messages.

// validation class
public boolean validateUser(User user, StringBuffer errors) {
    ...
}

// caller
StringBuffer errors = new StringBuffer();
boolean validUser = validateUser(user, errors);
if (!validUser) {
    log(errors.toString();
}

Is this good practice in Java? Or would it be better to log the errors to a buffer in the validation class and have a public method to get the errors? Or should the validateUser() method return an object that contains the boolean and errors?

Thank you!

share|improve this question
    
An Appendable or CharSequence parameter instead of a StringBuffer would be more flexible and allow e.g. StringBuilders. –  Martin Schröder Nov 16 '12 at 9:32

3 Answers 3

up vote 2 down vote accepted

I quite like the idea when implementing error handlers to do stuff through an interface to remove any dependencies on modules. Sometimes I've used that in conjunction with a global static Log method. I've typically done this in c# but the theory is the same across to Java I believe.

So how about something like this?

interface IErrorHandler
{
   void addError(string message);
   bool hasErrors();
   void persist();
}

Then you could do either:

// Possibly return an IErrorHandler instead of boolean here?
public boolean validateUser(User user, IErrorHandler errorHandler) {

   // on error
   errorHandler.AddError(...);
}

Then create an implementation of errorHandler that wraps a StringBuffer (if you wish or whatever i.e. FileWriter, DbWriter etc)

class MemoryErrorHandler implements IErrorHandler
{
   private final StringBuffer _errors;

   public MemoryErrorHandler() {
      _errors = new StringBuffer();
   }

   public void addError(string message) {
      _errors.append(message);
   }

   public bool hasErrors() {
      _errors.length() > 0;
   }

   public void persist() {
      System.out.console.printlin(_errors.toString();
   }
}

Then from the caller something like

IErrorHandler errorHandler = new MemoryErrorHandler();
validateUser(user, errorHandler);
if (errorHandler.hasErrors()) {
    // do something with the errors
    errorHandler.persist();
}

A further enhancement would be to consider DI of the IErrorHandler into your application so that it is only ever defined in one place. That way will end up having dependencies on IErrorHandler throughout the code which would make it easier to unit test as required.

Of course the IErrorHandler interface could include other methods, these were just suggestions based on your original code.

share|improve this answer
    
I agree with your comment regarding the validateUser() method: passing only a User and returning an IErrorHandler seems to be more clean than a validation method with side-effects. You could check the hasErrors() method after, and then handle with any errors as necessary. –  E-Man Nov 15 '12 at 19:07
    
Thank you for the detailed example and information –  user1331369 Nov 15 '12 at 19:36

Since you are dealing with a validation method, I believe you will want to read my question Is it better practice to have void method throw an exception or to have the method return a boolean? and its answers. My point there is that you don't really need to return a boolean, you could also have a void method throw an exception if the validation fails. The exception could then, in your case, contain the errors as its payload. But I think that passing a logger as a parameter isn't a bad practice, just basically a matter of personal preference and to be decided on a case by case basis, much like the question about whether to return a boolean or to throw an exception.

In any case, however, I would never use a StringBuffer as an error logger like in your case. For one, I might be building a web application where the output is rendered in HTML. In that case the StringBuffer would basically need to contain the HTML markup, which would then become a problem once you one day figure out that you need to run your process from the command line as well, and then you get all the HTML markup to mess up the output there. Also, with the StringBuffer you cannot easily get the error count, just whether there were any errors or not. So I would probably create an implementation of some IErrorHandler kind of interface wrapping a List<String> rather than a StringBuffer, as in @dreza's answer.

share|improve this answer

I would prefer to have a method that returns a class instance:

public ValidateUserResult validateUser(User user, StringBuffer errors) {
    ...
}

ValidateUserResult result = validateUser(user);
if (!result.isValid()) {
   for (String error: result.getErrors()) {
       System.out.println("Error: " + error);
   }
}

Java doesn't have the concept of "output" parameters and I think that anything that tries to mimic that behavior is confusing. Alternatively, you can just return a list of errors, and if that list is empty, then the user is valid. Or you can create a generic "validation result" class similar to what dreza suggested.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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