views:

632

answers:

17

I have a deceptively simple scenario, and I want a simple solution, but it's not obvious which is "most correct" or "most Java".

Let's say I have a small authenticate(Client client) method in some class. The authentication could fail for a number of reasons, and I want to return a simple boolean for control flow, but also return a String message for the user. These are the possibilities I can think of:

  • Return a boolean, and pass in a StringBuilder to collect the message. This is the closest to a C-style way of doing it.
  • Throw an exception instead of returning false, and include the message. I don't like this since failure is not exceptional.
  • Create a new class called AuthenticationStatus with the boolean and the String. This seems like overkill for one small method.
  • Store the message in a member variable. This would introduce a potential race condition, and I don't like that it implies some state that isn't really there.

Any other suggestions?

Edit Missed this option off

  • Return null for success - Is this unsafe?

Edit Solution:

I went for the most OO solution and created a small AuthenticationResult class. I wouldn't do this in any other language, but I like it in Java. I also liked the suggestion of returning an String[] since it's like the null return but safer. One advantage of the Result class is that you can have a success message with further details if required.

A: 

You could return a Collection of error messages, empty indicating that there were no problems. This is a refinement of your third suggestion.

sblundy
I actually use this elsewhere in the app, but here there will never be >1 message. However, the empty array could be a good success indicator.
Draemon
Everytime you think "success indicator", change it to "failure indicator" and use an exception ;-)
Guillaume
+4  A: 

Returning a small object with both the boolean flag and the String inside is probably the most OO-like way of doing it, although I agree that it seems overkill for a simple case like this.

Another alternative is to always return a String, and have null (or an empty String - you choose which) indicate success. As long as the return values are clearly explained in the javadocs there shouldn't be any confusion.

Ashley Mercer
That is exactly what I was going to answer.
Avi
I'd thought of the null idea but missed it off (edited now). I was worried it might be unsafe - if there's a bug it could end up returning null - which indicates success. The empty string is probably a much better idea. Thanks.
Draemon
Returning an empty String or null to indicate failure is called returning a "sentinel value". Neither is particularly safe, since you're effectively defeating the type system.
Apocalisp
Guys - the issue here is that return values are all to often and to easily ignored. Hence with something like authentication the Exception route is imho the more secure.
Nick Holt
Null is not a bad idea IF you're expecting it.
OscarRyz
Null is a bad idea, because at some point you will NOT be expecting it.
Guillaume
Sentinels are an especially bad idea if you require the caller to manually check the return value in order to assure the integrity of your auth system. When a user's credentials fail to authenticate, the application logic should completely derail, making it impossible to proceed forward. Full stop.
benjismith
More like the lazy-way of doing it.
Zombies
I feel strongly that returning null from a method is a poor practice because I am tired of dealing with lame NullPointerExceptions in legacy code.Please consider the following: http://c2.com/cgi/wiki?NoNullBeyondMethodScope
Kyle W. Cartmell
A: 

How about returning a string. Empty or Null for success. Error Message in case of failure. Simplest that would work. However not sure if it reads well.

Gishu
A: 

I personally think creating a new class called AuthenticationStatus with the boolean and the String is the most Java like way. And while it seems like overkill (which it may well be) it seems cleaner to me and easier to understand.

jschoen
A four line class definition doesnt seems like a small value of overkill to me :-)
James Anderson
It becomes overkill when you keep remaking this same file, only slightly different for each application, whereas maybe Y developer chooses to not use it in other applications. In addition it creates more bloat in certain EAR build environments where you might have duplicate but unequal POJOs.
Zombies
A: 

I use the "tiny class" myself, usually with an inner class. I don't like using arguments to collect messages.

Also, if the method that might fail is "low level" - like coming from an app server or the database layer, I'd prefer to return an Enum with the return status, and then translate that into a string at the GUI level. Don't pass around user strings at the low level if you're ever going to internationalize your code, because then your app server can only respond in one language at a time, rather than having different clients working in different languages.

Paul Tomblin
Fair point about the messages, but not important here. I should have made it clear it was a system level message that will only be used for debugging / logging purposes. It's not really user visible.
Draemon
A: 

Is this the only method where you have such a requirement? If not, just generate a general Response class with an isSuccessful flag and a message string, and use that everywhere.

Or you could just have the method return null to show success (not pretty, and does not allow returning a success AND a message).

Michael Borgwardt
+7  A: 

