13

As i am learning through design pattern concept and also wanted to implement the payment modules in my project using the proper design pattern. So for that I have created some sample code.

Currently I have two concrete implementation for the payment PayPal and Credit Card. But the concrete implementation will be added further on the project.

Payment Service

public interface IPaymentService
{
    void MakePayment<T>(T type) where T : class;
}

Credit Card and Pay Pal Service

public class CreditCardPayment : IPaymentService
{
    public void MakePayment<T>(T type) where T : class
    {
        var creditCardModel = (CreditCardModel)(object)type;
        //Implementation CreditCardPayment
    }
}

class PayPalPayment : IPaymentService
{
    public void MakePayment<T>(T type) where T : class
    {
        var payPalModel = (PayPalModel)(object)type;
        //Further Implementation will goes here
    }
}

Client Code Implementation

var obj = GetPaymentOption(payType);
obj.MakePayment<PayPalModel>(payPalModel);

Get Payment Option

private static IPaymentService GetPaymentOption(PaymentType paymentType)
{
        IPaymentService paymentService = null;

        switch (paymentType)
        {
            case PaymentType.PayPalPayment:
                paymentService = new PayPalPayment();
                break;
            case PaymentType.CreditCardPayment:
                paymentService = new CreditCardPayment();
                break;
            default:
                break;
        }
        return paymentService;
}

I thought of implementing this modules using strategy design pattern, and I got deviated from Strategy and ended up doing this way.

Is this a proper way for creating the payment modules. Is there a more better approach of solving this scenario. Is this a design pattern?

Edited:

Client Code:

static void Main(string[] args)
{
    PaymentStrategy paymentStrategy = null;


    paymentStrategy = new PaymentStrategy(GetPaymentOption((PaymentType)1));
    paymentStrategy.Pay<PayPalModel>(new PayPalModel() { UserName = "", Password = "" });

    paymentStrategy = new PaymentStrategy(GetPaymentOption((PaymentType)2));
    paymentStrategy.Pay<CreditCardModel>(
       new CreditCardModel()
    {
        CardHolderName = "Aakash"
    });

    Console.ReadLine();

}

Strategy:

public class PaymentStrategy
{
    private readonly IPaymentService paymentService;
    public PaymentStrategy(IPaymentService paymentService)
    {
        this.paymentService = paymentService;
    }

    public void Pay<T>(T type) where T : class
    {
        paymentService.MakePayment(type);
    }
}

Does this update inlines with the Strategy Pattern?

Rasik
  • 1,961
  • 3
  • 35
  • 72
  • 1
    This looks more like a code review and is also opinion based. – Nkosi Feb 11 '18 at 13:57
  • @Nkosi You mean the question should not be here? – Rasik Feb 11 '18 at 14:02
  • 1
    If you have a concrete implementation of IPaymentService in the form of CreditCardPayment, would it not make sense to have MakePayment coupled to a more specific type? You could do IPaymentService, where T is the type used for MakePayment. – Parrish Husband Feb 11 '18 at 16:17
  • Following that, it might make sense to have an IPayModel interface which the CreditCard and PayPal models inherit, and have the MakePayment method on IPaymentService take an IPayModel for the parameter – Parrish Husband Feb 11 '18 at 16:22
  • CreditCard and PayPal models have different properties, and they do not share any common properties. Since, they are not sharing any common things does it makes sense to inherit the class from IPayModel interface? – Rasik Feb 11 '18 at 16:35
  • Even without shared properties (yet), this coupling makes more sense than simply restricting T to reference types. Also down the line, you may end up with a need for a property or method available on all pay models. – Parrish Husband Feb 11 '18 at 16:41
  • So, you suggest to put the common properties on single interface, inherit that and make own implementation of different properties? – Rasik Feb 11 '18 at 16:52
  • My suggestion is more towards IPaymentService.MakePayment being coupled a bit tighter than just 'where T : class'. This could mean an IPayModel interface, a type specifier for MakePayment defined through IPaymentService, or some other implementation (maybe the visitor pattern). Think about the long-term usage and scaling of the project to make a best guess for what will work. – Parrish Husband Feb 11 '18 at 16:56
  • `interface IPaymentServices where TPayType : IPayModel` I have modified the code as this with the method `void MakePayment(T type);` But but how do i use in the client code? – Rasik Feb 11 '18 at 17:11
  • @aakash you could accept an answer which you could consider very helpful. I definitely learn more from your question as well as others answer – Jayendran Nov 14 '18 at 14:16
  • @Jayendran i am welcoming more answer for the question. – Rasik Nov 15 '18 at 14:48

