36

C#'s is operator and Java's instanceof operator allow you to branch on the interface (or, more uncouthly, its base class) an object instance has implemented.

Is it appropriate to use this feature for high level branching based on the capabilities an interface provides?

Or should a base class provide boolean variables to provide an interface to describe the capabilities an object has?

Example:

if (account is IResetsPassword)
       ((IResetsPassword)account).ResetPassword();
else
       Print("Not allowed to reset password with this account type!");

vs.

if (account.CanResetPassword)
       ((IResetsPassword)account).ResetPassword();
else
       Print("Not allowed to reset password with this account type!");

Are their any pitfalls for using interface implementation for capability identification?


This example was just that, an example. I am wondering about a more general application.

  • 10
    Look at your two code examples. Which one is more readable? – Robert Harvey Dec 11 '17 at 17:30
  • 39
    Just my opinion, but I'd go with the first one just because it makes sure you can safely cast to the appropriate type. The second one assumes that CanResetPassword will only be true when something implements IResetsPassword. Now you are making a property determine something the type system should control (and ultimately will when you preform the cast). – Becuzz Dec 11 '17 at 17:36
  • 10
    @nocomprende: xkcd.com/927 – Robert Harvey Dec 11 '17 at 17:45
  • 14
    Your question title and question body ask different things. To answer the title: Ideally, yes. To address the body: If you find yourself checking type and manually casting, you're probably not leveraging your type system correctly. Can you tell us more specifically how you found yourself in this situation? – MetaFight Dec 11 '17 at 17:49
  • 9
    This question might seem simple, but grows quite broad when you start thinking about it. Questions that pop into my mind: Are you expecting to add new behaviors to existing accounts or create account types with different behaviors? Do all account types have similar behaviors or are there big differences between them? Does the example code know about all the types of the accounts or just basic IAccount interface? – Euphoric Dec 11 '17 at 17:53
7

With the caveat that it's best to avoid downcasting if possible, then the first form is preferable for two reasons:

  1. you're letting the type system work for you. In the first example, if the is fires then the downcast will definitely work. In the second form, you need to manually ensure that only objects that implement IResetsPassword return true for that property
  2. fragile base class. You need to add a property to the base class/interface for every interface you want to add. This is cumbersome and error-prone.

That's not to say you can't ameliorate these issues somewhat. You could, for example, have the base class have a set of published interfaces that you can check inclusion in. But you're really just manually implementing a portion of the existing type system which is redundant.

BTW my natural preference is composition over inheritance but mostly as it regards to state. Interface inheritance isn't as bad, usually. Also, if you find yourself implementing a poor man's type hierarchy, you're better off using the one that's already there as the compiler will help you.

  • I also very much prefer compositional typing. +1 for making me see that this particular example is not leveraging it properly. Although, I would say computational typing (like traditional inheritance) is more limited when capabilities vary widely (as opposed to how they are implemented). Hence the interfacing approach, which solves a different problem than either compositional typing or standard inheritance. – TheCatWhisperer Dec 11 '17 at 18:45
  • you could simplify the check like so: (account as IResettable)?.ResetPassword(); or var resettable = account as IResettable; if(resettable != null) {resettable.ResetPassword()} else {....} – Berin Loritsch Dec 11 '17 at 18:52
89

Should an object's capabilities be identified exclusively by the interfaces it implements?

An objects capabilities should not be identified at all.

The client using an object shouldn't be required to know anything about how it works. The client should only know things it can tell the object to do. What the object does, once it's been told, is not the clients problem.

So rather than

if (account is IResetsPassword)
    ((IResetsPassword)account).ResetPassword();
else
    Print("Not allowed to reset password with this account type!");

or

if (account.CanResetPassword)
    ((IResetsPassword)account).ResetPassword();
else
    Print("Not allowed to reset password with this account type!");

consider

account.ResetPassword();

or

account.ResetPassword(authority);

because account already knows if this will work. Why ask it? Just tell it what you want and let it do whatever it's going to do.

Imagine this done in such a way that the client doesn't care if it worked or not because that's something elses problem. The clients job was only to make the attempt. It's done that now and it's got other things to deal with. This style has many names but the name I like the most is tell, don't ask.