You could use exceptions....

try {
    AuthenticateMethod();
} catch (AuthenticateError ae) {         
    // Display ae.getMessage() to user..
    System.out.println(ae.getMessage());
    //ae.printStackTrace();    
}

and then if an error occurs in your AuthenticateMethod you send a new AuthenticateError (extends Exception)

adam
I agree with this. Failure to authenticate is not an error you want the caller to be able to ignore easily.
jmucchiello
I agree too. Maybe this is not an exceptional case for the application, but it definately is an exceptional case for this connection "sub-system". That's why I would advice not only use an Exception throwing, but also make a different ones for different kinds of failures.
Max
Never use exceptions to implement bussiness logic.
Peter
It would be really slow, too.
kristina
Could you quantify "slow" ? Authentications are usually not the bottleneck of your application, and you pay the cost mostly when there actually is a failure. Not too bad.
Guillaume
Exceptional doesnt mean that it happens seldom. It means that it is not the expected outcome. When you try to authenticate, you expect it to succeed. So yes, this is the perfect case for Exceptions.
Guillaume
@Peter... The aphorism you're looking for is "Never use exceptions to implement flow control." This isn't flow control. The user provides credentials expecting to be logged in, but the credentials fail to authenticate. Why? Maybe no such username exists. Maybe the password has expired. Who knows?
benjismith
(...continued...) Regardless of cause, the user expected to be granted access using the supplied credentials, but that expectation has been thwarted for some reason, and the existing flow control (accessing protected resources) has been de-railed. It's a *perfect* example of an exceptional case.
benjismith
This is the better way: There are many ways that authentication could fail: "invalid username/password"(1), account locked(2), not authorized(3). The original requirement is to give the user a string DESCRIBING how their login failed ONLY when it fails. This is exactly what exception handling does.
Zombies
+1  A: 

Some more options:

  • Return an separate enum value for each type of failure. The enum object could contain the message
  • Return an int and have a separate method that looks up the appropriate message from an array
  • create a generic utility tuple class that can contains two values. Such a class can be useful in many more places.

simple tuple example, actual implementation may need more:

class Tuple<L, R> {

    public final L left;
    public final R right;

    public Tuple( L left, R right) {
     this.left = left;
     this.right = right;
    }
}
Bas Leijdekkers
I agree that PairTuples are very useful classes to have around, they remove the need to create all those clutter DTOs that hang around. I would override equals, hashcode and toString and it can be used anywhere, in collections etc.
Shawn
1) Enums + decoder functions < declarative property files2) What about 3 values? or 4?... Which value is which? Isn't this just a collection of values?
Zombies
A: 

I would choose the Exception option in first place.

But, in second place, I would prefer the C-style technique:

public boolean authenticate(Client client, final StringBuilder sb) {
 if (sb == null)
  throw new IllegalArgumentException();
 if (isOK()) {
  sb.append("info message");
  return true;
 } else {
  sb.append("error message");
  return false;
 }
}

This is not so strange and it's done in many places in the framework.

bruno conde
Can the person that vote me down explain why?
bruno conde
A: 

Instead of creating a special object for return type, I usually just return an array where all the returned information is stored. The benefit is that you can extend this array with new elements without creating new types and mess. The downside you have to know exactly what elements should present when array is returned from particular method to parse it correctly. Usually I agree on certain structure, like first element is always Boolean indication success, second is String with description, the rest is optional. Example:

public static void main(String[] args)
{
 Object[] result = methodReturningStatus();
 if(!(Boolean)result[0])
  System.out.println("Method return: "+ result[1]);
}

static Object[] methodReturningStatus()
{
 Object[] result = new Object[2];

 result[0] = false;
 result[1] = "Error happened";

 return result;
}
Ma99uS
A: 
Olivier
This is like the enum solution but with ints. Enums would be more java-y
Draemon
+1  A: 

