How to avoid duplication of code when using two constructors in a class C#

Question

Hi All I have two request DTOs, of which one is the parent and one is the child.

    public class PaymentSupplierPaymentRequest
{
    [JsonProperty("description")]
    public string Description { get; set; }

    [JsonProperty("amount")]
    [Required]
    public decimal Amount { get; set; }

    [JsonProperty("currency")]
    public string Currency { get; set; }

    [JsonProperty("supplierid")]
    [Required]
    public string SupplierId { get; set; }

    [JsonProperty("accountid")]
    [Required]
    public long AccountId { get; set; }
}

public class PaymentSupplierLitePaymentRequest: PaymentSupplierPaymentRequest
{
    [JsonProperty("merchantCode")]
    [Required]
    public string MerchantCode { get; set; }
}

There are two types of payment request one which the Merchant Code is required and one where the merchant code is not required. Depending on the request I have another class that uses two constructors to handle the data passed into the request.

    private readonly string _merchantCode;
    private readonly string _installationId;
    private readonly string _version;
    private readonly string _paymentMethodMaskCode;
    private readonly string _orderDescription;
    private readonly long _amount;
    private readonly string _currency;
    private readonly string _email;
    private const byte Exponent = 2;
    private readonly string _orderCode;

        public GemPayPaymentRequestBuilder(PaymentSupplierLitePaymentRequest paymentRequest)
    {
        _amount = (long)(paymentRequest.Amount * (decimal)Math.Pow(10, Exponent));
        _orderDescription = paymentRequest.Description;
        _currency = paymentRequest.Currency;
        _orderCode = $"{paymentRequest.AccountId}_{DateTime.UtcNow.Ticks}";
        _merchantCode = paymentRequest.MerchantCode;
    }

    public GemPayPaymentRequestBuilder(PaymentSupplierPaymentRequest paymentRequest, string email, WorldPayMerchantConfig config)
    {
        _amount = (long)(paymentRequest.Amount * (decimal)Math.Pow(10, Exponent));
        _orderDescription = paymentRequest.Description;
        _currency = paymentRequest.Currency;
        _orderCode = $"{paymentRequest.AccountId}_{DateTime.UtcNow.Ticks}";
        _email = email;
        _merchantCode = config.MerchantCode;
        _installationId = config.InstallationId;
        _version = config.Version;
        _paymentMethodMaskCode = config.PaymentMethodMaskCode;
    }

The problem is as you can see in the code I am duplicating the initialising of certain local variables in both constructors.

        _amount = (long)(paymentRequest.Amount * (decimal)Math.Pow(10, Exponent));
        _orderDescription = paymentRequest.Description;
        _currency = paymentRequest.Currency;
        _orderCode = $"{paymentRequest.AccountId}_{DateTime.UtcNow.Ticks}";

Does anyone have a cleaner way of doing this? I have tried using constructor inheritance for the second constructor as shown below.

       public GemPayPaymentRequestBuilder(PaymentSupplierLitePaymentRequest paymentRequest)
    {
        _amount = (long)(paymentRequest.Amount * (decimal)Math.Pow(10, Exponent));
        _orderDescription = paymentRequest.Description;
        _currency = paymentRequest.Currency;
        _orderCode = $"{paymentRequest.AccountId}_{DateTime.UtcNow.Ticks}";
        _merchantCode = paymentRequest.MerchantCode;
    }

    public GemPayPaymentRequestBuilder(PaymentSupplierPaymentRequest paymentRequest, string email, WorldPayMerchantConfig config) : this(paymentRequest)
    {
        _email = email;
        _merchantCode = config.MerchantCode;
        _installationId = config.InstallationId;
        _version = config.Version;
        _paymentMethodMaskCode = config.PaymentMethodMaskCode;
    }

But this does not work as using : this(paymentRequest) throws a conversion error as it cannot convert from PaymentSupplierPaymentRequest to PaymentSupplierLitePaymentRequest. Any Ideas?

Build an interface for PaymentSupplierLitePaymentRequest and use it instead — Maik Hasler
– Maik Hasler, Commented Jun 9, 2022 at 6:13
Alternatives: create a common base class and accept that as the input, or create a static method to convert from PaymentSupplierPaymentRequest to PaymentSupplierLitePaymentRequest and use this(ConversionMethod(paymentRequest)), or create an implicit cast operator onoverload on one of the two types to convert from one to the other. — ProgrammingLlama
– ProgrammingLlama, Commented Jun 9, 2022 at 6:13
@DiplomacyNotWar can you please shown an example by pasting an answer of the implicit cast operator. Do you mean : this((PaymentSupplierLitePaymentRequest)paymentRequest) — user17603119
– user17603119, Commented Jun 9, 2022 at 6:16
Essentially something like that, but your example uses an explicit cast operator. Would you like to go with that approach? If so, I can provide an answer. — ProgrammingLlama
– ProgrammingLlama, Commented Jun 9, 2022 at 6:17
Question: Why don't you use nullable values and just check them in the ctor? You wouldn't need a second ctor that way..? — Maik Hasler
– Maik Hasler, Commented Jun 9, 2022 at 6:20

Roman · Accepted Answer · 2022-06-09 07:30:32Z

2

Using constructors only.

The simplest way to do this is to call : this from different constructor (as PaymentSupplierLitePaymentRequest inherits PaymentSupplierPaymentRequest, not vice versa), but you need to handle other params in second constructor and change your code a bit:

public GemPayPaymentRequestBuilder(PaymentSupplierLitePaymentRequest paymentRequest) : this(paymentRequest, null, null)
{
    _merchantCode = paymentRequest.MerchantCode;
}