It's very tempting when writing the client to think you have to keep track of everything and so pull towards you everything you think you need to know. When you do that you turn objects inside out. Value your ignorance. Push the details away and let the objects deal with them. The less you know the better.

  • 54
    Polymorphism only works when I don't know or care exactly what I'm talking to. So the way I deal with that situation is to carefully avoid it. – candied_orange Dec 11 '17 at 21:58
  • 7
    If the client really does need to know if the attempt succeeded, the method could return a boolean. if (!account.AttemptPasswordReset()) /* attempt failed */; This way the client knows the result but avoids some of the issues with the decision logic in the question. If the attempt is asynchronous, you'd pass a callback. – Radiodef Dec 12 '17 at 3:14
  • 5
    @DavidZ We both speak English. So I can tell you to upvote this comment twice. Does that mean you're capable of doing it? Do I need to know if you can before I can tell you to do it? – candied_orange Dec 12 '17 at 3:14
  • 5
    @QPaysTaxes for all you know an error handler was passed into account when it was constructed. Popups, logging, printouts, and audio alerts could be happening at this point. The client's job is to say "do it now". It doesn't have to do more than that. You don't have to do everything in one place. – candied_orange Dec 12 '17 at 4:34
  • 6
    @QPaysTaxes It could be handled elsewhere and in a number of different ways, it's superfluous to this conversation and would only muddy the waters. – Tom.Bowen89 Dec 12 '17 at 9:21
17

Background

Inheritance is a powerful tool that serves a purpose in object-oriented programming. However, it does not solve every problem elegantly: sometimes, other solutions are better.

If you think back to your early computer science classes (assuming you have a CS degree) you may remember a professor giving you a paragraph that states what the customer wants the software to do. Your job is to read the paragraph, identify the actors and actions, and come away with a rough outline of what the classes and methods are. There will be some bum leads in there that look like they are important, but are not. There is a very real possibility of misinterpreting requirements as well.

This is an important skill that even the most experienced of us get wrong: properly identifying requirements and translating them into machine languages.

Your Question

Based on what you have written, I think you may be misunderstanding the general case of optional actions that classes can perform. Yes, I know your code is just an example and you are interested in the general case. However, it sounds like you want to know how to handle the situation where certain subtypes of an object can perform an action, but other subtypes cannot.

Just because an object such as an account has an account type does not mean that translates into a type in an OO language. "Type" in a human language does not always mean "class." In the context of an account, "type" may more closely correlate with "permission set." You want to use a user account to perform an action, but that action may or may not be able to be performed by that account. Rather than using inheritance, I would use a delegate or security token.

My Solution

Consider an account class that has several optional actions it can perform. Instead of defining "can perform action X" via inheritance, why not have the account return a delegate object (password resetter, form submitter, etc) or an access token?

account.getPasswordResetter().doAction();
account.getFormSubmitter().doAction(view.getContents());

AccountManager.resetPassword(account, account.getAccessToken());

The benefit to the last option there is what if I want to use my account credentials to reset someone else's password?

AccountManager.resetPassword(otherAccount, adminAccount.getAccessToken());

Not only is the system more flexible, not only have I removed type casts, but the design is more expressive. I can read this and easily understand what it is doing and how it can be used.


TL;DR: this reads like an XY problem. Generally when faced with two options that are suboptimal, it is worth taking a step back and thinking "what am I really trying to accomplish here? Should I really be thinking about how to make the typecasting less ugly, or should I look for ways to remove the typecast entirely?"

  • 1
    +1 for the design smells, even if you didn't use the phrase. – Jared Smith Dec 12 '17 at 13:41
  • what about cases where reset password has completely different arguments depending on the type? ResetPassword(pswd) vs ResetPasswordToRandom() – TheCatWhisperer Dec 12 '17 at 14:02
  • 1
    @TheCatWhisperer The first example is "change password" which is a different action. The second example is what I describe in this answer. – user22815 Dec 12 '17 at 14:38
  • Why not account.getPasswordResetter().resetPassword();. – AnoE Dec 12 '17 at 15:18
  • 1
    The benefit to the last option there is what if I want to use my account credentials to reset someone else's password?.. easy. You create an Account domain object for the other account and use that one. Static classes are problematic for unit testing (that method will by definition need access to some storage, so now you can't easily unit test any code that touches that class any more) and best avoided. The option of returning some other interface is often a good idea but doesn't really deal with the underlying problem (you just moved the problem to the other interface). – Voo Dec 12 '17 at 20:51
1

