Server Refactoring #99

Merged
merged 8 commits into from Mar 2, 2016

Projects

None yet

2 participants

@hendrikebbers
Member

This pull requests contains a refactoring of some internal (not public) server APIs:

  • Classpath scan is defined in an extra class
  • Better structure of the DolphinContext class
  • Additional test
  • Task Executor is removed
  • ControllerRepository isn't a singleton anymore
  • Marked some methods as deprecated

This changes are needed / very useful to continue work on several work:

  • EventBus refactoring
  • DolphinContextListener concept on the server
  • Better Lifecycle handling

Review on Reviewable

hendrikebbers added some commits Feb 17, 2016
@hendrikebbers hendrikebbers Server side refactoring & more tests 248a669
@hendrikebbers hendrikebbers Additional Tests and refactoring 5ff0c38
@hendrikebbers hendrikebbers removed first DolphinContextListener implementation. This should be d…
…one in a future Pull Request
841dc77
@hendrikebbers hendrikebbers Removed Deperecated TaskExecutor 1b52b0f
@hendrikebbers hendrikebbers additional refactoring
87adc96
@hendrikebbers hendrikebbers added this to the 0.8.2 milestone Feb 18, 2016
@aalmiray
Contributor

Reviewed 30 of 30 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


java-server-javaee/src/main/java/com/canoo/dolphin/server/javaee/BeanFactory.java, line 44 [r1] (raw file):
why was this type/method removed? is there no need to deprecate instead?


java-server/src/main/java/com/canoo/dolphin/server/context/DolphinContextHandler.java, line 58 [r1] (raw file):
wouldn't it make sense to supply this instance as a constructor arg?


java-server/src/main/java/com/canoo/dolphin/server/impl/ClasspathScanner.java, line 1 [r1] (raw file):
file header


java-server/src/main/java/com/canoo/dolphin/server/impl/ClasspathScanner.java, line 49 [r1] (raw file):
why is this method synchronized?


java-server/src/main/java/com/canoo/dolphin/server/impl/ClasspathScanner.java, line 54 [r1] (raw file):
I definitely don't like all these magic singletons lying around in the codebase. It couples types too much and hinders testing.


Comments from the review on Reviewable.io

@hendrikebbers
Member

Review status: all files reviewed at latest revision, 5 unresolved discussions.


java-server-javaee/src/main/java/com/canoo/dolphin/server/javaee/BeanFactory.java, line 44 [r1] (raw file):
What method do you mean? Can't see any change here


java-server/src/main/java/com/canoo/dolphin/server/impl/ClasspathScanner.java, line 49 [r1] (raw file):
I have no idea if the reflections API can be used async


java-server/src/main/java/com/canoo/dolphin/server/impl/ClasspathScanner.java, line 54 [r1] (raw file):
I already removed some singletons (ControllerRepository). For this class there are tests. If this won't be a singleton we need to pass it around with each constructor.


Comments from the review on Reviewable.io

@hendrikebbers hendrikebbers license header
d83d064
@aalmiray
Contributor

Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


java-server-javaee/src/main/java/com/canoo/dolphin/server/javaee/BeanFactory.java, line 44 [r1] (raw file):
I meant the method that created a TaskProcessor.. Looks like the whole type has been removed now


java-server/src/main/java/com/canoo/dolphin/server/impl/ClasspathScanner.java, line 49 [r1] (raw file):
then I suggest to find out instead of using synchronized like this.


Comments from the review on Reviewable.io

@hendrikebbers hendrikebbers refactoring
dc2e1b9
@hendrikebbers
Member

Review status: 31 of 47 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.


java-server-javaee/src/main/java/com/canoo/dolphin/server/javaee/BeanFactory.java, line 44 [r1] (raw file):
Yes, this was deprecated since several versions and as far as I know it's not used in any project.


java-server/src/main/java/com/canoo/dolphin/server/context/DolphinContextHandler.java, line 58 [r1] (raw file):
Currently the DolphinContextHandler is a singleton. But this will be refactored as a next step.


java-server/src/main/java/com/canoo/dolphin/server/impl/ClasspathScanner.java, line 1 [r1] (raw file):
done


java-server/src/main/java/com/canoo/dolphin/server/impl/ClasspathScanner.java, line 49 [r1] (raw file):
#111


Comments from the review on Reviewable.io

@hendrikebbers
Member

@aalmiray don't want to merge this without your final review :)

@aalmiray
Contributor
aalmiray commented Mar 1, 2016

Reviewed 16 of 16 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from the review on Reviewable.io

@hendrikebbers hendrikebbers Merge branch 'master' into context-listener
a1f3c03
@hendrikebbers hendrikebbers merged commit 01f1a44 into master Mar 2, 2016

3 of 4 checks passed

code-review/reviewable Review in progress: 42 of 47 files reviewed, all discussions resolved
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+2.4%) to 24.612%
Details
@hendrikebbers hendrikebbers deleted the context-listener branch Mar 2, 2016
@aalmiray aalmiray removed the in progress label Mar 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment