12

Scenario

A web application defines a user backend interface IUserBackend with the methods

  • getUser(uid)
  • createUser(uid)
  • deleteUser(uid)
  • setPassword(uid, password)
  • ...

Different user backends (e.g. LDAP, SQL,...) implement this interface but not every backend can do everything. For example a concrete LDAP server does not allow this web application to delete users. So the LdapUserBackend class that implements IUserBackend will not implement deleteUser(uid).

The concrete class needs to communicate to the web application what the web application is allowed to do with the backend's users.

Known solution

I have seen a solution where the IUserInterface has a implementedActions method that returns a integer which is the result of bitwise ORs of the actions bitwise ANDed with the requested actions:

function implementedActions(requestedActions) {
    return (bool)(
        ACTION_GET_USER
        | ACTION_CREATE_USER
        | ACTION_DELTE_USER
        | ACTION_SET_PASSWORD
        ) & requestedActions)
}

Where

  • ACTION_GET_USER = 1
  • ACTION_CREATE_USER = 2
  • ACTION_DELETE_USER = 4
  • ACTION_SET_PASSWORD = 8
  • .... = 16
  • .... = 32

etc.

So the web application sets a bitmask with what it needs and implementedActions() answers with a boolean whether it supports them.

Opinion

These bit operations to me look like relics from the C age, not necessarily easy to understand in terms of clean code.

Question

What is a modern (better?) pattern for the class to communicate the subset of the interfaces methods it implements? Or is the "bit operation method" from above still the best practice?

(In case it matters: PHP, although I am looking for a general solution for OO languages)

  • 5
    The general solution is to split the interface. The IUserBackend should not contain the deleteUser method at all. That should be part of IUserDeleteBackend (or whatever you want to call it). Code that needs to delete users will have arguments of IUserDeleteBackend, code that doesn't need that functionality will use IUserBackend and wont have any troubles with unimplemented methods. – Bakuriu Apr 27 '18 at 8:08
  • 3
    An important design consideration is whether the availability of an action depends on runtime circumstances. Is it all LDAP servers that don't support deletion? Or is that a property of the configuration of a server, and might change with a system restart? Should your LDAP connector automatically discover this situation, or should it require that the configuration is changed to plug in a different LDAP connector with different capabilities? These things have a strong impact on which solutions are viable. – Sebastian Redl Apr 27 '18 at 8:36
  • @SebastianRedl Yes, that is something that I did not take into account. I actually need a solution for runtime. Since I did not want to invalidate the very good answers, I opened a new question that focuses on runtime – problemofficer Apr 27 '18 at 18:28
24

Broadly speaking, there are two approaches you can take here: test & throw or composition via polymorphism.

Test & throw

This is the approach you already describe. Via some means, you indicate to the user of the class whether certain other methods are implemented or not. This can be done with a single method and a bitwise enumeration (as you describe), or via a series of supportsDelete() etc methods.

Then, if supportsDelete() returns false, calling deleteUser() might result in a NotImplementedExeption being thrown, or the method just not doing anything.

This is a popular solution amongst some, as it's simple. However, many - myself included - argue that it is a violation of Liskov's substitution principle (the L in SOLID) and therefore is not a nice solution.

Composition via Polymorphism

The approach here is to view IUserBackend as far too blunt an instrument. If classes cannot always implement all the methods in that interface, then break up the interface into more focused parts. So you might have: IGeneralUser IDeletableUser IRenamableUser ... In other words, all the methods that all your backends can implement go in IGeneralUser and you create a separate interface for each of the actions that only some can perform.

That way, LdapUserBackenddoesn't implement IDeletableUser and you test for that using a test such as (using C# syntax):

if (backend is IDeletableUser deletableUser)
{
    deletableUser.deleteUser(id);
}

