Deciding the best approach to refactor some code

[sock1992]

Hi,

I'm working on a test and I'm trying to find the best way to refactor the code below. Each case is an Enum and returns a boolean

switch (request.PaymentScheme)
            {
                case PaymentScheme.Bacs:
                    
                    if (accountRetrieved == null ||
                        !accountRetrieved.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Bacs)) // using an Enum here
                    {
                        paymentResult.Success = false;
                    }
                    
                    break;

                case PaymentScheme.FasterPayments:
                    
                    if (accountRetrieved == null ||
                        !accountRetrieved.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.FasterPayments) ||
                        accountRetrieved.Balance < request.Amount)
                    {
                        paymentResult.Success = false;
                    }
                  
                    break;

                case PaymentScheme.Chaps:
                    if (accountRetrieved == null ||
                        !accountRetrieved.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Chaps) ||
                        accountRetrieved.Status != AccountStatus.Live)
                    {
                        paymentResult.Success = false;
                    }
                    break;
            }

I was thinking of putting the following codeblock in a separate method? and then calling that in each of the cases

if (accountRetrieved == null ||
                        !accountRetrieved.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.FasterPayments) ||

Any help would be appreciated!

 

[sock1992]

Think ill go with the mapping between the two:

private IDictionary<PaymentScheme, AllowedPaymentSchemes> MappingEnums = new Dictionary<PaymentScheme, AllowedPaymentSchemes>
        {
            { PaymentScheme.FasterPayments, AllowedPaymentSchemes.FasterPayments },
            { PaymentScheme.Bacs, AllowedPaymentSchemes.Bacs  },
            { PaymentScheme.Chaps, AllowedPaymentSchemes.Bacs }
        };

The only thing im unsure of is how I would include this in my current code.

I managed to sort it

 

[Skydiver]

Sorry, the C/C++ programmer can't resist:

Debug.Assert(AllowedPaymentSchemes.FasterPayments == (AllowePaymentSchemes)(1 << (int)PaymentScheme.FasterPayments));
Debug.Assert(AllowedPaymentSchemes.Bacs == (AllowePaymentSchemes)(1 << (int)PaymentScheme.Bacs));
Debug.Assert(AllowedPaymentSchemes.Chaps == (AllowePaymentSchemes)(1 << (int)PaymentScheme.Chaps));

var allowedPaymentSchemes = (AllowedPaymentSchemes) (1 << (int) request.PaymentScheme);
var success = account?.AllowedPaymentSchemes.HasFlag(allowedPaymentSchemes) ?? false;
switch (request.PaymentScheme)
{
    case PaymentScheme.FasterPayments:
        success &= account.Balance > request.Amount;
        break;
    case PaymentScheme.Chaps:
        success &= account.Status == AccountStatus.Live;
        break;
}
result.Success = success;

 

[Skydiver]

Or:

Debug.Assert(AllowedPaymentSchemes.FasterPayments == MapToAllowed(PaymentScheme.FasterPayments));
Debug.Assert(AllowedPaymentSchemes.Bacs == MapToAllowed(PaymentScheme.Bacs));
Debug.Assert(AllowedPaymentSchemes.Chaps == MapToAllowed(PaymentScheme.Chaps));

var allowedPaymentSchemes = MapToAllowed(request.PaymentScheme);
var success = account?.AllowedPaymentSchemes.HasFlag(allowedPaymentSchemes) ?? false;
switch (request.PaymentScheme)
{
    case PaymentScheme.FasterPayments:
        success &= account.Balance > request.Amount;
        break;
    case PaymentScheme.Chaps:
        success &= account.Status == AccountStatus.Live;
        break;
}
result.Success = success;

:

static AllowedPaymentSchemes MapToAllowed(PaymentScheme paymentScheme)
    => (AllowePaymentSchemes)(1 << (int)paymentScheme);

 

[cjard]

Why even have two enums with the same names and different values?

I didn't notice, and neither did SD; that's clear evidence of creating a maintenance headache

 

[Skydiver]

Might have to be the first refactoring step: Normalize the multiple enums used.

 

[cjard]

Why are the balance and status only checked on FP/CHAPS respectively? Can an FP make a payment on a closed account? Can a CHAPS succeed even if there isn't any balance to pay for it?