Is returning null from a "get error message" method an anti-pattern?

Question

Is it "bad practice" to effectively cache the result of executing expensive stateless checking code by returning a null in the case of a "no problem found"? The upside is minimal code and no class/code bloat.

This is illustrated by this code:

public static String getErrorMessage(SomeState x) {
    // do some "expensive" processing with "x"
    if (someProblem)
        return "Some problem";
    if (someOtherProblem)
        return "Some other problem";
    return null; // no error message means "all OK"
}

And the calling code:

String message = getErrorMessage(something);
if (message != null) {
    display(message);
    return; 
}
// proceed

This pattern avoids having to repeat executing the expensive code twice by returning null to mean "there's no error message, because there's no error". And there's no extra "low value" classes/code.

The obvious alternatives are A) to separate out the concerns of checking and message creation:

public static boolean isOK(SomeState x) {
    // do some "expensive" processing with "x"
    return thereIsNoProblem;
}

public static String getErrorMessage(SomeState x) {
    // do some "expensive" processing with "x"
    if (someProblem)
        return "Some problem";
    if (someOtherProblem)
        return "Some other problem";
}

And the calling code:

if (!isOK(something)) {
     display(getErrorMessage(something)); // expensive code called a second time here
     return;
}
// proceed

which executes the expensive code once to determine if there's a problem, and again to determine what the problem is, or B) to return a "result" object that has a boolean field to answer the "if" part and a String field to answer the "message" part, eg

class MyResult { // like a struct in C
    boolean ok;
    String message;
    // accessor methods omitted 
}

public static MyResult verify(SomeState x) { ...}

And the calling code:

MyResult result = verify(something);
if (!result.ok) { // spare me the lecture on accessors - this is illustrative only
    display(result.message);
    return;
}

which creates class bloat and is a little clumsy IMHO.

Is it "bad" to "overload" the return value in this way?
It is certainly "neater" than all the alternatives I can think of.

---

If you offer an alternative, say why you think returning null is "bad". State the risks or downside to justify not using this simple technique.

Answer 1

I would use:

public class ValidationResult {

    public final boolean isValid;

    public final String errorMessage; // may be null if isValid == true

    public ValidationResult(boolean isValid, String errorMessage) {
        if (!isValid && errorMessage == null) {
            throw new IllegalArgumentException();
        }
        this.isValid = isValid;
        this.errorMessage = errorMessage;
    }
}

Then:

public static ValidationResult validate(SomeState x) {
    // blah
    return new ValidationResult(isValidTest, errorMessageIfNot);
}

And:

ValidationResult r = validate();
if (!r.isValid) {
    display(r.errorMessage);
}

---

Alternatively, you could have it throw a checked exception.

public class ValidationException extends Exception {
    // your code here
}

Then:

public static boolean validate(SomeState x) throws ValidationException {
    // ...
    if (isValid) {
        return true;
    } else {
        throw new ValidationException(message):
    }
}

Answer 2

It's ok as long as you are using it by yourself but it could be improved somehow. About why it could be improved, let me quote Tony Hoare:

I call it my billion-dollar mistake. It was the invention of the null reference in 1965. At that time, I was designing the first comprehensive type system for references in an object oriented language (ALGOL W). My goal was to ensure that all use of references should be absolutely safe, with checking performed automatically by the compiler. But I couldn't resist the temptation to put in a null reference, simply because it was so easy to implement. This has led to innumerable errors, vulnerabilities, and system crashes, which have probably caused a billion dollars of pain and damage in the last forty years.

First of all you could use exceptions:

public static String getErrorMessage(SomeState x) throws YourException {
    // do some "expensive" processing with "x"
    if (someProblem)
        throw YourException("Some problem");
    if (someOtherProblem)
        throw YourException("Some other problem");
    return null; // no error message means "all OK"
}

Then you can have a custom object:

class ErrorState {
  boolean hasFoundError;
  String message;

  ErrorState() {
    hasFoundError = false;
  }

  ErrorState(String message) {
    hasFoundError = true;
    this.message = message;
  }

  public final static ErrorState NO_ERROR = new ErrorState();
}

Finally if the set of potential errors is finite you could just use an enum (which is in my opinion the better choice):

enum ErrorState {
  NO_ERROR(""),
  SOME_ERROR("Some error"),
  SOME_OTHER_ERROR("Some other error");

  public final String message;

  ErrorState(String message) { this.message = message; }
}

Answer 3

Instead of checking isOK() then getting the error message, just add a throws exception to isOK() with appropriate message, then use a try/catch to display the error message.

There doesn't seem to be a reason to have multiple layers of checking then getting the errors when one function would suffice with throws.

Answer 4

Your initial solution is perfectly fine, null is perfect to represent the "absence of" a return value.

Answer 5

I've decided to work on my comment to make it an answer. I think this is a good context for an enumeration, something like this:

public enum OperationStatus {
    ERROR1 {
        @Override
        public String toString() {
            return "err1";
        }
    }, 
    ERROR2
    //The same as error 1
    //
    //Many error types here
    SUCCESS {
        @Override
        public String toString() {
            return "ok";
        }
    }
}

This way, your method can return a value of this enumeration specifying the actual error with a code that you can get as a String. This is in particular if you plan to localize such errors by having, for example, localized bundles that you can switch and obtain the associated value to each of the defined codes.

On the other hand, this would work if you don't plan to add errors in the future or if you can afford the occasional modifications this enumeration would require if new error types arise.

An alternative (you'd still need to update this array, but you prevent yourself from having an enumeration) to avoid the enum, a simple Stirng[] would do the trick too, since you can return an int as an error code and use it as an index of that array to obtain the actual code you can interpret, display and localize.

Answer 6

There are a limited number of options for reporting an error/abnormal condition within a called function:

1. Return a value (if the function can/does return a value) that indicates "error".
2. Return an error code at a location indicated by a "error pointer" parameter to the function.
3. Throw an exception.
4. Pass control to a previously-defined (or parameter-designated) "delegate" or "callback".
5. Update a global error indicator.
6. Invoke a global error routine.

None of these is especially attractive -- global is bad, we know, a delegate/callback is clumsy and verbose, and exceptions are slow (and the mark of the beast, as we all know). So the first two are the options most often used.

With 2 there is the problem that if you do return an error from a value-returning function, you still need to return a value. And, for an object-returning function, nil is the most logical value to return.

You kind of end up returning nil regardless.

Answer 7

I'm pretty anti-NULL, and, personally, would return the empty String "" to mean no error, or perhaps a constant like "OK". But null is acceptable.

The enum / value object concept does feel a bit like overkill, but often the requirements expand and you'll eventually need something like that.

In cases where there can be multiple errors, I like to return a List of Strings, and an empty list obviously means no problemo. In this case, do not return null, return an empty list.