Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
    
See this question for a solution that doesn't involve casting. –  Doval Jul 20 at 16:12
    
@Doval Do you mean the answer from Joey Adams? If yes, then I'm deducing the solution would probably involve the ChangePassword to accept two Actions, one accepting the ChangePasswordSuccessfulResult and the other ChangePasswordUnsuccessfulResult. Is that correct? –  Nikola Anusev Jul 20 at 16:42
    
Yes, that answer. You can use Either as-is and return Either<ChangePasswordSuccessfulResult, ChangePasswordUnsuccessfulResult>. The user can call match with two lambdas to take a decision based on the type of result without doing any casting. The only change I'd make to that implementation of Either would be to give it a private constructor and make Left and Right private inner classes so no one can add new subclasses and break it. –  Doval Jul 20 at 17:30
    
One more thing - if you're going to go with my approach, I would recommend against throwing an exception. The client code already has to do some branching for the different success cases; it's better to add the failure case to the return value so it can be handled in the same way. If you don't, the client has to do branching twice using two different mechanisms - a try/catch plus the call to the match method. –  Doval Jul 21 at 12:01
    
If C# had any kind of native support for ADTs (or if I was using a functional language like F#), I'd certainly go that way. Since this is something C# lacks, I don't suppose I would ever find a way to implement some pseudo-ADT to be immediately obvious and clear for an average C# programmer. Right now, I am torn between leaving it just as desribed in OP, or using exceptions for everything except successful password change. –  Nikola Anusev Jul 21 at 21:43

3 Answers 3

up vote 2 down vote accepted

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.

share|improve this answer
1  
"People are scared of exceptions for some reason" - sure, because in most (not all) languages a raised exception has a significant performance impact. Next reason is that an exception changes the program flow in an sometimes unexpected way‌​. Exceptions have their place and are a good tool, if you do it right. –  JensG Jul 21 at 7:15
2  
The questioneer's code is making HTTP requests. I don't think an exception is going to make an appreciable difference to the performance. I agree that you can't stop clients from handling exceptions badly, but you also can't stop them from ignoring return codes (which I'd argue is an easier mistake to make) –  Benjamin Hodgson Jul 21 at 9:52
    
Sure. And how is the "old school of thought" related to HTTP requests? Just asking. –  JensG Jul 21 at 15:18
3  
"which I'd argue is an easier mistake to make": This depends on the language you are using. If your language supports algebraic data types, then the error code is just an extra option in your return data type and the compiler will complain if you do not handle it in your code. –  Giorgio Jul 21 at 17:26
    
Yeah, I am now convinced that ADTs are exactly what I need. In C#, however, I suppose it really comes down to a choice between a return code and an exception. Solution I described in OP is basically about return codes. I'd say it's more likely that client will notice that there are two classes inheriting from ChangePasswordResult and start thinking about what it means - to notice the method can throw an exception, he would have to open up docs and really, what is a chance of that happening? Declared exceptions would help here, but this is another thing C# lacks.... –  Nikola Anusev Jul 21 at 21:17

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..."

share|improve this answer
1  
+1, especially considering that the result will be serialized to xml or json in this case –  jhewlett Jul 20 at 23:10
1  
"The fact that there are several different outcomes is an essential complexity of the task." Which is exactly why returning the union of all possible outcomes is the wrong thing to do. Now the client must figure out which fields are applicable to the current outcome. I agree with favoring composition, but Nikola isn't really trying to use inheritance. He needs a tagged union, which C# and Java lack, but can be implemented with inheritance in a way that hides the inheritance tree from the client (see my comments to the question.) –  Doval Jul 21 at 11:55
    
@Doval I'm not sure what you mean by "he isn't really using inheritance". He has a base class, and a subclass for each outcome. Regarding the extra properties, I don't see the success/error results being so wildly different that it creates problems. In a successful password change, WasPasswordLongEnough and DoesPasswordHaveEnoughNumbers are still valid. In a bad password result you have ONE property that you wouldn't use, ExpirationDate (I removed NewPassword property which wasn't supposed to be there). Which leads me to the same conclusion that a tagged union is also unnecessary in this case –  Despertar Jul 21 at 17:21
    
What I meant by that comment is that he's using inheritance to create a closed union (i.e. there's a fixed number of possibilities) of POCO result objects, as opposed to creating an actual object whose methods can be overwritten. And as long as there's at least one property that must be ignored, you've inconvenienced the client. If ExpirationDate is left null, there's potential for an exception at runtime. If you avoid null (which you should) the client has to unbox a Maybe/Optional. –  Doval Jul 21 at 17:34
    
I don't like this solution very much exactly because of reasons mentioned by @Doval - returning an union reeks of bad design. As for "favoring composition over inheritance", I may be wrong, but does it really apply in this case? I always thought that the meaning of the rule is that when there is some common functionality that needs to be reused in multiple places, one should favor creating class A encapsulating this common stuff and then reusing it in classes B and C by instantiating A rather than having B and C inheriting from A. –  Nikola Anusev Jul 21 at 20:32

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
    }
}
share|improve this answer
    
You are right, there is a different action for each outcome. However, in this particular case, both actions have something to do with updating UI and I wouldn't like putting UI code to those Result classes. –  Nikola Anusev Jul 21 at 20:39
    
@NikolaAnusev If you want to be really precise about it, you could define a factory interface, that creates the "results" and then inject the implementation of that factory that returns "results" that can update UI. That way you maintain the separation, but you just added 2 more entities and bunch of coupling to solve this problem. –  Euphoric Jul 21 at 20:43
    
.. But that makes it hard to add new results. I guess it is not that good idea. –  Euphoric Jul 21 at 20:49

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.