Bill Venner says that your approach is absolutely fine; jump to the section entitled 'When to Use instanceof'. You are downcasting to a specific type/interface that only implements that specific behaviour so you're absolutely right to be selective about it.

However, if you want to use another approach, there are many ways to shave a yak.

There is the polymorphism approach; you could argue that all accounts have a password, therefore all accounts should at least be able to attempt to reset a password. This means that, in the base account class, we should have a resetPassword() method that all accounts implement but in their own way. The question is how that method should behave when the capability is not there.

It could return a void and silently complete whether it reset the password or not, maybe taking responsibility for printing out the message internally if it doesn't reset the password. Not the best idea.

It could return a boolean indicating whether the reset was successful. Switching on that boolean, we could relate that password reset failed.

It could return a String indicating the result of the password reset attempt. The string could give more details on why a reset failed and could be output.

It could return a ResetResult object that conveys more details and combines all of the previous return elements.

It could return a void and instead throw an exception if you try to reset an account that doesn't have that capability (don't do this as using exception handling for normal flow control is bad practice for a variety of reasons).

Having a matching canResetPassword() method might not seem like the worst thing in the world as notionally it is a static capability built-in to the class when it was written. This is a clue as to why the method approach is a bad idea, though, as it suggests that the capability is dynamic and that canResetPassword() might change, which also creates the added possibility that it might change between asking for permission and making the call. As mentioned elsewhere, tell rather than asking permission.

Composition over inheritance could be an option: you could have a final passwordResetter field (or an equivalent getter) and equivalent class(es) that you can check for null for before calling it. While it acts a bit like asking for permission, finality might avoid any inferred dynamic nature.

You might think to externalise the functionality into its own class which could take an account as a parameter and act on it (e.g. resetter.reset(account)), although this is also often bad practice (a common analogy is a shopkeeper reaching into your wallet to get cash).

In languages with the capability, you might use mixins or traits, however, you end up in the position where you started where you might be checking for the existence of those capabilities.

  • Not all accounts may have a password (from the perspective of this particular system anyway). For example the account my be 3rd party... google, facebook, ect. Not to say that this isn't a great answer! – TheCatWhisperer Dec 13 '17 at 15:20
0

For this specific example of the "can reset a password", I'd recommend using composition over inheritance (in this case, inheritance of an interface/contract). Because, by doing this:

class Foo : IResetsPassword {
    //...
}

You're immediately specifying (at compile time) that your class 'can reset a password'. But, if in your scenario, the capability's presence is conditional and depends on other things, then you can't specify things at compile time anymore. Then, I suggest doing this:

class Foo {

    PasswordResetter passwordResetter;

}

Now, at runtime, you can check if myFoo.passwordResetter != null before doing this operation. If you want to decouple stuff even more (and you plan to add many more capabilities), you could:

class Foo {
    //... foo stuff
}

class PasswordResetOperation {
    bool Execute(Foo foo) { ... }
}

class SendMailOperation {
    bool Execute(Foo foo) { ... }
}

//...and you follow this pattern for each new capability...

UPDATE

After I read some other answers and comments from OP I understood the question is not about the compositional solution. So I think the question is about how to better identify capabilities of objects in general, in a scenario like below:

class BaseAccount {
    //...
}
class GuestAccount : BaseAccount {
    //...
}
class UserAccount : BaseAccount, IMyPasswordReset, IEditPosts {
    //...
}
class AdminAccount : BaseAccount, IPasswordReset, IEditPosts, ISendMail {
    //...
}

//Capabilities

interface IMyPasswordReset {
    bool ResetPassword();
}

interface IPasswordReset {
    bool ResetPassword(UserAccount userAcc);
}

interface IEditPosts {
    bool EditPost(long postId, ...);
}

interface ISendMail {
    bool SendMail(string from, string to, ...);
}

Now, I'll try to analyze all options mentioned:

OP second example:

if (account.CanResetPassword)
       ((IResetsPassword)account).ResetPassword();
else
       Print("Not allowed to reset password with this account type!");

Let's say this code is receiving some base account class (eg: BaseAccount in my example); this is bad since it's inserting booleans in the base class, polluting it with code that makes no sense at all to be there.

OP first example:

if (account is IResetsPassword)
       ((IResetsPassword)account).ResetPassword();
else
       Print("Not allowed to reset password with this account type!");

To answer the question, this is more appropriate than the previous option, but depending on the implementation it will break L principle of solid, and probably checks like this would be spread through the code and make further maintenance more difficult.

