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!

 

[Skydiver]

What's the default value of paymentResult.Success if none of the if conditions are true?

 

[Skydiver]

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

That's a good first step.

 

[sock1992]

What's the default value of paymentResult.Success if none of the if conditions are true?

    public class MakePaymentResult
    {
        public bool Success { get; set; }
    }
}

This is what its referring to in the solution, the default value isnt specified, but if none of the conditions are correct im guessing it would be true.

below is the original code:

var result = new MakePaymentResult();

            switch (request.PaymentScheme)
            {
                case PaymentScheme.Bacs:
                    if (account == null)
                    {
                        result.Success = false;
                    }
                    else if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Bacs))
                    {
                        result.Success = false;
                    }
                    break;

                case PaymentScheme.FasterPayments:
                    if (account == null)
                    {
                        result.Success = false;
                    }
                    else if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.FasterPayments))
                    {
                        result.Success = false;
                    }
                    else if (account.Balance < request.Amount)
                    {
                        result.Success = false;
                    }
                    break;

                case PaymentScheme.Chaps:
                    if (account == null)
                    {
                        result.Success = false;
                    }
                    else if (!account.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Chaps))
                    {
                        result.Success = false;
                    }
                    else if (account.Status != AccountStatus.Live)
                    {
                        result.Success = false;
                    }
                    break;
            }

 

[Skydiver]

If the default value is false, then aren't all your switch statements above effectively doing nothing?

 

[sock1992]

If the default value is false, then aren't all your switch statements above effectively doing nothing?

sorry I meant true** there is an if statement after which then makes the payment

if(Payment.Success)
{
   /// account updated
}

 

[Skydiver]

I'm thinking of something like

var success = account?.AllowedPaymentSchemes.HasFlag(request.PaymentScheme) :: 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;

 

[sock1992]

I'm thinking of something like

var success = account?.AllowedPaymentSchemes.HasFlag(request.PaymentScheme) :: 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;

Nice! the only thing which I don't get here, is how come you haven't made use of the Enum "AllowedPaymentScheme"

Secondly would using the ?? operator become the same condition here:

var success = account?.AllowedPaymentSchemes.HasFlag(request.PaymentScheme) ?? false;

 

[Skydiver]

It was suppose to be ??. Serves me right for trying to use my phone for writing code.

 

[sock1992]

It was suppose to be ??. Serves me right for trying to use my phone for writing code.

Just one more thing mate, here we are referring to an enum when using the HasFlag method:

!accountRetrieved.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.Chaps)

!accountRetrieved.AllowedPaymentSchemes.HasFlag(AllowedPaymentSchemes.FasterPayments)

The class is here:

  public enum AllowedPaymentSchemes
    {
        FasterPayments = 1 << 0,
        Bacs = 1 << 1,
        Chaps = 1 << 2
    }

In the code snippet above, you haven't made use of that, would it be important to apply this aswell?

 

[Skydiver]

The enum value is stored in request.PaymentScheme. If the value we're not stored there then your case statements in your unrefactored code wouldn't work.

Notice that in your switch statement you are switching on the value of request.PaymentScheme, and then you are passing in that same value to HasFlag().


So I am doing HasFlags(request.PaymentScheme).

 

[Skydiver]

Oh! I see now. You have two different enums. PaymentSchemes and AllowedPaymentSchemes.

Do the values match up between the two enums? If so then you can just cast between them. If not then one option would be some kind of mapping function between the two.

 

[Skydiver]

Also as an aside, in general in object oriented programming, a switch statement that is switching on a type or kind of object is a code smell that suggests that the Strategy pattern and polymorphism maybe appropriate to use.

 

[sock1992]

Oh! I see now. You have two different enums. PaymentSchemes and AllowedPaymentSchemes.

Do the values match up between the two enums? If so then you can just cast between them. If not then one option would be some kind of mapping function between the two.

The values are different:

  public enum PaymentScheme
    {
        FasterPayments,
        Bacs,
        Chaps
    }

  public enum AllowedPaymentSchemes
    {
        FasterPayments = 1 << 0,
        Bacs = 1 << 1,
        Chaps = 1 << 2
    }

 

[sock1992]

Oh! I see now. You have two different enums. PaymentSchemes and AllowedPaymentSchemes.

Do the values match up between the two enums? If so then you can just cast between them. If not then one option would be some kind of mapping function between the two.

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.