OPENMEETINGS-2585 Update StreamProcessor and KurentoHandler - moving method unrelated out of it

sebawagner commented 3 years ago

Move:

Move hasActivity/hasRight to existing methods in Client.java
Static methods into Util class
Remove wrapper/methods and pass in dependency in Constructor that are required for class

I think it makes it actually quite a lot easier.

sebawagner commented 3 years ago

Can this be merged? I don't see much risk in doing.

solomax commented 3 years ago

Hello @sebawagner,

it seems my English is not understandable

I see no need in these changes at this stage

additional constructor params are pure evil :( leading to the code like this: https://sonarcloud.io/project/issues?id=apache_openmeetings&resolved=false&rules=java%3AS107&types=CODE_SMELL

I would once again suggest you to create some sort of cluster ready code (in OM or separate repo) before refactoring OM code ....

sebawagner commented 3 years ago

Closed

sebawagner commented 3 years ago

Hello @sebawagner,

it seems my English is not understandable

I see no need in these changes at this stage

additional constructor params are pure evil :( leading to the code like this: https://sonarcloud.io/project/issues?id=apache_openmeetings&resolved=false&rules=java%3AS107&types=CODE_SMELL

I would once again suggest you to create some sort of cluster ready code (in OM or separate repo) before refactoring OM code ....

Arguing that with the number of parameters initialising dependencies of a class via constructor is a "code-smell" it sounds far fetched. That is basically why you have @Autowired and other annotations, so that the constructor doesn't get too big. But the problem with those classes are that they are not beans.

They are a mix of "behaviour" and "storage" which is the nature of the problem why you need some strange constructs in order to load dependencies into them.

Behaviour should be in Controllers. Holding information should be DTOs. Mixing them leads to issues. Which is one of the reasons why it would be good to refactor this part of the code.

solomax commented 3 years ago

They are a mix of "behaviour" and "storage" which is the nature of the problem why you need some strange constructs in order to load dependencies into them.

Behaviour should be in Controllers. Holding information should be DTOs. Mixing them leads to issues. Which is one of the reasons why it would be good to refactor this part of the code.

In such case we can

directly get Bean via Spring's magic https://github.com/apache/openmeetings/blob/master/openmeetings-db/src/main/java/org/apache/openmeetings/IApplication.java#L34
manually perform injections like https://www.baeldung.com/spring-inject-bean-into-unmanaged-objects

I can do some investigation on this should not take too much time :)))

will write back here :)

solomax commented 3 years ago

@sebawagner I was able to perform @Autowire'ing for KStream using @Configurable (IApplication.getBean works as expected but it is "less magical" :)))

I can create dumb PR with PoC Or can commit the real code

what do you prefer?

apache / openmeetings

OPENMEETINGS-2585 Update StreamProcessor and KurentoHandler - moving method unrelated out of it #136