CandiedOrange's anser:

account.ResetPassword(authority);

If this ResetPassword method is inserted in BaseAccount class, then it's also polluting the base class with inappropriate code, like in OP's second example

Snowman's answer:

AccountManager.resetPassword(otherAccount, adminAccount.getAccessToken());

This is a good solution, but it considers that the capabilities are dynamic (and might change over time). However, after I read several comments from OP I guess the talk here is regarding polymorphism and statically defined classes (although the example of accounts intuitivelly points to the dynamic scenario). EG: in this AccountManager example the check for permission would be queries to DB; in the OP question the checks are attempts of casting the objects.

Another suggestion from me:

Use Template Method pattern for high level branching. The class hierarchy mentioned is kept as it is; we only create more appropriate handlers for the objects, in order to avoid casts and inappropriate properties/methods poluting the base class.

//Template method
class BaseAccountOperation {

    BaseAccount account;

    void Execute() {

        //... some processing

        TryResetPassword();

        //... some processing

        TrySendMail();

        //... some processing
    }

    void TryResetPassword() {
        Print("Not allowed to reset password with this account type!");
    }

    void TrySendMail() {
        Print("Not allowed to reset password with this account type!");
    }
}

class UserAccountOperation : BaseAccountOperation {

    UserAccount userAccount;

    void TryResetPassword() {
        account.ResetPassword(...);
    }

}

class AdminAccountOperation : BaseAccountOperation {

    AdminAccount adminAccount;

    override void TryResetPassword() {
        account.ResetPassword(...);
    }

    void TrySendMail() {
        account.SendMail(...);
    }
}

You could bind the operation to the appropriate account class by using a dictionary/hashtable, or perform run-time operations using extension methods, use dynamic keyword, or as last option use only one cast in order to pass the account object to the operation (in this case the number of casts is only one, at the beginning of the operation).

  • 1
    Would it work to just always implement the "Execute()" method but sometimes have it do nothing? It returns a bool, so I am guessing that means it did it or not. That throws it back to the client: "I tried to reset password, but it didn't happen (for whatever reason), so now I should..." – user251748 Dec 11 '17 at 19:25
  • 4
    Having "canX()" and "doX()" methods is an antipattern: if an interface exposes a "doX()" method, it better be able to do X even if doing X means a no-op (e.g. null object pattern). It should never be incumbent on users of an interface to engage in temporal coupling (invoke method A before method B) because that is too easy to get wrong and break things. – user22815 Dec 11 '17 at 20:15
  • 1
    Having interfaces has no bearing on using composition over inheritance. They just represent a functionality that is available. How that functionality happens to come into play is independent of the use of the interface. What you have done here is use an ad-hoc implementation of an interface without any of the benefits. – Miyamoto Akira Dec 11 '17 at 21:28
  • Interesting: the top-voted answer says basically what my comment above says. Some days you just get lucky. – user251748 Dec 12 '17 at 16:34
  • @nocomprende - yes, I updated my answer according to several comments. Thanks for the feedbacks :) – Emerson Cardoso Dec 12 '17 at 16:39
0

