0
\$\begingroup\$

I need to be able to access a number of different payment methods in a common list. They can be different methods - credit card, cheque, direct debit etc. The only common attributes are the payment method id and PaymentMethodEnum. I would then be casting the payment method to it's appropriate type to get the extended properties. Is this code arrangement the best way to achieve that?

Update: I've added the enums. The usage for this is an MVC Website, and these payment details are effectively read only. The website shows different options based on the current payment method, and the other available methods that belong to a customer.

public class PaymentDetailBase
{
    public PaymentDetailBase(string id, PaymentMethodEnum paymentMethod)
    {
        this.Id = id;
        this.PaymentMethod = paymentMethod;
    }

    public string Id { get; }

    public PaymentMethodEnum PaymentMethod { get; }
}

public class CardDetail : PaymentDetailBase
{
    public CardDetail(string id, PaymentMethodEnum paymentMethod, string last4, DateTime expiry, PaymentCardTypeEnum cardType) : base(id, paymentMethod)
    {
        this.Last4 = last4;
        this.Expiry = expiry;
        this.CardType = cardType;
    }

    public string Last4 { get; }

    public DateTime Expiry { get; }

    public PaymentCardTypeEnum CardType { get; }
}

public class DirectDebitDetail : PaymentDetailBase
{
    public DirectDebitDetail(string id, PaymentMethodEnum paymentMethod, string accountName, string routingNumber, string bankAccount, DirectDebitAccountTypeEnum accountType) : base(id, paymentMethod)
    {
        this.AccountName = accountName;
        this.RoutingNumber = routingNumber;
        this.BankAccount = bankAccount;
        this.AccountType = accountType;
    }

    public string AccountName { get; }

    public string RoutingNumber { get; }

    public string BankAccount { get; }

    public DirectDebitAccountTypeEnum AccountType { get; }
}


[Flags]
public enum PaymentMethodEnum
{
    [Description("NONE")]
    None        = 0,

    [Description("Credit Card")]
    CreditCard  = 1 << 0,

    [Description("Direct Debit")]
    DirectDebit = 1 << 1,

    [Description("Check")]
    Cheque      = 1 << 2,

    [Description("Invoice")]
    Invoice     = 1 << 3,

    [Description("Money Order")]
    MoneyOrder  = 1 << 4,

    [Description("Paper Invoice")]
    PaperInvoice = 1 << 5
}

public enum PaymentCardTypeEnum
{
    [Description("Unknown")]
    Unknown,

    [Description("AMEX")]
    Amex,

    [Description("MASTERCARD")]
    MasterCard,

    [Description("DISCOVER")]
    Discover,

    [Description("VISA")]
    Visa
}

public enum BillingFrequencyEnum
{
    [Description("Unknown")]
    Unknown,

    [Description("Annual")]
    Annual,

    [Description("Semi-annual")]
    SemiAnnual,

    [Description("Quarterly")]
    Quarterly,

    [Description("Bi-monthy")]
    BiMonthly,

    [Description("Ten months")]
    TenMonths,

    [Description("Monthly")]
    Monthly
}

public enum DirectDebitAccountTypeEnum
{
    [Description("Unknown")]
    Unknown,

    [Description("Checking Account")]
    Checking,

    [Description("Savings Account")]
    Savings
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Please include your different enums \$\endgroup\$ – IEatBagels Apr 23 '19 at 17:23
  • 1
    \$\begingroup\$ Have you tested it and does it work? \$\endgroup\$ – pacmaninbw Apr 23 '19 at 19:38
  • 1
    \$\begingroup\$ I've added the enums \$\endgroup\$ – JimboFlex Apr 24 '19 at 9:27
  • \$\begingroup\$ @pacmaninbw no, it's a work in progress... \$\endgroup\$ – JimboFlex Apr 24 '19 at 9:35
  • 2
    \$\begingroup\$ In such a case it's off-topic here as we require working code. \$\endgroup\$ – t3chb0t Apr 24 '19 at 11:52
3
\$\begingroup\$

I'm not in love with having an enum to know what base type to cast to. If you want to stick to it then I'll give you a couple of pointers then later I'll give you a couple more options to think about

Make the base abstract and make the enum abstract

public abstract class PaymentDetailBase
{
    protected PaymentDetailBase(string id)
    {
        this.Id = id;
    }

    public string Id { get; }

    public abstract PaymentMethodEnum PaymentMethod { get; }
}

if you are going to cast based on the Enum then you don't want it being sent in the constructor. What if I sent in the wrong enum for the type

Now classes the inherit from the base will be forced to fill in the type

public class CardDetail : PaymentDetailBase
{
    public CardDetail(string id, string last4, DateTime expiry, PaymentCardTypeEnum cardType) : base(id)
    {
        this.Last4 = last4;
        this.Expiry = expiry;
        this.CardType = cardType;
    }

    public string Last4 { get; }

    public DateTime Expiry { get; }

    public PaymentCardTypeEnum CardType { get; }

    public override PaymentMethodEnum PaymentMethod => PaymentMethodEnum.CreditCard;
}

I've worked with applications like this and I don't really care for the Enum. It violates the open/close principle. If I want another payment type I need to change the enum and some switch code that does hardcoded cast (yuck). And what are you saving? A switch statement instead of getting the base class and doing the AS statement and checking for null?

Really when you run into this situation you need to think about your design. What in the outside needs to know about those extended properties. If everything then why cast to the base object in the first place just pass in the right class or you can try to make use of generics with constraints.

Another option is if not a lot needs to know about the specific class then have the class do the operation you want done. For Example you need to just copy over those properties to an entity. Then aleave the base class abstract and make an abstract method like

public void UpdateEntity(PaymentEntity entity);

now each class need to implement the update entity and it knows about it's own properties.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you. The rationale for using the base type and not an abstract was that there are several types of payment methods but only two are supported for now. By 'supported' I mean contain extended information. I though this would allow the unsupported types to just be instantiated as the PaymentDetailBase type directly. Perhaps this is a flawed idea. I like the override enum, which seems much better. I'm not entirely clear on what you mean about enum and switch vs cast, if the enum was dropped, wouldn't I then need to AS the object to all possible types just to determine the payment type? \$\endgroup\$ – JimboFlex Apr 24 '19 at 9:48
  • \$\begingroup\$ Guess I should have been more clear. I don't care for the enum or using the AS. Without knowing what your classes need to do it's hard to say what the correct design is. But typically I do go with the option at the end. You can always inject dependency in the class to do the common stuff between all of the payment types and then there is no enum or casting and each class takes care of themselves allowing it to be extended without changing any code. \$\endgroup\$ – CharlesNRice Apr 24 '19 at 13:39

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