Closed JoKeKt closed 3 months ago
Thinking further, I find the combination of sub-classes (sub-records) and required
properties difficult to handle in practice.
I tried to first create the method-specific PaymentRequest
objects and set the method-specific parameters in a big switch
statement (handling all different payment methods). And afterwards I wanted to set the common properties like Amount
, Description
, RedirectUrl
etc.
But this is not possible because I have to specify Amount
and Description
with each and every method-specific contructor (due to the required
keyword).
This means a lot of duplicated code.
Wouldn't it be better if we just removed all method-specific records and have one big PaymentRequest
record which contains all possible properties. Then one could create such an object and set all common properties. And afterwards in a big switch
statement, one could set method-specific properties.
Of course, this would not be backwards-compatible.
Maybe we could have an additional record UniversalPaymentRequest
containing all properties? (This could inherit from PaymentRequest
and just declare all missing properties)
If you want, I could provide such a UniversalPaymentRequest
via a PR.
If you do not like this idea, do you think it would be possible for me to declare such a UniversalPaymentRequest
in my own project? Since you only use normal JSON serialization, this should work without any further adjustments, right?
Hi @JoKeKt
Regarding your first point on the missing Billing / Shipping address properties: I think these are definitely new. I will create a pull request to add them and include support for the missing properties on the Address object.
About the second point: I've received feedback from other users regarding the required properties on PaymentRequest
as well and am therefore very tempted to remove the required
keyword from the PaymentRequest
record. Would that make your code easier to write?
Thanks a lot for your feedback regarding the required properties. I'm always open to suggestions!
Kind regards, Vincent
Hi @JoKeKt
I've been thinking a bit more about the payment method specific request types and I've created a PR with an alternative solution. I'm eager to hear if this improves the situation for you. The code can be found here #384
The way it works is that every payment method specific PaymentRequest
record type now has an additional constructor that takes a regular PaymentRequest
record type. This allows you to avoid all the duplicate code in your switch statement. A code example how to use it is:
[Theory]
[InlineData(PaymentMethod.CreditCard, typeof(CreditCardPaymentRequest))]
[InlineData(PaymentMethod.Ideal, typeof(IdealPaymentRequest))]
public void CreatePaymentMethodSpecificPaymentRequest(string paymentMethod, Type expectedType) {
var amount = new Amount(Currency.EUR, 50m);
var description = "my-description";
var paymentRequest = new PaymentRequest() {
Amount = new Amount(Currency.EUR, 50m),
Description = description
};
switch (paymentMethod) {
case PaymentMethod.CreditCard:
paymentRequest = new CreditCardPaymentRequest(paymentRequest) {
CardToken = "card-token"
};
break;
case PaymentMethod.Ideal:
paymentRequest = new IdealPaymentRequest(paymentRequest) {
Issuer = "ideal_issuer"
};
break;
}
paymentRequest.Should().BeOfType(expectedType);
paymentRequest.Amount.Should().Be(amount);
paymentRequest.Description.Should().Be(description);
}
If the above approach helps you, I'd be happy to merge it in.
Thank you very much for your quick reply!
Regarding your first PR which adds additional properties to the address object and which adds BillingAddress
and ShippingAddress
to PaymentRequest
:
Thank you very much! :-)
However, would it not be better to remove BillingAddress
and ShippingAddress
from CreditCardPaymentRequest
and PayPalPaymentRequest
?
It gives warnings like
"
'CreditCardPaymentRequest.ShippingAddress' hides inherited member 'PaymentRequest.ShippingAddress'. Use the new keyword if hiding was intended.
"
and I don't know what JSON serialization does when there are two properties with the same name.
Regarding your second proposal which adds additional constructors. Yes, that's a really clever way of solving the problem of repeated code! I would say: Perfect!
Hi Jokekt, you are correct regarding the compiler warning. I´ve noticed this after merging and added a new PR to fix it https://github.com/Viincenttt/MollieApi/pull/383
Hi Jokekt, you are correct regarding the compiler warning. I´ve noticed this after merging and added a new PR to fix it #383
Ah, I did not see that you have already fixed that. Great! :-)
Thanks for your feedback JoKeKt. I've just published version 4.3.0.0 of the library on NuGet, which contains the new version of the code. Feel free to open new issues if you have more feedback or notice any other missing properties.
Happy coding! Vincent
First of all, thank you for the wonderful library! :-)
In
PaymentRequest
record, there is noBillingAddress
orShippingAddress
. These two properties are only inCreditCardPaymentRequest
. However, in the docs https://docs.mollie.com/reference/create-payment both properties are stated as normal (and not method-specific) properties. Maybe that changed in the last years?Would it be possible to move these properties to the base class
PaymentRequest
?Additionally, the docs list address properties that are not covered by
AddressObject
record:Would it be possible to add these?
If you want, I could provide both changes via a PR.