Can I refactor my code anymore? [closed]

Question

I have written one method; however I feel I can refactor it even more but not sure what

Can someone help me here?

    public static PaymentOptions GetPaymentOptions_NonGBCulture(TestConfigurationCDO testConfiguration, int siteId)
    {
        var paymentOptions = default(PaymentOptions);

        var paymentOptionList = SitePaymentRepository.GetSitePaymentInfoBySiteId(
        testConfiguration,
        siteId);

        var paymentAuto = paymentOptionList.First(x => x.PaymentType == PaymentMethod.PayPalDirect || x.PaymentType == PaymentMethod.Braintree || x.PaymentType == PaymentMethod.BankTransfer || x.PaymentType == PaymentMethod.AdyenDropIn);

        paymentOptions = new ClientCheckoutOptions()
        {
            paymentMethod = paymentAuto.PaymentType
        };

        return paymentOptions;
    }

I feel if I create list for possible Payment Type as below

     PaymentMethod[] checkoutGroup = { PaymentMethod.PayPalDirect, PaymentMethod.Braintree, PaymentMethod.BankTransfer, PaymentMethod.AdyenDropIn };

But not sure how do I add that check in query

Answer 1

I can see at least two places where you can make your code more concise.

PaymentType

As you have suspected replacing multiple x.PaymentType == PaymentMethod.XYZ || statements with a collection lookup might help. With this approach you separate data from operation.

Before

var paymentAuto = paymentOptionList
   .First(x => x.PaymentType == PaymentMethod.PayPalDirect 
       || x.PaymentType == PaymentMethod.Braintree 
       || x.PaymentType == PaymentMethod.BankTransfer 
       || x.PaymentType == PaymentMethod.AdyenDropIn);

After

ImmutableArray<PaymentMethod> checkoutGroup = new []
{ 
   PaymentMethod.PayPalDirect, 
   PaymentMethod.Braintree,
   PaymentMethod.BankTransfer,
   PaymentMethod.AdyenDropIn 
}.ToImmutableArray();

var paymentAuto = paymentOptionList
   .First(x => checkoutGroup.Contains(x.PaymentType));

Note: I've used here Immutable collection to prevent accidental (or intentional) add and/or remove.

paymentOptions

You can get rid of this variable because you have set it once (the initialization code is unnecessary) and immediately return.

Before

public static PaymentOptions GetPaymentOptions_NonGBCulture(TestConfigurationCDO testConfiguration, int siteId)
{
    var paymentOptions = default(PaymentOptions);

    //...

    paymentOptions = new ClientCheckoutOptions()
    {
        paymentMethod = paymentAuto.PaymentType
    };

    return paymentOptions;
}

After

public static PaymentOptions GetPaymentOptions_NonGBCulture(TestConfigurationCDO testConfiguration, int siteId)
{
    //...

    return new ClientCheckoutOptions { paymentMethod = paymentAuto.PaymentType };
}

Note: In case of object initialization you don't need to explicitly call the parameterless ctor ( new ClientCheckoutOptions() { ... } >> new ClientCheckoutOptions { ... })

---

UPDATE: The most concise version I can think of:

private static readonly ImmutableArray<PaymentMethod> checkoutGroup = new []
{
   PaymentMethod.PayPalDirect,
   PaymentMethod.Braintree,
   PaymentMethod.BankTransfer,
   PaymentMethod.AdyenDropIn
}.ToImmutableArray();

public static PaymentOptions GetPaymentOptions_NonGBCulture(TestConfigurationCDO testConfiguration, int siteId)
    => new ClientCheckoutOptions
    {
        paymentMethod = SitePaymentRepository
          .GetSitePaymentInfoBySiteId(testConfiguration, siteId)
          .Select(po => po.PaymentType)
          .First(checkoutGroup.Contains)
    };