What to Return? Error String, Bool with Error String Out, or Void with Exception

Question

I spend most of my time in C# and am trying to figure out which is the best practice for handling an exception and cleanly return an error message from a called method back to the calling method.

For example, here is some ActiveDirectory authentication code. Please imagine this Method as part of a Class (and not just a standalone function.)

bool IsUserAuthenticated(string domain, string user, string pass, out errStr)
{
  bool authentic = false;
  try
  {
    // Instantiate Directory Entry object
    DirectoryEntry entry = new DirectoryEntry("LDAP://" + domain, user, pass);

    // Force connection over network to authenticate
    object nativeObject = entry.NativeObject;

    // No exception thrown? We must be good, then.
    authentic = true;
  }
  catch (Exception e) { errStr = e.Message().ToString(); }
  return authentic;
}

The advantages of doing it this way are a clear YES or NO that you can embed right in your If-Then-Else statement. The downside is that it also requires the person using the method to supply a string to get the Error back (if any.)

I guess I could overload this method with the same parameters minus the "out errStr", but ignoring the error seems like a bad idea since there can be many reasons for such a failure...

Alternatively, I could write a method that returns an Error String (instead of using "out errStr") in which a returned empty string means that the user authenticated fine.

string AuthenticateUser(string domain, string user, string pass)
{
  string errStr = "";
  try
  {
    // Instantiate Directory Entry object
    DirectoryEntry entry = new DirectoryEntry("LDAP://" + domain, user, pass);

    // Force connection over network to authenticate
    object nativeObject = entry.NativeObject;
  }
  catch (Exception e) { errStr = e.Message().ToString(); }
  return errStr;
}

But this seems like a "weak" way of doing things.

Or should I just make my method "void" and just not handle the exception so that it gets passed back to the calling function?

void AuthenticateUser(string domain, string user, string pass)
{ 
   // Instantiate Directory Entry object
   DirectoryEntry entry = new DirectoryEntry("LDAP://" + domain, user, pass);

   // Force connection over network to authenticate
   object nativeObject = entry.NativeObject; 
}

This seems the most sane to me (for some reason). Yet at the same time, the only real advantage of wrapping those 2 lines over just typing those 2 lines everywhere I need to authenticate is that I don't need to include the "LDAP://" string. The downside with this way of doing it is that the user has to put this method in a try-catch block.

Thoughts?

Is there another way of doing this that I'm not thinking of?

Answer 1

Don't handle the exception or return a message of any kind. Let the consumer of your method take care of this.

Answer 2

There is no "one size fits all". If you return a flag, that makes it easy to use a method in if() and loops. Exceptions always need a lot of boiler plate code. If you just want a string which you can display to the user (say, in a web UI), returning the error string (or null for "no error") is good, too.

But most of the time, I throw an exception (and in Java a subclass of RuntimeException) because that allows me to return more than a single information about the error (like: Which file caused the error? Which line/column? What was I doing? Which field in a form should be marked as illegal? etc).

In your case, you can't handle the exception in your method, so you shouldn't catch it. Only catch it when you can do something about it.

Answer 3

In this example, I agree, you should let the exception flow through to the consumer. However, as an alternative to the approaches you highlighted, consider this approach.

You can use a response object to hold information coming out of a method run, for example:

public abstract class BaseResponse
{
  public bool IsOk { get; protected set;}
  public string Message { get; protected set; }
}

public class AuthenticationResponse: BaseResponse
{
  public AuthenticationResponse(bool isOk): this(isOk, "") {}
  public AuthenticationResponse(bool isOk, string message)
  {
    IsOk = isOk;
    Message = message;
  }
}

AuthenticationResponse IsUserAuthenticated(string domain, string user, string pass)
{
  bool authentic = false;
  string errStr;
  try
  {
    // Instantiate Directory Entry object
    DirectoryEntry entry = new DirectoryEntry("LDAP://" + domain, user, pass);

    // Force connection over network to authenticate
    object nativeObject = entry.NativeObject;

    // No exception thrown? We must be good, then.
    authentic = true;
  }
  catch (Exception e) { errStr = e.Message().ToString(); }
  return new AuthenticationResponse(authentic, errStr);
}

Then to use it in your if statements:

AuthenticationResponse response;
if((response = IsUserAuthenticated("domain", "user", "pass")).IsOk)
{
  // do successful activity
} else {
  Console.WriteLine(response.Message)
}

The trick is the return value of an assignment operation is the value that was assigned. So, we can do the assignment and the valid check in the same line. If you didn't need to hold onto the result of the call, you could simply call the method and check the IsOk property.

if(IsUserAuthenticated("domain", "user", "pass").IsOk)
{
  // do successful activity
}

Then you can build up your custom response object to return any combination of values from your method as you need.