0

I am mainly concerned about how to create concrete objects properly when implementing different interfaces.

I have a resource whose variants can support different interfaces. For example, say I have two types of cards, C1 and C2. C1 supports blocking and unblocking of cards. C2 does not support that but the rest of the features are the all same.

So, I have implemented an abstract class, which holds the common denominator C. C1and C2 extends C.

For supporting the feature of blocking and unblocking cards, we have implemented an interface, BlockInterface to be implemented by C1.

This is going to be an API(web). Now when the call comes involving any card, the first thing I want to do is to create a concrete class card, of the proper type. For that, I need to put if else ladder in the client code. Anytime a new card type is added to the system, I have to go there and add one more stair to the ladder. That does not look right and also seems to violate SRP as the client code is mainly responsible for taking the order.

  1. So, I have created a factory for it. It takes the data and returns proper concrete object depending on the data set it is getting. Is it the right approach or is it ok to put the conditionals in client code for proper object creation?

  2. Now say some malicious user tries blocking a card which does not support the block card feature. I think I can not leverage my CardFactory because the return type of CardFactory is C and that does not satisfy the type expected by code calling .blockCard. So either I need to change the placeholder to C and check if the method exists or not. Or I can put the logic in the constructor only to check if the card supports the feature and then throw an error if not. But this approach leads to some redundant work in the factory and the constructor. If I remove the factory then every time a new card is created I need to do an if-else check. What's the best way to achieve it?

6

TL;DR

You're violating the Open/Closed Principle by writing your code in a way that requires constant updating when classes are added to the assembly. You're also violating LSP by expecting that a base class can do more than its definition defines.

Note: The link refers to LSP, but the top answer show a workaround which in turn violates OCP, and that is the same as what you're trying to do in your case.

The example bad code in the linked answer:

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

    duck.Swim();
}

is the same as what you're describing:

void MakeCardDoSomething(C c)
{
    if (c is IBlockingC)
        ((IBlockingC)c).Block();

    c.DoSomething();
}

The rest of the answer explores your question and expectations of how things should work. But any answer that addresses OCP will also answer your question. I'm just trying to put it closer to home for you.


Wrong expectations

C1 supports blocking and unblocking of cards. C2 does not support that but the rest of the features are the all same.

So, I have implemented an abstract class, which holds the common denominator C. C1and C2 extends C.

Just to elaborate: this means that C does not contain any blocking logic. Someone who handles a C type object has no reasonable expectation to use blocking features.

Now say some malicious user tries blocking a card which does not support the block card feature. I think I can not leverage my CardFactory because the return type of CardFactory is C and that does not satisfy the type expected by code calling .blockCard. So either I need to change the placeholder to C and check if the method exists or not. Or I can put the logic in the constructor only to check if the card supports the feature and then throw an error if not.

If your method's return type is C, then the caller cannot expect the ability to block cards. It would be counterintuitive for the caller to expect something they are not receiving.

When you say "Foo() returns C", you state that it returns any valid implementation of C. Since the class definition of C does not contain blocking logic, then you cannot guarantee that every valid implementation of C has blocking logic, and therefore you cannot acknowledge its existence.

In reality, it's possibly to cast C to C1 and then call the blocking logic, but your cast is a guess. And it's not what you're supposed to do. It's not logically consistent, and it's a dirty patch to a glaring problem of polymorphism abuse.

In short, a common interface (whether it's an interface, abstract base class or concrete base class doesn't matter) should be handled as a black box with buttons. When you (as the consumer) are dealing with a common interface, you do not know what's happening under the hood, but you know how to operate the black box (because it has all the necessary buttons).


A real world example

Think of it like a vending machine. You put a coin in the slot, you press a button, the drink comes out. As the customer (consumer), you only care about three things:

  • I can put a coin in.
  • I can press a button.
  • When I put a coin in and press the button, I get a drink.

This is the interface. It defines what exchange takes place between you and the machine. This does not define how the machine works. There could be many different types of vending machines:

  • An electrical mechanical device like we have in real life.
  • There is a person in there who will fetch the right drink and put it through the slot.
  • There are hamsters in there who are fed by coins and give drinks.
  • Inside the machine there is a portal to another dimension, where coins get transmuted to soda cans and then sent back through the portal.

It simply doesn't matter. As long as you get the drink you wanted, the transaction between you and the vending machine is satisfied.


Hiding the innards of the black box

