Using a returned error message to determine if error is present

Question

I was recently talking with a buddy about return values taking only a single meaning. At my previous job, we worked with C++ and had typedef'ed wBOOL so that a 0 was wFALSE, and 1 was wTRUE. The architect said that we can also return 2, 3, 4... for more information, which I think is a horrible idea. If we expect wTRUE = 1 and wFALSE = 0 and wBOOL = {wTRUE, wFALSE}, returning anything else should be avoided... now, on to today's C#.

I recently reviewed a piece of code where there were a collection of functions that determined if there was an error and returned the string back to the user:

private bool IsTestReady(out string errorMessage)
{
  bool isReady = true;
  errorMessage = string.Empty;
  if(FailureCondition1)
  {
    isReady = false;
    errorMessage = FailureMessage1;
  }
  else if(FailureCondition2)
  {
    isReady = false;
    errorMessage = FailureMessage2;
  }
  //... other conditions
  return isReady;
}

Then, to use these functions...

private enum Tests
{ TestA, TestB, TestC }
private void UpdateUI()
{
  string error = string.Empty;
  bool isTestReady;
  switch(this.runningTest) // which test are we running (TestA, TestB, or TestC)
  {
    case Tests.TestA:
      isTestReady = IsTestAReady(error);
      break;
    case Tests.TestB:
      isTestReady = IsTestBReady(error);
      break;
    case Tests.TestC:
      isTestReady = IsTestCReady(error);
      break;
  }
  runTestButton.Enabled = isTestReady;
  runTestLabel.Text = error;
}

I thought to separate these out into two methods:

private string GetTestAErrorMessage()
{
  //same as IsTestReady, but only returns the error string, no boolean stuffs
}

private bool IsTestAReady
{
  get{ return string.IsNullOrEmpty(GetTestAErrorMessage()); }
}

Does this violate the principal of not having a return value mean more than one thing? For instance, in this case, if there error message IsNullOrEmpty, then there is no error. I think that this does not violate that principal; my co-worked does. To me, it's no different than this:

class Person
{
  public int Height {get;}
  public bool IsTall() { return Height > 10; }
}

Any thoughts or suggestions on a different approach to this issue? I think the out parameter is the worst of the solutions.

Answer 1

The return value and the error message are technically not bound together. You could have a developer come along at a later time and add a new failure condition to IsTestReady, and that failure condition may not set an error message. Or, perhaps there is a message, but it doesn't exactly represent a failure (like, perhaps a warning or something), so the error message parameter may get set, but the return value is true.

An exception doesn't really work in this case either, for the exact reason that StriplingWarrior wrote in his comment - exceptions should be used for non-normal operational states, and a non-ready test is a normal state.

One solution might be to remove the error message parameter and have the IsTestReady function return a class:

public class TestReadyResult {
    public bool IsReady { get; set; }
    public string Error { get; set; }
}

There is just one property to check - TestReadyResult.IsReady - for test state, and if necessary, the Error property can be used for non-ready states. There is no extra parameter to manage for the function call, either.

Answer 2

I'm not a big fan of having the null or empty return value indicate that nothing is wrong. A better comparison than the one you gave is:

class Person
{
    public int Height {get;}
    public bool IsBorn() { return Height > 0; }
}

In .NET, it is common practice to use the "bool return with out parameter" pattern you see in your original method (see the various TryParse methods, for example). However, if you prefer, another solution would be to create a TestReadyCheck class with both the boolean and the string as properties. I've done something similar with the following class, and been quite happy with it.

public class RequestFilterResult
{
    public static readonly RequestFilterResult Allow = new RequestFilterResult(true, null);
    public static RequestFilterResult Deny(string reason) { return new RequestFilterResult(false, reason); }
    protected RequestFilterResult(bool allowRequest, string denialReason)
    {
        AllowRequest = allowRequest;
        DenialReason = denialReason;
    }

    public bool AllowRequest { get; private set; }
    public string DenialReason { get; private set; }
}

This allows for the following usage:

public RequestFilterResult Filter(...)
{
    if (FailureCondition1) return RequestFilterResult.Deny(FailureMessage1);
    if (FailureCondition2) return RequestFilterResult.Deny(FailureMessage2);
    return RequestFilterResult.Allow();
}

It's concise, while enforcing that failure results provide a failure message, and success results don't.

On a side note, the structure of your switch statement feels like a code smell to me. You may want to consider ways to leverage polymorphism. Maybe make each test have its own class, with an IsTestReady method on it?