Is it good practice in Java to pass a StringBuffer to a method to get error messages

Question

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!

Answer 1

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.

Answer 2

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.

Answer 3

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.