public GemPayPaymentRequestBuilder(PaymentSupplierPaymentRequest paymentRequest, string email, WorldPayMerchantConfig config)
{
    _amount = (long)(paymentRequest.Amount * (decimal)Math.Pow(10, Exponent));
    _orderDescription = paymentRequest.Description;
    _currency = paymentRequest.Currency;
    _orderCode = $"{paymentRequest.AccountId}_{DateTime.UtcNow.Ticks}";

    _email = email;

    if (config != null)
    {
        _merchantCode = config.MerchantCode;
        _installationId = config.InstallationId;
        _version = config.Version;
        _paymentMethodMaskCode = config.PaymentMethodMaskCode;
    }
}

Or another approach - you can create non-public constructor to initialize common data and call it from public ones (the code is cleaner (my opinion) than in approach above and you have separate place to initialize common properties without nulls and ifs):

// protected (only for common data) and PaymentSupplierPaymentRequest as parameter
protected GemPayPaymentRequestBuilder(PaymentSupplierPaymentRequest paymentRequest) 
{
    _amount = (long)(paymentRequest.Amount * (decimal)Math.Pow(10, Exponent));
    _orderDescription = paymentRequest.Description;
    _currency = paymentRequest.Currency;
    _orderCode = $"{paymentRequest.AccountId}_{DateTime.UtcNow.Ticks}";
}

public GemPayPaymentRequestBuilder(PaymentSupplierLitePaymentRequest paymentRequest) : this((PaymentSupplierPaymentRequest)paymentRequest)
{
    _merchantCode = paymentRequest.MerchantCode;
}

public GemPayPaymentRequestBuilder(PaymentSupplierPaymentRequest paymentRequest, string email, WorldPayMerchantConfig config) : this(paymentRequest)
{
    _email = email;
    _merchantCode = config.MerchantCode;
    _installationId = config.InstallationId;
    _version = config.Version;
    _paymentMethodMaskCode = config.PaymentMethodMaskCode;
}

Using separate method.

You can create separate method which will contain logic which is same for both types:

private void InitializeCommonData(PaymentSupplierPaymentRequest paymentRequest)
{
    _amount = (long)(paymentRequest.Amount * (decimal)Math.Pow(10, Exponent));
    _orderDescription = paymentRequest.Description;
    _currency = paymentRequest.Currency;
    _orderCode = $"{paymentRequest.AccountId}_{DateTime.UtcNow.Ticks}";
}

Then you can initialize properties/fields:

public GemPayPaymentRequestBuilder(PaymentSupplierLitePaymentRequest paymentRequest)
{
    this.InitializeCommonData(paymentRequest);
    _merchantCode = paymentRequest.MerchantCode;
}

public GemPayPaymentRequestBuilder(PaymentSupplierPaymentRequest paymentRequest, string email, WorldPayMerchantConfig config)
{
    this.InitializeCommonData(paymentRequest);
    _email = email;
    _merchantCode = config.MerchantCode;
    _installationId = config.InstallationId;
    _version = config.Version;
    _paymentMethodMaskCode = config.PaymentMethodMaskCode;
}

edited Jun 9, 2022 at 7:30

answered Jun 9, 2022 at 6:26

Roman

12.3k10 gold badges42 silver badges50 bronze badges

Sign up to request clarification or add additional context in comments.

12 Comments

ProgrammingLlama Over a year ago

Not OP, but I prefer your constructor approach here because it still allows readonly fields/properties to be set. [Redacted part where I missed that PaymentSupplierLitePaymentRequest is derived from PaymentSupplierPaymentRequest]

Roman Over a year ago

@DiplomacyNotWar

This does make the assumption that PaymentSupplierLitePaymentRequest is derived from or implicitly castable to PaymentSupplierPaymentRequest

- it is in question - PaymentSupplierLitePaymentRequest inherits PaymentSupplierPaymentRequest, so it should be fine (at least for OP case).

ProgrammingLlama Over a year ago

Somehow I missed that, and now I'm confused as to why OP is asking the question. :D

user17603119 Over a year ago

Yes I think I like the constructor way here too. Thanks @DiplomacyNotWar for your help but I think I like this answer. I wish I could give 2 ticks

user17603119 Over a year ago

Wait @Roman the constructor way does not work. The issue is when I use the : this(paymentrequest) on the first public constructor, I get an error saying cannot call itself. its like the protected constructor is not being picked up as the base constructor. Could that be because its protected?

|

Victor · Accepted Answer · 2022-06-09 07:48:52Z

You can pass always the base class and invoke always to your complete constructor:

    public GemPayPaymentRequestBuilder(PaymentSupplierPaymentRequest paymentRequest)
        : this(paymentRequest, null, null)
    {
    }

    public GemPayPaymentRequestBuilder(PaymentSupplierPaymentRequest paymentRequest, string email, WorldPayMerchantConfig config)
    {
        _amount = (long)(paymentRequest.Amount * (decimal)Math.Pow(10, Exponent));
        _orderDescription = paymentRequest.Description;
        _currency = paymentRequest.Currency;
        _orderCode = $"{paymentRequest.AccountId}_{DateTime.UtcNow.Ticks}";
            
        _email = email;

        var liteRequest = paymentRequest as PaymentSupplierLitePaymentRequest;
        if (liteRequest != null)
        {
            _merchantCode = liteRequest.MerchantCode;
        }
        else if (config != null)
        {
            _merchantCode = config.MerchantCode;
        }

        if (config != null)
        {
            _installationId = config.InstallationId;
            _version = config.Version;
            _paymentMethodMaskCode = config.PaymentMethodMaskCode;
        }
    }
}

In the complete constructor (the constructor that manage all possible parameters) you can try cast your class. If the class is Lite, you set it's properties.

Collectives™ on Stack Overflow

How to avoid duplication of code when using two constructors in a class C#

2 Answers 2

12 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

12 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related