Avoid returning a "sentinel value", especially null. You will end up with a codebase where methods cannot be understood by the caller without reading the implementation. In the case of null, callers may end up with NullPointerExceptions if they forget (or don't know) that your method may return null.

The tuple suggestion from Bas Leijdekkers is a good one that I use all the time if I want to return more than one value from a method. The one we use is P2<A, B> from the Functional Java library. This kind of type is a joint union of two other types (it contains one value of each type).

Throwing Exceptions for control flow is a bit of a code smell, but checked exceptions are one way of getting more than one type of value from a method. Other, cleaner possibilities exist though.

  1. You can have an Option<T> abstract class with two subclasses Some<T> and None<T>. This is a bit like a type-safe alternative to null, and a good way to implement partial functions (functions whose return value isn't defined for some arguments). The Functional Java library has a full-featured Option class that implements Iterable<T>, so you can do something like this:

    public Option<String> authenticate(String arg) {
       if (success(arg))
          return Option.some("Just an example");
       else
          return Option.none();
    }
    
    
    ...
    
    
    for(String s : authenticate(secret)) {
       privilegedMethod();
    }
    
  2. Alternatively, you can use a disjoint union of two types, as an Either<L, R> class. It contains one value which is either of type L or R. This class implements Iterable<T> for both L and R, so you can do something like this:

    public Either<Fail, String> authenticate(String arg) {
       if (success(arg))
          return Either.right("Just an example");
       else
          return Either.left(Fail.authenticationFailure());
    }
    
    
    ...
    
    
    Either<Fail, String> auth = authenticate(secret);
    for(String s : auth.rightProjection()) {
       privilegedMethod();
    }
    for(Fail f : auth.leftProjection()) {
       System.out.println("FAIL");
    }
    

All of these classes, P2, Option, and Either are useful in a wide variety of situations.

Apocalisp
A: 

Just because failed authentication is commonplace doesn't mean it isn't exceptional.

In my opinion, authentication failures are the poster-child use case for checked exceptions. (Well... maybe file non-existence is the canonical use case, but authentication failure is a close #2.)

benjismith
There's a snag with exceptions. They don't compose. For example, if you want to authenticate a whole list of users and collect failures, having to catch Exceptions makes that unnecessarily difficult.
Apocalisp
Interesting. What's the use case for simultaneously authenticating multiple users?
benjismith
Even if there is a use case for it, processing auths in a batch implies that they should ALL either succeed or fail, as a batch (in which case, exceptions are still the best option). Otherwise, I'd argue that you're not really operating in batch mode, and each auth should have its own try/catch.
benjismith
It could be a test, or maybe you're performing a background job on behalf of several users. The principle is applies to any number of use cases, many of which may not involve lists. You might want to pass an authorization module that acts differently for authenticated and unauthenticated users.
Apocalisp
A: 

Return the Object. It allows you to put additional functionality into the Class if you need it. Short lived objects in Java are quick to create and collect.

Javamann
A: 

This seems like a common idiom in other programming languages, but I cannot figure out which one ( C I guess as I read in the question ) .

Almost the same question is posted here and here

Attempting to return two values from a single function, may be misleading. But as it has been proved by the attempts of doing so, it may be very useful too.

Definitely creating and small class with the results should be the correct way to proceed if that is a common flow in the app as posted before.

Here's a quote about returning two values from a function:

As a matter of programming style, this idea is not appealing in a object oriented programming language. Returning objects to represent computation results is the idiom for returning multiple values. Some suggest that you should not have to declare classes for unrelated values, but neither should unrelated values be returned from a single method.

I've found it in a feature request for java to allow multiple return values

look at the "evaluation" section dated: 2005-05-06 09:40:08

OscarRyz
A: 

Successful authentication should be the "normal" case, so an authentication failure is the exceptional case.

What are the different status strings for the user anyway. I can see only two, success or failure. Any further information is a potential security issue. Another advantage of the solution with exceptions is that it cannot be called in the wrong way and the failure case is more obvious. Without exceptions, you write:

if (authenticate()) {
  // normal behaviour...
}
else {
  // error case...
}

You can accidently call the method ignoring the return value. The "normal behaviour" code is then executed without successful authentication:

authenticate();
// normal behaviour...

If you use exceptions, that cannot happen. If you decide to not use exceptions, at least name the method so that it is clear that it returns a state, e. g.:

if (isAuthenticated()) {
//...
}
Markus
A: 

There are a lot of good answers here so I will keep it short.

I think failure of a user to authenticate can be considered a valid case for a checked exception. If your style of programming favoured handling exceptions then there would be no reason not to do this. It also removes the "How to return multiple values from a method, my method does one thing It authenticates a user"

If you are going to return multiple values then spend 10 minutes creating a generic PairTuple (can also be more than a pair TripleTuple, I won't repeat the example listed above) and return your values that way. I hate having small dto style objects to return various multiple values they just clutter the place.

Shawn