starting to obsolete properties and go for methods

[stephanstapel]

#372

[stephanstapel]

@tobitege : can you review this pr? Code seems to be working, all tests are green.
Tests not yet been adopted.

[tobitege]

I'm a bit skeptical about the purpose of these changes, do these fix a problem?

Adding extra "Get" methods for all descriptor List elements just adds interface bloat and functionally causes unnecessary code changes - imho with little to no advantages.
It should suffice to have them as "private set".

[tobitege]

The processing of SpecifiedTradeSettlementPaymentMeans is not correct, imo.
Each means (card, bank account) is its own node within the NodeList of SpecifiedTradeSettlementPaymentMeans and elements within it are not lists.
Thus below "for" loops and the use of "numberOfAccounts" is obsolete (InvoiceDescriptor20Reader.cs probably the same issue).
From ZF 2.3.2:

[stephanstapel]

Let's address this separately

[stephanstapel]

I'm a bit skeptical about the purpose of these changes, do these fix a problem?

Adding extra "Get" methods for all descriptor List elements just adds interface bloat and functionally causes unnecessary code changes - imho with little to no advantages. It should suffice to have them as "private set".

I understand this. Currently, items are added to these lists using Add() methods but items are retrieved accessing the pure properties. That is unbalanced and I'd like to correct this.

[georg-jung]

As the new Get*() APIs still return mutable lists, don't I have multiple ways to achieve the same?

E.g. I could write both of

GetCreditorFinancialAccounts().Add(new Bank Account() { ... });
AddCreditorFinancialAccount( ... );

// or

GetCreditorFinancialAccounts().Any();
AnyCreditorFinancialAccount();

Wouldn't these currently be equivalent? Does it have a benefit to provide both of them then?

One advantage could be that e.g. AnyCreditorFinancialAccount()'s implementation could change in the future. But wouldn't that still require a breaking change then to discourage users from directly using GetCreditorFinancialAccounts().Any() or GetCreditorFinancialAccounts().Count == 0?

I'm not experienced with ZUGFeRD-chsarp, but could it make sense to keep the List properties and instead deprecate the APIs that mainly replicate List's API?

From a consumer's perspective, is it really easier to write

AddCreditorFinancialAccount("DE07123412341234123412", "XXXXDE11XXX");

then

CreditorBankAccounts.Add(new() { IBAN = "DE07123412341234123412", BIC = "XXXXDE11XXX" });

I think the latter provides a lot of implicit familarities. I know what's going on in an instant. The former might do more stuff under the hood, but I can't know if it does.

Imo, developers are quite used to working with List and thus just providing a List property is straight forward to use and has minimal API surface and maintenance burden. When seeing these kind of Java-style getters as a .NET developer, I'd be confused if there is a reason the API was designed this way and thus if e.g. GetCreditorFinancialAccounts().Any(); and AnyCreditorFinancialAccount(); are equaivalent or maybe in a subtle way not.

What do you think?

[georg-jung]

That is unbalanced and I'd like to correct this.

One option might be to differentiate cases where Add does more under the hood and those where it doesn't. E.g. because AddTradeLineCommentItem does more than instantiating a new TradeLineItem(),

public List<TradeLineItem> TradeLineItems { get; internal set; } = new List<TradeLineItem>();

could be

public IReadOnlyList<TradeLineItem> TradeLineItems => theBackingField.AsReadOnly();

while

 public List<BankAccount> CreditorBankAccounts { get; internal set; }

could stay a list property with AddCreditorFinancialAccount and AnyCreditorFinancialAccount obsoleted. 🤔