-2
\$\begingroup\$

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

\$\endgroup\$
8
  • \$\begingroup\$ .First(x => checkoutGroup.Contains(x.PaymentType)); \$\endgroup\$ Commented Apr 14, 2021 at 11:57
  • \$\begingroup\$ Thank you!! @PeterCsala - Do you think I have any more room for improvement in this method? \$\endgroup\$
    – Chandler
    Commented Apr 14, 2021 at 12:07
  • \$\begingroup\$ You can also get rid of the paymentOptions variable. So, your return could be streamlined like this: return new ClientCheckoutOptions { paymentMethod = paymentAuto.PaymentType }; \$\endgroup\$ Commented Apr 14, 2021 at 12:59
  • 1
    \$\begingroup\$ The primary point of the Code Review Community is to review code that is working as expected and provide suggestions on how that code can be improved. This seems to be a How to question and that would be better on the stackoverflow site rather than on the Code Review site. How to questions generally indicate the code in the question is not working as expected. \$\endgroup\$
    – pacmaninbw
    Commented Apr 15, 2021 at 18:34
  • 2
    \$\begingroup\$ On top of that, there is no description of what your code is supposed to be doing. Peter wrote a surprisingly good answer considering you gave them little to work with, but please, next time, take a look at what makes a good question. \$\endgroup\$
    – Mast
    Commented Apr 16, 2021 at 19:29

1 Answer 1

1
\$\begingroup\$

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)
    };
\$\endgroup\$
3
  • 1
    \$\begingroup\$ thank you @petercsala \$\endgroup\$
    – Chandler
    Commented Apr 14, 2021 at 17:08
  • \$\begingroup\$ This seems to be a How to question and that would make it off-topic and not answerable. \$\endgroup\$
    – pacmaninbw
    Commented Apr 15, 2021 at 18:23
  • 1
    \$\begingroup\$ @pacmaninbw Yeah I know. It was such a low hanging fruit I could not resist, sorry. \$\endgroup\$ Commented Apr 16, 2021 at 7:11

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