Can I refactor my code any more?

[Palak Shah]

Hello Everyone,

I have created one class - and it has 3 methods in it - I tried refactoring code considering all the factors; however I am not sure if there is more room for improvement in my code or not

So seek help to review my code and suggest me any means of improvement; so that I can improve more and do coding in standard way

using Base;
using Config;
using Entities;
using JsonModel;
using Model.PaymentOptions;
using Repositories;
using System.Linq;

namespace Helpers
{
    public static class PaymentOptionsHelper
    {
        public static PaymentMethod[] checkoutGroup = { PaymentMethod.AdyenDropIn, PaymentMethod.PayPalDirect, PaymentMethod.Braintree, PaymentMethod.BankTransfer, };

        /// <summary>
        /// Below method will set the Payment Type based on the low priority type retrieved from DB
        /// </summary>
        /// <param name="testConfiguration"></param>
        /// <param name="siteId"></param>
        /// <returns></returns>
        public static PaymentOptions GetPaymentOptions_Auto(TestConfigurationCDO testConfiguration, int siteId)
        {
            var paymentOptionList = SitePaymentRepository.GetSitePaymentInfoBySiteId(
                testConfiguration,
                siteId);

            var lowestPriority = paymentOptionList.Min(x => x.Priority);
            var paymentAuto = paymentOptionList.FirstOrDefault(x => x.Priority == lowestPriority);

            PaymentOptions paymentOptions;

            switch (paymentAuto.PaymentType)
            {
                case PaymentMethod.PayPalDirect:
                case PaymentMethod.Braintree:
                case PaymentMethod.BankTransfer:
                case PaymentMethod.AdyenDropIn:
                    paymentOptions = new ClientCheckoutOptions() { paymentMethod = paymentAuto.PaymentType };
                    break;

                case PaymentMethod.Klarna:
                    paymentOptions = new KlarnaOptions();
                    break;

                case PaymentMethod.PayPalExpress:
                default:
                    paymentOptions = new PaypalOptions();
                    break;
            }

            return paymentOptions;
        }

        public enum Scenario { ManualScenario_1, ManualScenario_2, ManualScenario_3, ManualScenario_4, ManualScenario_5, ManualScenario_6, ManualScenario_7, ManualScenario_8, ManualScenario_10, ManualScenario_11, ManualScenario_12, ManualScenario_13, ManualScenario_14 }

        /// <summary>
        /// Below method will set the Payment Type based on the Manual Regression Scenario
        /// </summary>
        /// <param name="testConfiguration"></param>
        /// <param name="siteId"></param>
        /// <returns></returns>
        public static PaymentOptions GetPaymentOptions_ManualRegression(SharedParameters shared_parameters, Scenario Scenario)
        {
            var cultureCode = shared_parameters.site.EnumCultureCode();

            if (cultureCode == CultureEnum.GB)
            {
                switch (Scenario)
                {
                    case Scenario.ManualScenario_1:
                    case Scenario.ManualScenario_3:
                    case Scenario.ManualScenario_10:
                        return new ClientCheckoutOptions()
                        {
                            delivery = DeliveryMethod.Billing,
                            paymentMethod = PaymentMethod.PayPalDirect
                        };

                    case Scenario.ManualScenario_5:
                        return new KlarnaOptions()
                        {
                            klarnaPaymentOptions = KlarnaPaymentOptions.PayLater,
                            delivery = DeliveryMethod.Billing
                        };

                    case Scenario.ManualScenario_8:
                    case Scenario.ManualScenario_12:
                        return new KlarnaOptions()
                        {
                            klarnaPaymentOptions = KlarnaPaymentOptions.ByCard,
                            delivery = DeliveryMethod.Billing
                        };

                    case Scenario.ManualScenario_2:
                    case Scenario.ManualScenario_4:
                    case Scenario.ManualScenario_6:
                    case Scenario.ManualScenario_7:
                    case Scenario.ManualScenario_11:
                    case Scenario.ManualScenario_13:
                    case Scenario.ManualScenario_14:
                    default:
                        return new PaypalOptions()
                        {
                            byAccount = true
                        };
                }
            }
            else
            {
                return GetPaymentOptions_NonGBCulture(shared_parameters.testConfiguration,
                shared_parameters.site.SiteId);
            }
        }

