What is a good design for a method that can return several logically different results?

Question

The question title is probably too abstract, so let me provide a particular example of what I have in mind:

There is a webservice that encapsulates a process of changing passwords for users of a distributed system. The webservice accepts user's login, his old password and a new password. Based on this input, it can return one of the following three results:

1. In case user was not found, or his old password does not match, it will simply return with HTTP 403 Forbidden.
2. Otherwise, it takes a new password and makes sure that it conforms to a password policy (e.g. it is long enough, contains a proper mix of letters and numbers, etc.). If it does not, it will return an XML describing why the password does not conform to the policy.
3. Otherwise, it will change the password and return an XML containing an expiration date of the new password.

Now, I'd like to design a class, ideally with a single method, to encapsulate working with this webservice. My first shot was this:

public class PasswordManagementWebService
{
    public ChangePasswordResult ChangePassword(string login, string oldPassword, string newPassword)
    {
        ChangePasswordResult result;

        // send input to websevice, it's not important how; the httpResponse
        // will contain a response from webservice
        var httpResponse;
        if (HasAuthenticationFailed(httpResponse)
        {
            throw new AuthenticationException();
        }
        else if (WasPasswordSuccessfullyChanged(httpResponse))
        {
            result = new ChangePasswordSuccessfulResult(httpResponse);
        }
        else
        {
            result = new ChangePasswordUnsuccessfulResult(httpResponse);
        }

        return result;
    }
}    

public abstract class ChangePasswordResult
{
    public abstract bool WasSuccessful { get; }
}

public abstract class ChangePasswordSuccessfulResult
{
    public ChangePasswordSuccessfulResult(HttpResponse  httpResponse)
    {
        // initialize the class from the httpResponse
    }

    public override bool WasSuccessful { get { return true; } }
    public DateTime ExpirationDate { get; private set; }
}

public abstract class ChangePasswordUnsuccessfulResult
{
    public ChangePasswordUnsuccessfulResult(HttpResponse  httpResponse)
    {
        // initialize the class from the httpResponse
    }

    public override bool WasSuccessful { get { return false; } }

    public bool WasPasswordLongEnough { get; private set; }        
    public bool DoesPasswordHaveToContainNumbers { get; private set; }
    // ... etc.         
}

As you can see, I've decided to use separate classes for return cases #2 and #3 - I could have used a single class with a boolean, but it feels like a smell, the class would have no clear purpose. With two separate classes, an user of my PasswordManagementWebService class now has to know which classes inherit from ChangePasswordResult and to cast to a correct one based on the WasSuccessful property. While I now do have a nice, laser-focused classes, I made a life of my users more difficult than it should be.

As for the case #1, I've just decided to throw an exception. I could have created a separate exception for the case #2, too, and only return something from the method when the password was successfully changed. However, this doesn't feel right - I don't think that a new password being invalid is a state exceptional enough to warrant throwing an exception.

I am not very sure how would I design things were there more than two un-exceptional result types from the webservice. Probably, I would change a type of WasSuccessful property from boolean to an enum and rename it to ResultType, adding a dedicated class inherited from ChangePasswordResult for each possible ResultType.

Finally, to the actual question: Is this design approach (i.e. having one abstract class and forcing clients to cast to a correct result based on a property) a correct one when dealing with problems like this? If yes, is there a way to improve it (perhaps with a different strategy for when to throw exceptions vs. return results)? If no, what would you recommend?

Answer 1

I don't subscribe to the school of thought which says "exceptions should only be for exceptional cases!" People are scared of exceptions for some reason. If you can't do what you said you're going to do, throw an exception - even if it's a common or expected failure.

I like this for a few reasons. It's impossible for the caller to ignore (someone could easily forget to inspect the return value of a method that indicates failure by a return code.) It's simple (no special placeholder values for missing results or hierarchies which need downcasting.) It's atomic (either my change succeeded or you tell me to get lost; I'm left in no doubt as to which it is.) It's granular (you can attach as much information as you like to the exception, and throw different exceptions for different failures.)

So I'd design your method like this:

public void ChangePassword(string password)
{
    HttpResponse response = RequestPasswordChange();
    if (AuthFailed(response))
        throw new AuthorizationException();
    if (PasswordWasNotChanged(response))
        throw new InvalidNewPasswordException(
            WasPasswordLongEnough(response),
            DidPasswordContainNumbers(response)
        );
}

"Succeed-or-throw" is a fine rule of thumb, and people are used to it, no matter how much they wave their arms. After all, Dictionary<TKey, TValue> throws KeyNotFoundException when you fail to index into it. A non-exceptional use of exceptions.

Answer 2

I think this approach is over-using inheritance which adds additional (unnecessary) complexity. Favor composition over inheritance when possible. Especially if it's a Success/Non-Success thing that is easily represented with a boolean it doesn't warrant the need for sub-classing.

You could include everything you need to into a single class, and based on the result you can use the properties accordingly. The class still has a single purpose, to return information about the password change. The fact that there are several different outcomes is an essential complexity of the task.

class ChangePasswordResult
{
    public bool Result { get; private set; }
    public DateTime ExpirationDate { get; private set; }  
    public bool WasPasswordLongEnough { get; private set; }        
    public bool DoesPasswordHaveToContainNumbers { get; private set; }
}

Edit:

jhewlett Brings up a very good point too about serialization. With a single result structure being returned you have a more stable interface clients can interact with. When you document your interface you can say "this endpoint returns X" as opposed to "this endpoint returns X OR Y depending on..."

Answer 3

I would assume there is different action when WasSuccessful is true and different action when WasSuccessful is false. Why not put this code inside of ChangePasswordSuccessfulResult and ChangePasswordUnsuccessfulResult and call it through virtual method, thus deciding based on the type. Then, you don't have to worry about casting.

public abstract class ChangePasswordResult
{
    public abstract SomeResponseData GetResponseData();
}

public class ChangePasswordSuccessfulResult
{
    public ChangePasswordSuccessfulResult(HttpResponse  httpResponse)
    {
        // initialize the class from the httpResponse
    }

    public DateTime ExpirationDate { get; private set; }

    public override SomeResponseData GetResponseData()
    {
        // return data for successful result
    }
}

public abstract class ChangePasswordUnsuccessfulResult
{
    public ChangePasswordUnsuccessfulResult(HttpResponse  httpResponse)
    {
        // initialize the class from the httpResponse
    }

    public bool WasPasswordLongEnough { get; private set; }        
    public bool DoesPasswordHaveToContainNumbers { get; private set; }
    // ... etc.         

    public override SomeResponseData GetResponseData()
    {
        // return data for unsuccessful result
    }
}