(I'm not sure of the mechanism in PHP for determining if an instance implements an interface and how you then cast to that interface, but I'm sure there's an equivalent in that language)

The advantage of this method is that it makes good use of polymorphism to enable your code to comply with SOLID principles and is just way more elegant in my view.

The downside is that it can become unwieldy all too easily. If, for example, you end up having to implement dozens of interfaces because every concrete backend has slightly different capabilities, then this isn't a good solution. So I'd simply advise you use your judgement on whether this approach is practical for you on this occasion and use it, if it is.

  • 4
    +1 for SOLID design considerations. Always nice to show answers with different approaches that will keep the code cleaner moving forward! – Caleb Apr 27 '18 at 7:26
  • 2
    in PHP it would be if (backend instanceof IDelatableUser) {...} – Rad80 Apr 27 '18 at 10:58
  • You already mention violation of LSP. I agree, but wanted to slightly add to it: Test&Throw is valid if the input value makes it impossible to perform the action, e.g. passing a 0 as the divisor in a Divide(float,float) method. The input value is variable, and the exception covers a small subset of possible executions. But if you throw based on your implementing type, then its inability to execute is a given fact. The exception covers all possible inputs, not just a subset of them. That's like putting a "wet floor" sign on every wet floor in a world where every floor is always wet. – Flater Apr 27 '18 at 12:22
  • There is an exception (pun not intended) for the principle of not throwing on a type. For C#, that is is NotImplementedException. This exception is intended for temporary outages, i.e. code that is not yet developed but will be developed. That's not the same as definitively deciding that a given class will never do anything with a given method, even after the development is complete. – Flater Apr 27 '18 at 12:27
  • Thank you for the answer. I actually needed a runtime solution but failed to emphasize it in my question. Since I did not want to invaliate your answer, I decided to create a new question. – problemofficer Apr 27 '18 at 18:30
5

The current situation

The current setup violates the Interface Segregation Principle (the I in SOLID).

Reference

According to Wikipedia the interface segregation principle (ISP) states that no client should be forced to depend on methods it does not use. The interface segregation principle was formulated by Robert Martin in the mid 1990s.

In other words, if this is your interface:

public interface IUserBackend
{
    User getUser(int uid);
    User createUser(int uid);
    void deleteUser(int uid);
    void setPassword(int uid, string password);
}

Then every class that implements this interface must utilize every listed method of the interface. No exception.

Imagine if there's a generalized method:

public void HaveUserDeleted(IUserBackend backendService, User user)
{
     backendService.deleteUser(user.Uid);
}

If you were to actually make it so that only some of the implementing classes are actually able to delete a user, then this method will occasionally blow up in your face (or do nothing at all). That is not good design.


Your proposed solution

I have seen a solution where the IUserInterface has a implementedActions method that returns a integer which is the result of bitwise ORs of the actions bitwise ANDed with the requested actions.

What you essentially want to do is:

public void HaveUserDeleted(IUserBackend backendService, User user)
{
     if(backendService.canDeleteUser())
         backendService.deleteUser(user.Uid);
}

I'm ignoring how exactly we determine whether a given class is able to delete a user. Whether it's a boolean, a bit flag, ... doesn't matter. It all boils down to a binary answer: can it delete a user, yes or no?

That would solve the problem, right? Well, technically, it does. But now, you're violating the Liskov Substitution Principle (the L in SOLID).

Forgoing the rather complex Wikipedia explanation, I found a decent example on StackOverflow. Take note of the "bad" example:

void MakeDuckSwim(IDuck duck)
{
    if (duck is ElectricDuck)
        ((ElectricDuck)duck).TurnOn();

    duck.Swim();
}

I assume you see the similarity here. It's a method that is supposed to handle an abstracted object (IDuck, IUserBackend), but because of a compromised class design, it is forced to first handle specific implementations (ElectricDuck, ensure it's not a IUserBackend class that can't delete users).

This defeats the purpose of developing an abstracted approach.

Note: The example here is easier to fix than your case. For the example, it suffices to have the ElectricDuck turn itself on inside the Swim() method. Both ducks are still able to swim, so the functional result is the same.

You may want to do something similar. Don't. You can't just pretend to delete a user but in reality have an empty method body. While this does work from a technical perspective, it makes it impossible to know whether or not your implementing class will actually do something when asked to do something. That is a breeding ground for unmaintainable code.


My proposed solution

But you said that it's possible (and correct) for an implementing class to only handle some of these methods.

For the sake of example, let's say that for every possible combination of these methods, there is a class that will implement it. It covers all our bases.

The solution here is to split the interface.

public interface IGetUserService
{
    User getUser(int uid);
}

public interface ICreateUserService
{
    User createUser(int uid);
}

public interface IDeleteUserService
{
    void deleteUser(int uid);
}

public interface ISetPasswordService
{
    void setPassword(int uid, string password);
}

Note that you could've seen this coming in the beginning of my answer. The Interface Segregation Principle name already reveals that this principle is designed to make you segregate the interfaces to a sufficient degree.

This allows you to mix-and-match interfaces as you please:

public class UserRetrievalService 
              : IGetUserService, ICreateUserService
{
    //getUser and createUser methods implemented here
}

public class UserDeleteService
              : IDeleteUserService
{
    //deleteUser method implemented here
}

public class DoesEverythingService 
              : IGetUserService, ICreateUserService, IDeleteUserService, ISetPasswordService
{
    //All methods implemented here
}

Every class can decide which they want to do, without every breaking the contract of their interface.

This also means that we don't need to check if a certain class is able to delete a user. Every class that implements the IDeleteUserService interface will be able to delete a user = No violation of the Liskov Substitution Principle.

public void HaveUserDeleted(IDeleteUserService backendService, User user)
{
     backendService.deleteUser(user.Uid); //guaranteed to work
}

If anyone tries to pass an object that does not implement IDeleteUserService, the program will refuse to compile. This is why we like having type safety.

HaveUserDeleted(new DoesEverythingService());    // No problem.
HaveUserDeleted(new UserDeleteService());        // No problem.
HaveUserDeleted(new UserRetrievalService());     // COMPILE ERROR

Footnote

I took the example to an extreme, segregating the interface into the smallest possible chunks. However, if your situation is different, you can get away with bigger chunks.

For example, if every service that can create a user is always capable of deleting a user (and vice versa), you can keep these methods as part of a single interface:

public interface IManageUserService
{
    User createUser(int uid);
    void deleteUser(int uid);
}

There is no technical benefit do doing this instead of separating to the smaller chunks; but it will make development slightly easier because it requires less boilerplating.

  • +1 for splitting interfaces up by the behavior they support, which is the whole purpose of an interface. – Greg Burghardt Apr 27 '18 at 17:00
  • Thank you for the answer. I actually needed a runtime solution but failed to emphasize it in my question. Since I did not want to invaliate your answer, I decided to create a new question. – problemofficer Apr 27 '18 at 18:30
  • @problemofficer: Runtime evaluation for these cases is rarely the best option, but there are indeed cases for doing so. In such a case, you either create a method that can be called but may end up not doing anything (call it TryDeleteUser to reflect that); or you have the method willfully throw an Exception if it's a possible-but-problematic situation. Using a CanDoThing() and DoThing() method approach works, but it would require your external callers to use two calls (and get punished for failing to do so), which is less intuitive and not as elegant. – Flater May 1 '18 at 10:11
0

If you want to use higher level types, you could go with the set type in your language of choice. Hopefully it provides some syntax sugar for doing set intersections and subset-determination.

This is basically what Java does with EnumSet (minus the syntax sugar, but hey, it's Java)

0

In the .NET world you can decorate methods and classes with custom attributes. This may not be relevant to your case.

It sounds to me though that the issue you have may be more to do with a higher level of the design.

If this is a UI feature, such as an edit users page or component, then how are the different capabilities being masked out? In this case 'test and throw' will be a pretty inefficient approach for that purpose. It assumes that prior to loading every page you run a mock call to each function to determine if the widget or element should be hidden, or presented differently. Alternatively, you have a web page that basically forces the user to discover what is available by manual 'test and throw,' whichever coding route you take, because the user does not discover that something is unavailable until a pop-up warning shows.

So for a UI, you might want to look into how you do feature management, and tie the choice of available implementations to that, rather than have the selected implementations drive what features can be managed. You might want to look at frameworks for composing feature dependencies, and explicitly define the capabilities as entities in your domain model. This could even be tied into authorization. Essentially, deciding if a capability is available or not based on authorization level can be extended to deciding if a capability is actually implemented, and then high level UI 'features' can have explicit mappings to capability sets.

If this is a Web API, then the overall design choice may be complicated by having to support multiple public versions of the 'Manage User' API or 'User' REST resource as the capabilities expand over time.

So to summarise, in the .NET world you can exploit various Reflection/Attribute ways of determining in advance which classes implement what, but in any case it seems that the real issues are going to be in what you do with that information.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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