        /// <summary>
        /// Specific to NonGB Culture: Below method will set the Payment Type from pre-defined selection
        /// </summary>
        /// <param name="testConfiguration"></param>
        /// <param name="siteId"></param>
        /// <returns></returns>
        public static PaymentOptions GetPaymentOptions_NonGBCulture(TestConfigurationCDO testConfiguration, int siteId)
        {
            var paymentOptionList = SitePaymentRepository.GetSitePaymentInfoBySiteId(
            testConfiguration,
            siteId);

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

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

 

[Skydiver]

The enum identifiers ManualScenario_xxx is not very descriptive. You'll want to use descriptive identifiers.

I'm not sure that you want an object named "TestConfigurationCDO" living in your production code, but it's your code and your teams coding standards.

Indentation levels in lines 64-112 can likely be lessened by checking for non-GB first and returning immediately if so. The switch statement bump up a level since it doesn't need to be in an else clause. So you'll have code that looks something like:

if (is non-GB)
    return GetPaymentOptions_NonGBCulture( ... );

switch (scenario)
{
    :
}

Lines 123-129 can very likely be trimmed further down to look something like:

return SitePaymentRepository.GetSitePaymentInfoBySiteId( ... )
                            .First( ... )
                            .Select(p => new ClientCheckoutOptions() { paymentMethod = p.PaymentType });

 

[Skydiver]

Lines 27-28 can be combined to use OrderBy and First

 

[Skydiver]

Also you my want to decide what your coding standards are for your team with regards to switch statements and returning objects. Do you do the variable assignment to a base class and have a single return statement after the switch statement like in your first method, or do you just return the concrete class with return statements within the switch cases like in the second method?

 

[Palak Shah]

Lines 123-129 can very likely be trimmed further down to look something like:

return SitePaymentRepository.GetSitePaymentInfoBySiteId( ... )
                            .First( ... )
                            .Select(p => new ClientCheckoutOptions() { paymentMethod = p.PaymentType });

I am not able to use select afterwards!!!

return SitePaymentRepository.GetSitePaymentInfoBySiteId(testConfiguration,siteId)
                            .First(x => checkoutGroup.Contains(x.PaymentType))
                            .Select(p => new ClientCheckoutOptions() { paymentMethod = p.PaymentType });

 

[Palak Shah]

The enum identifiers ManualScenario_xxx is not very descriptive. You'll want to use descriptive identifiers.

Yes, even I think I have not used properly as I should - I am calling this methods in separate class like below:

public void QuantityDiscount_EndToEnd_Scenario()
        {
            var param = new WorkflowParameters()
            {
                designMethod = DesignMethod.UploadImage,
                signedIn = true,
                quantity = 55,
                productId = NominatedProducts.fabric,

                paymentOptions = PaymentOptionsHelper.GetPaymentOptions_ManualRegression(shared_parameters, PaymentOptionsHelper.Scenario.ManualScenario_13)
            };
}

I just don't understand what other ways I can define enumerators

 

[Palak Shah]

Also you my want to decide what your coding standards are for your team with regards to switch statements and returning objects. Do you do the variable assignment to a base class and have a single return statement after the switch statement like in your first method, or do you just return the concrete class with return statements within the switch cases like in the second method?

Usually the case happens like second method

 

[Skydiver]

I am not able to use select afterwards!!!

return SitePaymentRepository.GetSitePaymentInfoBySiteId(testConfiguration,siteId)
                            .First(x => checkoutGroup.Contains(x.PaymentType))
                            .Select(p => new ClientCheckoutOptions() { paymentMethod = p.PaymentType });

Your right. I probably should not be online while I am not feeling well. Going to need a Where instead of First, and then add a First after the Select. It may not be worth it in terms of lines saved. You'll have to decide if it is worth it for code readability.