Check reference parameter value or return bool?

Question

Hi,

I am using a method that has the following signature:

public static bool TryAuthenticate(string userName, string password, 
    string domainName, out AuthenticationFailure authenticationFailure)

The method declares: bool authenticated = false; then goes on to authenticate the user.

Whenever authenticated is set to true or false, authenticationFailure is set to AuthenticationFailure.Failure or AuthenticationFailure.Success correspondingly.

So basically I can use either authenticationFailure or the return value of the method to check the result. However it seems to be a pointless violation of DRY to have these two approaches in the same method.

Just to clarify, authenticationFailure is not used anywhere else in the method so it appears to be totally redundant.

At the moment I'm doing this:

public static bool IsValidLDAPUser(string username, string password, string domain)
{
    var authenticationStatus = new AuthenticationFailure();   
    if (ActiveDirectoryAuthenticationService.TryAuthenticate(username, password, domain, out authenticationStatus)) 
        return true;
    else return false;
}

But I could do this and get a similar result:

public static AuthenticationFailure IsValidLDAPUser(string username, string password, string domain)
{
    var authenticationStatus = new AuthenticationFailure();   
    ActiveDirectoryAuthenticationService.TryAuthenticate(username, password, domain, out authenticationStatus)
    return authenticationStatus;
}

• Why would you have a reference parameter that does the same thing as the return value?
• Which one should I use to check the result and does it make any difference?
• Is this just a case of bad code or am I missing the point?

Thanks in advance!

Answer 1

If a function returns only one value then the regular function return value should be used in preference to an out or ref parameter.

I would say this is even more important for methods named IsXyz (the emphasis being on is) and methods named in this way should return bool.

Answer 2

Often there are more error codes than just success or failure. Perhaps the designer of this method is going to add more enumerations for all the different failure types.

Sometimes, there is also more than one success type -- e.g. HTTP has a lot of return codes in the 200 and 300 block that would all be considered success in some way. So the bool tells you generally if it was successful or not, and the enum gives more exact information.

There are a lot of ways to do it, and this one is unusual, but not against DRY if they plan to add more codes.

Another way is to just encapsulate into a Result class that has the enum and a IsSuccess property. You could even provide a conversion to bool to make it easy to use in if statements.

Answer 3

First off, your enum variable name seems a little twisted to me. AuthenticationFailure.Success does not make much sense! It should be AuthenticationResult or something. Also, since in your method you just care whether authentication was successful, you should return bool. You are thinking of DRY but also think of KISS!

You can still make use of the enum for other error codes that you might add, but one look at the return value of your method should tell you whether it was successful or not. If you wish to find the details of why it was not successful, then use the enum that was passed as the 'out' parameter.

Answer 4

Ref parameters are not so easy to use and understand comparing to return value. However, there are some situation that using ref parameters can be advantage. For this case, if we assume allocating AuthenticationFailure is time-consuming, so using ref parameter is suitable because no allocation will be needed in case of authentication success.

Anyway, In this case I prefer using AuthenticationResult return type, as there should be no performance impact.

Answer 5

IMHO I don't think this is bool vs ref thing. To me it looks like the the wrong thing is being returned in the ref.

I would expect to see the following methods

public static bool TryAuthenticate(string userName, 
               string password, 
               string domainName, 
               out User user)

and

public static User Authenticate(string userName, 
               string password,                    
               string domainName)

One for when you don't care about why and one that you do. e.g.

This allows the calling code to do authenticate and then do something without a catch

User u;
if (TryAuthenticate(user,pass,domain,ref u))
  //do somthing
else
  return;

Or if they need that extra info then use a catch

e.g.

User u;
try
{
    u = Authenticate(user,pass,domain);
    //do somthing
}
catch(AuthenticationError ae)
{
    switch (ae.Failure)
    {
        case AuthenticationFailure.AccountLocked:
            //dosomthing A
            break;
        case AuthenticationFailure.BadPassword:
            //dosomthing B
            break;
        case AuthenticationFailure.InvalidUser:
            //dosomthing c
            break;
        etc..

    }
}

Now if there's no concept of a user or token then the Try Pattern probably isn't needed