3 Answers3

28

One major drawback of using an abstract factory for this is the fact that it contains a switch case statement. That inherently means if you want to add a payment service, you have to update the code in the factory class. This is a violation of the Open-Closed Principal which states that entities should be open for extension but closed for modification.

Note that using an Enum to switch between payment providers is also problematic for the same reason. This means that the list of services would have to change every time a payment service is added or removed. Even worse, a payment service can be removed from the strategy, but still be an Enum symbol for it even though it isn't valid.

On the other hand, using a strategy pattern doesn't require a switch case statement. As a result, there are no changes to existing classes when you add or remove a payment service. This, and the fact that the number of payment options will likely be capped at a small double-digit number makes the strategy pattern a better fit for this scenario.

Interfaces

// Empty interface just to ensure that we get a compile
// error if we pass a model that does not belong to our
// payment system.
public interface IPaymentModel { }

public interface IPaymentService
{
    void MakePayment<T>(T model) where T : IPaymentModel;
    bool AppliesTo(Type provider);
}

public interface IPaymentStrategy
{
    void MakePayment<T>(T model) where T : IPaymentModel;
}

Models

public class CreditCardModel : IPaymentModel
{
    public string CardHolderName { get; set; }
    public string CardNumber { get; set; }
    public int ExpirtationMonth { get; set; }
    public int ExpirationYear { get; set; }
}

public class PayPalModel : IPaymentModel
{
    public string UserName { get; set; }
    public string Password { get; set; }
}

Payment Service Abstraction

Here is an abstract class that is used to hide the ugly details of casting to the concrete model type from the IPaymentService implementations.

public abstract class PaymentService<TModel> : IPaymentService
    where TModel : IPaymentModel
{
    public virtual bool AppliesTo(Type provider)
    {
        return typeof(TModel).Equals(provider);
    }

    public void MakePayment<T>(T model) where T : IPaymentModel
    {
        MakePayment((TModel)(object)model);
    }

    protected abstract void MakePayment(TModel model);
}

Payment Service Implementations

public class CreditCardPayment : PaymentService<CreditCardModel>
{
    protected override void MakePayment(CreditCardModel model)
    {
        //Implementation CreditCardPayment
    }
}

public class PayPalPayment : PaymentService<PayPalModel>
{
    protected override void MakePayment(PayPalModel model)
    {
        //Implementation PayPalPayment
    }
}

Payment Strategy

Here is the class that ties it all together. Its main purpose is to provide the selection functionality of the payment service based on the type of model passed. But unlike other examples here, it loosely couples the IPaymentService implementations so they are not directly referenced here. This means without changing the design, payment providers can be added or removed.

public class PaymentStrategy : IPaymentStrategy
{
    private readonly IEnumerable<IPaymentService> paymentServices;

    public PaymentStrategy(IEnumerable<IPaymentService> paymentServices)
    {  
        this.paymentServices = paymentServices ?? throw new ArgumentNullException(nameof(paymentServices));
    }

    public void MakePayment<T>(T model) where T : IPaymentModel
    {
        GetPaymentService(model).MakePayment(model);
    }

    private IPaymentService GetPaymentService<T>(T model) where T : IPaymentModel
    {
        var result = paymentServices.FirstOrDefault(p => p.AppliesTo(model.GetType()));
        if (result == null)
        {
            throw new InvalidOperationException(
                $"Payment service for {model.GetType().ToString()} not registered.");
        }
        return result;
    }
}

Usage

// I am showing this in code, but you would normally 
// do this with your DI container in your composition 
// root, and the instance would be created by injecting 
// it somewhere.
var paymentStrategy = new PaymentStrategy(
    new IPaymentService[]
    {
        new CreditCardPayment(), // <-- inject any dependencies here
        new PayPalPayment()      // <-- inject any dependencies here
    });


// Then once it is injected, you simply do this...
var cc = new CreditCardModel() { CardHolderName = "Bob" /* Set other properties... */ };
paymentStrategy.MakePayment(cc);