Neither of the options you have presented are good OO. If you are writing if statements around the type of an object, you are most likely doing OO wrong (there are exceptions, this isn't one.) Here's the simple OO answer to your question (may not be valid C#):

interface IAccount {
  bool CanResetPassword();

  void ResetPassword();

  // Other Account operations as needed
}

public class Resetable : IAccount {
  public bool CanResetPassword() {
    return true;
  }

  public void ResetPassword() {
    /* RESET PASSWORD */
  }
}

public class NotResetable : IAccount {
  public bool CanResetPassword() {
    return false;
  }

  public void ResetPassword() {
    Print("Not allowed to reset password with this account type!");}
  }

I've modified this example to match what the original code was doing. Based on some of the comments, it seems people are getting hung up on whether this is the 'right' specific code here. That is not the point of this example. The whole Polymorphic overloading is essentially to conditionally execute different implementations of logic based on the type of the object. What you are doing in both examples is hand-jamming what your language gives you as a feature. In a nutshell you could get rid of the sub-types and put the ability to reset as a boolean property of the Account type (ignoring other features of the sub-types.)

Without a wider view of the design, it's impossible to tell whether this is a good solution for your particular system. It's simple and if it works for what you are doing, you will likely never need to think much about it again unless someone fails to check CanResetPassword() prior to calling ResetPassword(). You could also return a boolean or fail silently (not recommended). It really depends on the specifics of the design.

  • Not all accounts can be reset. Also, might an account have more functionality than being resetable? – TheCatWhisperer Dec 11 '17 at 20:41
  • 2
    "Not all accounts can be reset." Not sure if this was written before edit. Second class is NotResetable. "might an account have more functionality than being resetable" I would expect so but it doesn't seem to be important to the question at hand. – JimmyJames Dec 11 '17 at 20:42
  • 1
    This is solution goes on the right direction, I think, as it provides a nice solution inside OO. Probably would change the name the interface name to IPasswordReseter (or something to that effect), to segregate interfaces. IAccount could be declared as supporting multiple other interfaces. That would give you the option to have classes with the implementation that is then composed and used by delegation on the domain objects. @JimmyJames, minor nitpick, on C# both classes will need to specify that they implement IAccount. Otherwise the code looks a bit strange ;-) – Miyamoto Akira Dec 11 '17 at 21:41
  • 1
    Also, check a comment from Snowman in one of the other answers about CanX() and DoX(). Not always an anti-pattern, though, as it has it uses (for example on UI behaviour) – Miyamoto Akira Dec 11 '17 at 21:44
  • 1
    This answer starts out well: this is bad OOP. Unfortunately your solution is just as bad because it also subverts the type system but throws an exception. A good solution looks different: don’t have unimplemented methods on a type. The .NET framework actually uses your approach for some types but that was unambiguously a mistake. – Konrad Rudolph Dec 12 '17 at 14:12
0

Answer

My slightly opinionated answer to your question is:

The capabilities of an object should be identified through themselves, not by implementing an interface. A statically, strongly typed language will limit this possibility, though (by design).

Addendum:

Branching on object type is evil.

Rambling

I see that you did not add the c# tag, but are asking in a general object-oriented context.

What you are describing is a technicality of static/strong typed languages, and not specific of "OO" at large. Your problem stems, amongst others, from the fact that you cannot call methods in those languages without having a sufficiently narrow type at compile time (either through variable definition, method parameter/return value definition, or an explicit cast). I have done both of your variants in the past, in these kinds of languages, and both seem ugly to me these days.

In dynamically typed OO languages, this problem does not occur due to a concept called "duck typing". In short, this means that you basically do not declare the type of an object anywhere (except when creating it). You send messages to objects, and the objects handle those messages. You don't care about whether the object actually has a method of that name. Some languages even have a generic, catch-all method_missing method which makes this even more flexible.

So, in such a language (Ruby, for example), your code becomes:

account.reset_password

Period.

The account will either reset the password, throw a "denied" exception, or throw a "don't know how to handle 'reset_password'" exception.

If you need to branch explicitly for whatever reason, you would still do so on the capability, not on the class:

account.reset_password  if account.can? :reset_password

(Where can? is just a method which checks whether the object can execute some method, without calling it.)

Note that while my personal preference is currently on dynamically typed languages, this is just a personal preference. Statically typed languages have things going for them as well, so please do not take this rambling as a bash against statically typed languages...

  • Great to have your perspective here! We on the java/C# side tend to forget a different branch of OOP exists. When I tell my colleagues, JS (for all its problems) is, in many ways, more OO than C#, they laugh at me! – TheCatWhisperer Dec 12 '17 at 16:23
  • I love C#, and I think it's a good general purpose language, but it's my personal opinion, that in terms of OO, it is quite poor. In both features and practice, it tends to encourage procedural programming. – TheCatWhisperer Dec 12 '17 at 16:24
  • 2
    Arguably, testing for the existence of a method in a duck-typed language is the same as testing for the implementation of an interface in a statically-typed one: you are performing a run-time check of whether the object has a particular capability. Every method declaration is effectively its own interface, identified only by the method name, and its existence cannot be used to infer any other information about the object. – IMSoP Dec 13 '17 at 14:56
0

It seems that your options are derived from different motivational forces. The first one seems to be a hack employed due to poor object modeling. It demonstrated that your abstractions are wrong, since they break polymorphism. More probably than not it breaks The Open closed principle, which results in a tighter coupling.

Your second option could be fine from OOP perspective, but type casting confuses me. If your account lets you to reset its password, why do you need a typecasting then? So assuming that your CanResetPassword clause represents some fundamental business requirement that is a part of account's abstraction, your second option is OK.

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