When you want variations on basic behavior, i.e. C1 and C2 behave differently but both can be handled like a C, what you need to do is hide their differences. Every derivation of C needs to appear to behave the exact same way. Their actual behavior under the hood will be different (because otherwise you wouldn't need different classes), but a consumer can interact with all of them the exact same way.

For example:

public abstract class C
{
    public abstract int CalculateDamageOutput(int damage);
}

We've defined a C, and we've stated that every valid implementation of C is able to calculate damage output. We have not defined how damage output is calculated, we have only defined that it can be calculated.

And now we start implementing it. Please note that I am skipping the additional interfaces as they are not relevant to the answer. The subclasses can also implement additional interfaces, and the principle remains the same.

public class WeakC : C // Takes double damage
{
    public override int CalculateDamageOutput(int damage)
    {            
        return damage * 2;
    }
}

public class BlockingC : C // Can mitigate a certain amount of damage.
{
    private bool BlockedDamageLastTime = false;
    private int ArmorValue = 15;

    public override int CalculateDamageOutput(int damage)
    {
        if(this.BlockedDamageLastTime)
        {
            // You can't block twice in a row
            this.BlockedDamageLastTime = false;
            return damage;
        }

        var output = Math.Max(0, damage - this.ArmorValue);

        // Armor loses efficiency every time you block
        if(this.ArmorValue > 0)
            this.ArmorValue--;

        // Prevents blocking the next time.
        this.BlockedDamageLastTime = true;

        return output;
    }
}

public class InvincibleC : C // Does not take damage
{
    public override int CalculateDamageOutput(int damage)
    {            
        return 0;
    }
}

These are just some examples of very different behaviors. Some are more complex than others. But the important thing to consider here is what does this mean for a consumer?

var originalAttackDamage = 25;

var currentC = CFactory.Get();

var actualAttackValue = currentC.CalculateDamageOutput(originalAttackDamage);

Try to figure out which subclass of C is being used here. Can you tell me which one we're using? No, you can't, because it doesn't matter. This code works for any valid implementation of C.

Even if you end up developing new subclasses of C and change the CFactory.Get() return value to return an object of type MyNewlyDevelopedC, this code does not need to be updated or rechecked, as the logic remains the same.


Direct answers

  1. So, I have created a factory for it. It takes the data and returns proper concrete object depending on the data set it is getting. Is it the right approach or is it ok to put the conditionals in client code for proper object creation?

It is a correct approach, but you need to take care of the return type that your factory method has.

If your factory method returns a C, then the consuming code cannot expect to receive an object that has capabilities that are not defined in C (or any of the classes it derives from).

If your consumer is expecting to receive an object which is capable of blocking, then your method should have the IBlockingC return type, not the C return type.

Now say some malicious user tries blocking a card which does not support the block card feature. I think I can not leverage my CardFactory because the return type of CardFactory is C.

Regardless of the user being malicious, it makes no sense for a user to ask for a particular object (a blocking C) from a method whose signature does not stipulate that it returns blocking C objects.

Or I can put the logic in the constructor only to check if the card supports the feature and then throw an error if not.

By "the card", I assume you mean the C class. Your C class should NEVER EVER be aware of its own subclasses. The purpose of the C class is to provide something that you expect all subclasses to share. Its purpose is not to act as a collection of references to all its subclasses.

The subclass references the base class. The base class never references the subclass.

If I remove the factory then every time a new card is created I need to do an if-else check. What's the best way to achieve it?

You're putting the cart before the horse. If you want to work under the "block if you can, don't if you can't" principle, then you shouldn't be using subclasses here.

What you have done is created subclasses which innately (and definitively) define whether or not this object is capable of blocking. That means that when you create the correct object, you are already deciding whether the ability to block is needed or not.

Later, when you pass that object around, you will therefore already be aware whether you created a blocking object or not. If you are not aware of it anymore, there are two possibilities:

  • If you (the consumer) had no say in it and don't know what happened under the hood, that means that you (the consumer) are not supposed to know if this object can block or not.
  • If you (the consumer) had a say in it, but you simply don't remember anymore, that means you're mismanaging your data and have simply lost some information that was important to you. This is not solved by reobserving the object. This is solved by making sure you don't lose that contextual information in the first place.

In either case, the solution is not to look at the object and ask yourself "can this block? Because I don't know!". Either you're not supposed to know, or you already know.

  • I am still stuck how to know if the card supports blocking. As the data is coming from untrusted client, shall I do the validation before entering the service layer. So, I get the request and I validate by some means at controller layer to decide if the card can be blocked and if it satisfies it then only proceed ? – Krrish Raj Jun 5 at 18:16
  • @KrrishRaj: If you define blocking behavior through types (classes/interfaces), then you should be expecting the correct type to work with it. It's not that you can't upcast, but rather that for you to instantiate an object of a certain class, you already know whether it blocks or not, so you shouldn't be guessing at it anymore. – Flater Jun 5 at 22:12
  • @KrrishRaj: If instead the behavior cannot be expected from your end, then you shouldn't handle it through type, and rather have a CanBlock property on the card class itself. However, I get the feeling that this isn't what you're currently doing, and I'm not sure if it's what you actually want. What do you mean when you say it's an untrusted client? Do you figure out whether it can or cannot block based on the data values you receive? Is it then not a matter of simply checking the object's data values, rather than trying to implement interfaces on the class? – Flater Jun 5 at 22:14
  • As the request comes from the API just with the card number, I am never sure if the card can be blocked or not. Also, the client can tamper the data and send a request with a card number that can not be blocked. If I add it as a property on the card itself, tomorrow when I add more card types with various capabilities to assembly, I need to add more flags to my base card. That violates the OC principle. – Krrish Raj Jun 6 at 6:45
  • @KrrishRaj: If they only send the card number, that means you already store the data (retrievable by its identifier) on whether it can block or not. This means there's no point to "forgetting" that data and then having to figure out if it can block based on the type. When you instantiated the object (and thus would've chosen its type in your proposed solution), you already knew if the card could block or not. You are way overengineering your approach. – Flater Jun 6 at 10:47

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.