Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

Review abstraction levels of services #61

Open
odrotbohm opened this issue May 25, 2020 · 5 comments
Open

Review abstraction levels of services #61

odrotbohm opened this issue May 25, 2020 · 5 comments
Assignees
Labels
enhancement New feature or request in review Moderators are investigating how to best proceed with the issue

Comments

@odrotbohm
Copy link

Current Implementation

The services currently expose a mixture of abstraction levels in their APIs that provide an incoherent set of operations and lead to a fractured programming model. Take the TAN redemption use case for example. The programming model currently looks as follows:

  1. A controller loads an instance of the aggregate.
  2. There are some business checks applied.
  3. If they succeed, state is manipulated by calling some setters.
  4. The state is persisted by calling save(…).

That's a very low level interaction model and it's error prone fro a couple of reasons:

  1. You can accidentally forget one of the steps and might not actually realize this. Ideally a business operation consists of exactly one call to some service that either fails or succeeds.
  2. State is manipulated through setters. I.e. we're poking on individual fields. It's the responsibility of the calling code to make sure that the right setters are called, potentially in the right order. Also, as the setters are public, any code can actually call them.

Suggested Enhancement

  1. Revisit all methods that call save(…) on the service, look for which state transitions they apply to the entity beforehand and turn those use cases into service methods. All code that does not need any references to other Spring beans could go into entities ("If you need to call two setters in a row you're missing a method.").
  2. In combination with Avoid layer packages #60, setters could be made package protected, so that the web code is unable to call those individually but is forced to go through the service by the compiler.

Expected Benefits

  1. Reduces the amount of ceremony to be implemented in controllers to realize a use case.
  2. A clear separation of code that is strict (services, entities, etc.) and prevents any kind of misuse through the help of the compiler or by rejecting invalid state. This can then be separated by code that needs to be "forgiving", i.e. also accept invalid data by clients in the first place to produce proper results (usually though some kind of validation).

Again, happy to provide a POC for this in a PR.

@odrotbohm odrotbohm added the enhancement New feature or request label May 25, 2020
@jensmuehlner jensmuehlner added the in review Moderators are investigating how to best proceed with the issue label May 25, 2020
@anjeyy
Copy link

anjeyy commented May 30, 2020

This would be really welcoming, since moving parts of controller logic into the service and if necessary in further classes to get a good encapsulation and readability.

For example take

  • generateRegistrationToken(...) - checking for keyType and perform actions on it. Method is doing multiple things based on the keyType. (Reference: Robert C. Martin , Clean Code. Chapter 3 Functions)

  • generateTan(...) - same as above mentioned. Mixing level of abstraction.

@damirseit
Copy link

This would be really welcoming, since moving parts of controller logic into the service and if necessary in further classes to get a good encapsulation and readability.

For example take

  • generateRegistrationToken(...) - checking for keyType and perform actions on it. Method is doing multiple things based on the keyType. (Reference: Robert C. Martin , Clean Code. Chapter 3 Functions)
  • generateTan(...) - same as above mentioned. Mixing level of abstraction.

Dear @anjeyy , I would like to make this code refactor if possible.

@damirseit
Copy link

This would be really welcoming, since moving parts of controller logic into the service and if necessary in further classes to get a good encapsulation and readability.
For example take

  • generateRegistrationToken(...) - checking for keyType and perform actions on it. Method is doing multiple things based on the keyType. (Reference: Robert C. Martin , Clean Code. Chapter 3 Functions)
  • generateTan(...) - same as above mentioned. Mixing level of abstraction.

Dear @anjeyy , I would like to make this code refactor if possible.

@alstiefel @dfischer-tech @f11h @jhagestedt @mlaue-tech @mschulte-tsi @tence @kreincke @ascheibal @BugBuster1701

Is it possible to accept me as a contributor, please? Even though I am a beginner in open source project, I would be very grateful for you acception. Thanks in advance!

@ascheibal
Copy link
Contributor

Hi @damirseit,
I see, that you already forked the verification server. You can apply your changes to your private copy and then make a pull request to get your changes accepted for the official main branch. That would also identify you as an official contributor.
Would this work for you?

@damirseit
Copy link

Hi @damirseit, I see, that you already forked the verification server. You can apply your changes to your private copy and then make a pull request to get your changes accepted for the official main branch. That would also identify you as an official contributor. Would this work for you?

Yes, thank you very much! I already made a pull request. Would be extremely grateful for your feedback. Thanks in advance!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request in review Moderators are investigating how to best proceed with the issue
Projects
None yet
Development

No branches or pull requests

6 participants