// Or this...
var pp = new PayPalModel() { UserName = "Bob" /* Set other properties... */ };
paymentStrategy.MakePayment(pp);

Additional References:

NightOwl888
  • 55,572
  • 24
  • 139
  • 212
  • In Client code How would i know which instances of Payment model to instantiate, if i don't have the switch case statement? Because in my case the user select only one Payment mode and i need to identity which payment mode has been selected. – Rasik Feb 15 '18 at 15:20
  • 1
    That is an implementation detail of the UI. There is no rule that says you can't use an `Enum` to work out what model to instantiate. But then, you probably would want the selector on the UI to be generated based off of the payment services in the strategy (for which you can add a property for each provider), not some hard-coded `Enum`. The list of available providers could then update on the UI based on the actual providers that are registered in the strategy. – NightOwl888 Feb 15 '18 at 15:45
  • Excellent post. Can you explain the (TModel)(object)model casting in the MakePayment method of the PaymentService class? Also could it make sense here to create IPaymentService and IPaymentService, where the IPaymentService.MakePayment gets implemented explicitly on IPaymentService? – Parrish Husband Feb 15 '18 at 17:24
  • 1
    The `(TModel)(object)model` is required because there is no direct cast from `T` to `TModel`, so we do an intermediary cast to `object` first. – NightOwl888 Feb 15 '18 at 17:29
  • Cool I hadn't seen that before, makes sense. – Parrish Husband Feb 15 '18 at 17:37
  • As for creating `IPaymentService`, there would still need to be a way to do the cast because the strategy class only knows about `IPaymentService`. I suppose there could be some Reflection code to do that in the strategy similar to the [QueryProcessor class here](https://www.cuttingedge.it/blogs/steven/pivot/entry.php?id=92), but it is both complicated and slow. A generic abstract class is a better fit than an interface in this case because it allows for an implementation that is shared between all `IPaymentService` implementations to keep the code DRY. – NightOwl888 Feb 15 '18 at 17:37
  • Yeah I'm not negating the need for the abstract class, I lean heavily on that approach quite often. Specifically I was wondering if the IPaymentService also needed a generic flavor. So you'd end up with MakePayment(IPaymentModel) and MakePayment(T) essentially. – Parrish Husband Feb 15 '18 at 17:40
  • That might make sense if you intend to use `IPaymentService` by itself without the strategy class. But since it is hidden behind the strategy abstraction it is YAGNI otherwise. – NightOwl888 Feb 15 '18 at 18:00
  • @NightOwl888 Sorry to add some question here on the comment. But to make some clarification, what approach should i consider if i want to add some validation before making any payment, like querying to database or making some API call to validate the user's information. – Rasik Oct 05 '18 at 06:00
  • @AkshayKriti - If the validation is for *every* payment, then it should be a service that could be either injected into the strategy class or into the class that depends on the strategy interface (depending on how sure you are it will need to be done 100% of the time). For individual card validations, you could inject validation services into each card type or alternatively use a decorator pattern or a proxy pattern on `IPaymentService`. This explains the latter 2 patterns: https://msdn.microsoft.com/en-us/magazine/dn574804.aspx. – NightOwl888 Oct 06 '18 at 16:12
  • @NightOwl888 thanks for your great answer, I'm new to this design pattern and DI in dotnet. In the usage block, you mentioned like `do this with your DI container in your composition root, and the instance would be created by injecting it somewhere.` could you explain (or) give some reference to understand this pls? – Jayendran Nov 14 '18 at 14:47
  • @NightOwl888 I got understand from your another answer which you referred here, **Dependency Injection Unity - Conditional Resolving** thanks – Jayendran Nov 15 '18 at 06:32
  • Naming wise, isn't the strategy part, a Facade, and the service part, the actual Strategy? – Hassan Faghihi Nov 08 '21 at 16:41
  • Also what happens if our banking gateway system, has multiple functionality which they need to be called separately. – Hassan Faghihi Nov 09 '21 at 11:46
  • 1
    @deadManN - If you mean you have multiple responsibilities that you wish to separate, then I would suggest putting those responsibilities into individual classes that are injected into the service (where I denoted "inject any dependencies here"). If you mean you have a *workflow* that you need to kick off that has multiple steps, then you need more patterns. You might be able to use the strategy to kick off the first step, though. – NightOwl888 Nov 21 '21 at 04:01
  • yeah, the workflow I mean, I have three step so far, and It may grow to be four depend on the next API I later receive or five. But most banking system in my country require 2 step, which 1 step can break down into 2, that's why I have 3 step by default. I have A, B, C steps, and a part of C, is requires me to implement different controller actions. -1 A. Sending data to bank. -2 B. Transfer user to bank payment page. -3 C. Return result of bank from the URL that bank calls in our system. -4 C. Verify the result. – Hassan Faghihi Nov 21 '21 at 09:16
1

This is one approach you could take. There's not a lot to go on from your source, and I'd really reconsider having MakePayment a void instead of something like an IPayResult.

public interface IPayModel { }  // Worth investigating into common shared methods and properties for this 
public interface IPaymentService
{
    void MakePayment(IPayModel  payModel);
}
public interface IPaymentService<T> : IPaymentService where T : IPayModel
{
    void MakePayment(T payModel);  // Void here?  Is the status of the payment saved on the concrete pay model?  Why not an IPayResult?
}

public class CreditCardModel : IPayModel
{
    public string CardHolderName { get; set; }
}
public class PayPalModel : IPayModel
{
    public string UserName { get; set; }
    public string Password { get; set; }
}

public class CreditCardPayment : IPaymentService<CreditCardModel>
{
    public void MakePayment(CreditCardModel payModel)
    {
        //Implmentation CreditCardPayment
    }
    void IPaymentService.MakePayment(IPayModel payModel)
    {
        MakePayment(payModel as CreditCardModel);
    }
}
public class PayPalPayment : IPaymentService<PayPalModel>
{
    public void MakePayment(PayPalModel payModel)
    {
        //Implmentation PayPalPayment
    }
    void IPaymentService.MakePayment(IPayModel payModel)
    {
        MakePayment(payModel as PayPalModel);
    }
}

public enum PaymentType
{
    PayPalPayment = 1,
    CreditCardPayment = 2
}

So following your implementation approach, it could look something like:

static class Program
{
    static void Main(object[] args)
    {
        IPaymentService paymentStrategy = null;
        paymentStrategy = GetPaymentOption((PaymentType)1);
        paymentStrategy.MakePayment(new PayPalModel { UserName = "", Password = "" });

        paymentStrategy = GetPaymentOption((PaymentType)2);
        paymentStrategy.MakePayment(new CreditCardModel { CardHolderName = "Aakash" });

        Console.ReadLine();
    }

    private static IPaymentService GetPaymentOption(PaymentType paymentType) 
    {
        switch (paymentType)
        {
            case PaymentType.PayPalPayment:
                return new PayPalPayment();
            case PaymentType.CreditCardPayment:
                return new CreditCardPayment();
            default:
                throw new NotSupportedException($"Payment Type '{paymentType.ToString()}' Not Supported");
        }
    }
}

I also think for a strategy/factory pattern approach, manually creating an IPayModel type doesn't make much sense. Therefore you could expand the IPaymentService as an IPayModel factory:

public interface IPaymentService
{
    IPayModel CreatePayModel();
    void MakePayment(IPayModel payModel);
}
public interface IPaymentService<T> : IPaymentService where T : IPayModel
{
    new T CreatePayModel();
    void MakePayment(T payModel);
}

public class CreditCardPayment : IPaymentService<CreditCardModel>
{
    public CreditCardModel CreatePayModel()
    {
        return new CreditCardModel();
    }
    public void MakePayment(CreditCardModel payModel)
    {
        //Implmentation CreditCardPayment
    }

    IPayModel IPaymentService.CreatePayModel()
    {
        return CreatePayModel();
    }
    void IPaymentService.MakePayment(IPayModel payModel)
    {
        MakePayment(payModel as CreditCardModel);
    }
} 

Usage would then be:

IPaymentService paymentStrategy = null;
paymentStrategy = GetPaymentOption((PaymentType)1);

var payModel = (PayPalModel)paymentStrategy.CreatePayModel();
payModel.UserName = "";
payModel.Password = "";
paymentStrategy.MakePayment(payModel);
Parrish Husband
  • 3,148
  • 18
  • 40
0

Your code is basically using the factory pattern. This is a good way to handle more than one method of payment

http://www.dotnettricks.com/learn/designpatterns/factory-method-design-pattern-dotnet

Ken Tucker
  • 4,126
  • 1
  • 18
  • 24