st-tu-dresden/salespoint

Allow lookup of custom `AccountancyEntry` extensions via `Accountancy`

cmrschwarz opened this issue · 1 comments

This issue was raised during SWP2019/20 evaluation. By request of @martinmo I'm resubmitting it here. Currently it's quite cumbersome to add custom data to UserAccounts and AccountancyEntry objects. A positive Salespoint example of a solution is the catalog package. The main Catalog's interface type is:

interface Catalog<T extends Product>

This allows the Salespoint user to subclass Product and add all the fields they want. The additional fields are still be managed by the Catalog repository, and especially when you query that repository you get out objects of your subtype, not some generic Product instances.

In contrast, Accountancy and UserAccountManager don't allow this kind of flexibility.

Therefore I suggest changing the type of Accountancy to

interface Accountancy<T extends AccountancyEntry> { … }

and of UserAccountManager to

interface UserAccountManager<T extends UserAccount> { … }

which would make them much more flexible and applicable.

I think what you suggest is fine for Accountancy. UserAccount is deliberately not designed for extension but delegation: a customer has an account, it is not one. All examples of additional data that folks intended to add to the UserAccount type have been better off in a custom domain type that's then holding a reference to the UserAccount.

Generally speaking, we shouldn't equate "extensibility" with "possible to inherit from" or "easy to inherit from". Extensibility is more often achieved by using delegation or exposing dedicated strategy interfaces so that custom implementations can be used. See LineItemFilter in the inventory module for example. Adding extensibility to the mix in the way suggested adds significant complexity for students that want to use the types as is as they need to understand why they'd have to declare Accountancy<AccountancyEntry> everywhere, which looks like stating the obvious in the first place.

I guess we even have to carefully test this for Accountancy as all other abstractions that use that pattern assume that only a single extension type is used. E.g. if you customize Order to MyOrder, using OrderManagement<MyOrder> only work if all instances added are MyOrder instances. For Accountancy, we already persist ProductPaymentEntry instances. So if a user created CustomAccountancyEntry and used Accountancy<CustomAccountancyEntry>, the call to AccountancyEntryRepository.findByDateBetween(…) would return both the ProductPaymentEntry and CustomAccountancyEntry with the former resulting in ClassCastExceptions as they don't implement CustomAccountancyEntry.