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.

TheCatWhisperer
  • 5,231
  • 1
  • 22
  • 41
  • 10
    Look at your two code examples. Which one is more readable? – Robert Harvey Dec 11 '17 at 17:30
  • 2
    Of course, as with all things programming, it depends. You could look at this as "favor composition over inheritance," in which case the second example is better. Or you could look at this as fulfilling a specific contract, in which case the first example is better. – Robert Harvey Dec 11 '17 at 17:32
  • So which of those two rules is better? –  Dec 11 '17 at 17:34
  • also the first example is more "DRY", the second is more flexible if you need it – TheCatWhisperer Dec 11 '17 at 17:35
  • 1
    I guess if your capabilities are set statically (won't change once the app starts running), then using the interface to represent the presence of it would work (although I don't like any of your two examples). But, if in your app, the same account can acquire some capability over time, then this approach of yours won't be suitable enough; then maybe flags would be better; and command pattern for the execution of the operations. – Emerson Cardoso Dec 11 '17 at 17:36
  • 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
  • @Becuzz ideally, in the second one, you'd do a type check too... editing now – TheCatWhisperer Dec 11 '17 at 17:37
  • 1
    @TheCatWhisperer If you do a type check, then what is the point of `CanResetPassword`? At that point you are back to the first example. – Becuzz Dec 11 '17 at 17:38
  • @Becuzz their might be several different ways to reset a password, with different arguments – TheCatWhisperer Dec 11 '17 at 17:39
  • @nocomprende: The one that most effectively meets your specific requirements. – Robert Harvey Dec 11 '17 at 17:44
  • 2
    @TheCatWhisperer I'm not sure how that makes a difference. You ultimately have to cast your object to `IResetPassword` or `IResetPasswordThatTakesABunchOfArguments`. The type system will tell you that (if you do a check and when you actually do the cast). I fail to see what `CanResetPassword` tells you that the type checks don't. – Becuzz Dec 11 '17 at 17:44
  • It is so good that there are so many rules and guidelines to choose from! –  Dec 11 '17 at 17:44
  • 10
    @nocomprende: https://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
  • AI is starting to seem easier and easier. –  Dec 11 '17 at 19:30
  • I disagree that this is too broad: looking at the general case, there is a reasonably-scoped approach. –  Dec 11 '17 at 20:31
  • Once example I can think of is the *[Stream](https://msdn.microsoft.com/en-us/library/system.io.stream%28v=vs.110%29.aspx)*. It has *CanRead* and *CanWrite* to go with *Read* and *Write*. While I sometimes would like to be able to explicitly limit the parameter type by some *OutputStream* or *InputStream*, you would have to have multiple inheritance for other classes, like *FileStream*, for example (with some *InputFileStream* derived from both *InputStream* and *Stream*). – IS4 Dec 12 '17 at 02:07
  • 1
    Another important point - the result of *CanResetPassword* might change over time or depend on other factors. Take it into consideration when deciding which one is better. You cannot change which interface you implement, so if suddenly you couldn't reset the password via *IResetsPassword*, you would have to throw an exception, effectively breaking the purpose you used interfaces for in the first place. – IS4 Dec 12 '17 at 02:10
  • The mere fact that an explicit cast is required tells me something is not quite right with the object structure... Both propositions look just as bad from here. – Newtopian Dec 12 '17 at 22:00
  • Branching on type. Usually bad. [There](https://refactoring.guru/replace-conditional-with-polymorphism) are [many](https://refactoring.guru/smells/switch-statements) design [patterns](https://refactoring.guru/replace-type-code-with-state-strategy) that exist to factor this smell out of your code. – spender Dec 13 '17 at 02:48
  • @IllidanS4 The WinRT stream interfaces have `IInputStream`, `IOutputStream`, `IRandomAccessStream`. Far more sensible that the (usually partially redundant) abstract `Stream`. – spender Dec 13 '17 at 02:53

8 Answers8

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.

candied_orange
  • 102,279
  • 24
  • 197
  • 315
  • great perspective! But, given this approach, how would you deal with a situation where different classes need different arguments? – TheCatWhisperer Dec 11 '17 at 21:53
  • 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
  • 2
    When you say "The client should only know things it can tell the object to do," that sounds like practically the definition of capabilities, so I'm somewhat uneasy about your first sentence. (But I agree with the point you're making.) – David Z Dec 12 '17 at 02:53
  • 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 03: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 03:14
  • 1
    +1, though I find the lack of any error checking odd. I'd fully expect this to return a `bool`, like Radiodef said, indicating success or failure, or throw an exception, or return an `enum`, or something. Otherwise, imagine the user's POV: They click the "reset password" button. Nothing happens -- no error pops up, and there's no password reset. I'd suggest editing your answer to take that into account. – Nic Dec 12 '17 at 04:11
  • 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 04:34
  • 1
    @CandiedOrange Your answer continues to not mention error handling. The specific method is irrelevant. I only mentioned return values because those would be the easiest refactoring to make, based on the code in the question. – Nic Dec 12 '17 at 05:08
  • 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 Bowen Dec 12 '17 at 09:21
  • 2
    @QPaysTaxes "Value your ignorance. Push the details away and let the objects deal with them. The less you know the better." – Caleth Dec 12 '17 at 11:39
  • Interesting: this answer says basically what a comment I made to another answer said, a few hours earlier. –  Dec 12 '17 at 16:37
  • 1
    @nocomprende Good artists copy. Great artists [steal](https://www.quora.com/What-did-Picasso-mean-when-he-said-%E2%80%9Cgood-artists-copy-great-artists-steal%E2%80%9D-Did-he-really-say-this-or-did-someone-else-say-it). But I'm honest enough to [cite](https://softwareengineering.stackexchange.com/questions/362179/should-an-objects-capabilities-be-identified-exclusively-by-the-interfaces-it-i/362192?noredirect=1#comment786870_362182) and upvote your comment. – candied_orange Dec 12 '17 at 16:56
  • 2
    Ignorance is a bliss, sometimes, but you're leaving the behavior of `ResetPassword` unspecified. I agree that the caller usually shouldn't care, but I, as a reader of you answer, do care about what happens, when it's unsupported. Without it, **your answer ignores the problem**, and this is a kind of ignorance, I don't value. Does it throw? Does it do nothing? – maaartinus Dec 12 '17 at 22:30
  • @maaartinus I very carefully made it clear that you don't have to throw and you don't have to do nothing. It sounds like you're asking me to design an example solution. Fine, on construction pass `account` an output port that it can use to tell the user what happens when `ResetPassword` is called. The client still doesn't see any of this. If you really find this idea hard to understand here's some suggested reading: [request vs event driven](http://epthinking.blogspot.com/2012/12/more-on-request-driven-vs-event-driven.html) and [CQRS](https://martinfowler.com/bliki/CQRS.html). – candied_orange Dec 13 '17 at 04:35
  • 1
    What if you want to hide the "Reset password" button, depending on whether it's possible? – BartoszKP Dec 13 '17 at 12:10
  • 1
    @BartoszKP that makes this the wrong event. You'd want to change the button's enabled state either when the permission changes (see observer pattern) or when the button is first displayed. Nether forces you to violate the encapsulation of `account` by adding a `CanResetPassword()` getter or interface. Tell `account` to display the button. `account` passes the event to something that can display the button correctly. Done this way private internal state is not available publicly. [See this example](https://codereview.stackexchange.com/questions/148809/a-button-as-a-clean-architecture-plugin). – candied_orange Dec 13 '17 at 14:49
  • 1
    @CandiedOrange I didn't say anything about violating encapsulation of `account`. I'm just indicating that the solution in your post doesn't cover this - quite common scenario. We prefer to disable parts of GUI, instead of lying to the user that they are available and then displaying a message "yeah, that button, right. Well, we allowed you to click it, but the thing is, at this time you cannot reset the password, sorry" :) So just consider adding this detail to your answer - as it stands now, it suggests the slightly exaggerated scenario I've presented ;) Such cases seem to interest the OP. – BartoszKP Dec 13 '17 at 15:41
  • @BartoszKP I think Tom.Bowen89s [comment](https://softwareengineering.stackexchange.com/questions/362179/should-an-objects-capabilities-be-identified-exclusively-by-the-interfaces-it-i/362192?noredirect=1#comment786938_362192) applyes here as well. If you want an answer focussed on that issue feel free to ask it as a question. I'd be happy to answer it. – candied_orange Dec 13 '17 at 16:09
  • 1
    @CandiedOrange There is nothing about error handling in the OP, whereas the scenario proposed by me matches exactly the problem of "discovering capabilities" (even if this name seems a bit too much ;) ). Your answer basically just says to "let the caller don't care" - which of course valid, but doesn't provide any constructive alternative when we need to know **beforehand** whether we can reset the password or not. It's not a trivial problem, and as I've explained seems to be the main issue here. – BartoszKP Dec 13 '17 at 16:51
  • 1
    Let us [continue this discussion in chat](http://chat.stackexchange.com/rooms/70174/discussion-between-candiedorange-and-bartoszkp). – candied_orange Dec 13 '17 at 16:53
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. –  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
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.

Alex
  • 3,882
  • 1
  • 15
  • 16
  • 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
1

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.

JimmyJames
  • 24,682
  • 2
  • 50
  • 92
  • 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
  • @MiyamotoAkira Good call on the implements. I tried to fix it but if that's not right, feel free to correct it. Snowman's answer is interesting but a fair bit more complicated. – JimmyJames Dec 11 '17 at 22:27
  • 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
  • @KonradRudolph The solution here is isomorphic to the original code. There is no one correct design for all problems. The point of polymorphic overriding is to eliminate if statements around the type of data. You can have an opinion on whether this is good but 'unambiguously' is a level of certainty that implies hubris. – JimmyJames Dec 12 '17 at 15:29
  • @JimmyJames Right, but the original code is *already* bad. Hubris schmubris, there’s a wide consensus among experts that this is an anti-pattern. Why unnecessarily mince words? See CandiedOrange’s answer for a more detailed explanation. – Konrad Rudolph Dec 12 '17 at 17:38
  • @KonradRudolph "there’s a wide consensus among experts that this is an anti-pattern." You should be able to provide a reference or two. "See CandiedOrange’s answer for a more detailed explanation" I see CandiedOrange's answer to very similar. The only distinction here is whether anyone needs to know if you can set the password and what you do if someone calls ResetPassword when it's not allowed. [Failing-fast](https://martinfowler.com/ieeeSoftware/failFast.pdf) is a valid choice. – JimmyJames Dec 12 '17 at 18:21
  • @JimmyJames No, CandiedOrange’s answer is fundamentally different from yours. It correctly implements Fowler’s maxim “[tell, don’t ask](https://softwareengineering.stackexchange.com/a/157527/2366)”. But more generally this is an instance of “[interface bloat](https://en.wikipedia.org/wiki/Interface_bloat)”. See also Meyers’ *Effective C++*. – Konrad Rudolph Dec 13 '17 at 09:48
  • @KonradRudolph interface bloat "is when a computer interface incorporates too many operations on some data into an interface, only to find that most of the objects cannot perform the given operations." The idea that this describes having a method that informs you whether an account has the ability to reset it's password is sheer nonsense. I'd think you were joking but it seems not. – JimmyJames Dec 13 '17 at 14:38
  • @JimmyJames The relevant part of that sentence is “only to find that most of the objects cannot perform the given operations”. The phrasing is still bad though, and other sources have clearer phrasing. It would be better stated as: “a type shall not have methods it cannot implement”. This is the contrary of “sheer nonsense”. – Konrad Rudolph Dec 13 '17 at 14:44
  • But at any rate you seem to have misunderstood my comment: having a method that informs you whether an object has an ability is *not* bad (though “tell, don’t ask” still applies). But having a method which informs you that *the class* doesn’t have a capability — that’s bad. And *this* is the part I’m objecting to, and that’s covered by the engineering guidelines I’m referring to. – Konrad Rudolph Dec 13 '17 at 14:45
  • @KonradRudolph Let's say you are building a UI and the requirement is that you don't show an option to reset passwords for accounts that don't have that are not able/allowed to do so. You have a class that encapsulates logic for a type of account that will never have that option. What do you do? – JimmyJames Dec 13 '17 at 14:59
  • @KonradRudolph It would be better stated as: “a type shall not have methods it cannot implement”. It has an implementation. That implementation is to throw an exception. Or if you want you can just do nothing. There's no way to know from the detail in the question which is the best option for the OP. The question just shows printing a statement which I hope isn't really the plan. – JimmyJames Dec 13 '17 at 15:02
  • @JimmyJames If you follow MVC? Have different view types for different account types that are coupled via dependency injection. Then the view for an account that can’t be reset doesn’t display the relevant option. — Regarding your other comment: the “throws an exception” in this case is the *lack* of a meaningful implementation. And it converts a compile time type error into a runtime exception. This subverts static typing. – Konrad Rudolph Dec 13 '17 at 15:02
  • @KonradRudolph "view types for different account types that are coupled via dependency injection" So you take that property of the account and manage it in your view layer? I'm not really talking MVC per se, just in general. I thought MVC was passe anyway. – JimmyJames Dec 13 '17 at 15:06
  • @JimmyJames The latest fad in GUI programming has equivalent solutions. CandiedOrange gives a simplified solution to the equivalent question in a comment to their answer: let the account object *itself* draw the button. Point being: you don’t need to separate model and view, if you don’t want to. – Konrad Rudolph Dec 13 '17 at 15:07
  • @KonradRudolph "let the account object itself draw the button." So I have a web ui with a rest backend. The domain object is going to tell me what web components to put on the screen? Does it manage the details required to do things right in various web browswers? What about language support? – JimmyJames Dec 13 '17 at 15:11
  • @JimmyJames At this point I’m unsure what your question is. At any rate, every language supports this, if it supports drawing a button. Same answer for doing the right thing in various web browsers: that’s unrelated from the question of which entity decides whether to render a button. – Konrad Rudolph Dec 13 '17 at 15:13
  • @KonradRudolph So let's be clear, it's OK (simpler even) for a domain object to describe how it should be displayed on a screen but having a boolean method that provides a simple answer to "do I have capability X" is always incorrect? – JimmyJames Dec 13 '17 at 15:13
  • Let us [continue this discussion in chat](http://chat.stackexchange.com/rooms/70166/discussion-between-jimmyjames-and-konrad-rudolph). – JimmyJames Dec 13 '17 at 15:14
  • It should *issue* that description. It should (usually) not do the actual drawing itself (duh). — And, again: it’s *not* always incorrect to have a boolean getter for an object’s property (though, again, you *generally* don’t want that: tell, don’t ask. But I’m not dogmatic about this). But this is orthogonal to the question whether types should have methods they cannot implement meaningfully. That is *always* wrong. Anyway, at this point I’m simply repeating myself so I think we should stop now. – Konrad Rudolph Dec 13 '17 at 15:19
  • @KonradRudolph So how is the CandiedOrange's answer (which I think is a perfectly fine option) avoiding having a method that the type cannot implement? The method is there and does nothing (or at least it's not specified what it doe). How does adding a metadata method change that? – JimmyJames Dec 13 '17 at 15:22
  • @KonradRudolph "It should issue that description. It should (usually) not do the actual drawing itself (duh)." So it's OK for the object to issue a description that tells the UI that it doesn't have an capability but it's not OK to have a boolean method providing that same information? – JimmyJames Dec 13 '17 at 15:24
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.

Danikov
  • 119
  • 2
  • 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).

Emerson Cardoso
  • 2,050
  • 7
  • 14
  • 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..." –  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](https://en.wikipedia.org/wiki/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. –  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. –  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

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

AnoE
  • 5,614
  • 1
  • 13
  • 17
  • 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.

Vadim Samokhin
  • 2,158
  • 1
  • 12
  • 17