Controller proxy factory #106

Merged
merged 7 commits into from Mar 2, 2016

Projects

None yet

2 participants

@hendrikebbers
Member

Refactored the creation of the Java client ControllerProxy.

Review on Reviewable

hendrikebbers added some commits Feb 18, 2016
@hendrikebbers hendrikebbers NEW CLASSES f8be2a3
@hendrikebbers hendrikebbers ControllerProxyFactory 6ce3897
@hendrikebbers hendrikebbers Merge branch 'master' into controller_proxy_factory
# Conflicts:
#	java-client/src/main/java/com/canoo/dolphin/client/ClientContextFactory.java
03efe70
@hendrikebbers hendrikebbers added this to the 0.8.2 milestone Feb 19, 2016
@aalmiray
Contributor

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


java-client/src/main/java/com/canoo/dolphin/client/ClientContext.java, line 44 [r1] (raw file):
please explain why this method is deprecated. If there's a replacement then also mention it


java-client/src/main/java/com/canoo/dolphin/client/ControllerInitalizationException.java, line 1 [r1] (raw file):
missing file header


java-client/src/main/java/com/canoo/dolphin/client/ControllerInitalizationException.java, line 6 [r1] (raw file):
I'd prefer if there was a root DolphinPlatformException instead of just using RuntimeException. Also, all exceptions are serializable so please add a serialVersionUID field.


java-client/src/main/java/com/canoo/dolphin/client/impl/ClientBeanManagerImpl.java, line 24 [r1] (raw file):
please explain why it's deprecated and if there's an alternative for this class


java-client/src/main/java/com/canoo/dolphin/client/impl/ClientContextImpl.java, line 74 [r1] (raw file):
hmm this ControllerProxyFactory contract is too big. It returns a CompletableFuture that wraps a ControllerProxy. The previous code required usage of internal DP beans (such as platformBeanRepository.getInternalAttributesBean()). I'm afraid this design will force every implementation of the factory to use the bean in one way or another, leading to duplicate code or usage of a base abstract controller factory class.

I envisioned the factory to be a replacement for invoking new ControllerProxyImpl(...). The basic idea of this factory is to decouple ClientControllerImpl from ControllerProxyImpl.


java-client/src/main/java/com/canoo/dolphin/client/impl/ControllerProxyFactory.java, line 1 [r1] (raw file):
missing file header. There's bound to be a Maven plugin that takes care of this task (Apache RAT comes to mind). Or switch to Gradle and apply the license plugin ;-)


java-client/src/main/java/com/canoo/dolphin/client/impl/ControllerProxyFactory.java, line 12 [r1] (raw file):
see comment on ClientContextImpl.. This method should return ControllerProxy<T>.


java-client/src/main/java/com/canoo/dolphin/client/impl/ControllerProxyFactoryImpl.java, line 19 [r1] (raw file):
please validate inputs. check file header


java-client/src/main/java/com/canoo/dolphin/client/impl/DolphinCommandHandler.java, line 11 [r1] (raw file):
please check inputs, file header.
What's the reasoning for creating a top-level class for this code? This used to be hidden inside ClientContextImpl. Please follow the same approach of base interface + default class impl if this type may be superclassed by developers for providing tweaks in their app configuration.


java-client/src/main/java/com/canoo/dolphin/client/impl/DolphinCommandHandler.java, line 19 [r1] (raw file):
why is this method synchronized? the only state hold by this class is the clientDolphin field. It makes no sense to synchronize here.


java-client/src/test/java/com/canoo/dolphin/client/impl/TestDolphinCommandHandler.java, line 22 [r1] (raw file):
please follow naming conventions and attach Testor Tests as a suffix


java-client/src/test/java/com/canoo/dolphin/client/impl/TestDolphinCommandHandler.java, line 47 [r1] (raw file):
I'd declare the method to throw Exception avoiding the try/catch block


Comments from the review on Reviewable.io

hendrikebbers added some commits Feb 20, 2016
@hendrikebbers hendrikebbers Changes based on review
e657279
@hendrikebbers hendrikebbers Factory returns ControllerProxy
9f8404d
@hendrikebbers
Member

Review status: 7 of 15 files reviewed at latest revision, 12 unresolved discussions.


java-client/src/main/java/com/canoo/dolphin/client/ClientContext.java, line 44 [r1] (raw file):
The ClientBeanManager extends the BeanManager and therefore provides methods like create() and remove(). Currently Michael and I think that we don't need this methods on the client. The ClientBeanManager class is deprecated since some time. I know that you are currently using methods and that's the point why we don't remove it. But maybe we find a better way to handle the needs on the client. Therefore the API of ClientBeanManager can change completely in a future version.


java-client/src/main/java/com/canoo/dolphin/client/ControllerInitalizationException.java, line 1 [r1] (raw file):
Done


java-client/src/main/java/com/canoo/dolphin/client/ControllerInitalizationException.java, line 6 [r1] (raw file):
added UID to all exceptions. Let's think about a general exception hierarchy in an additional issue: #107


java-client/src/main/java/com/canoo/dolphin/client/impl/ClientBeanManagerImpl.java, line 24 [r1] (raw file):
See my first comment.


java-client/src/main/java/com/canoo/dolphin/client/impl/ClientContextImpl.java, line 74 [r1] (raw file):
I avoided this in the first try because this will end in the usage of an additional Executor. I commit a refactoring now that uses a factory that directly returns a ControllerProxy. By doing so the ClientContextImpl will use an Executor internally.


java-client/src/main/java/com/canoo/dolphin/client/impl/ControllerProxyFactory.java, line 1 [r1] (raw file):
Header added. i create an issue to find a Maven Plugin: #108


java-client/src/main/java/com/canoo/dolphin/client/impl/ControllerProxyFactoryImpl.java, line 19 [r1] (raw file):
Done


java-client/src/main/java/com/canoo/dolphin/client/impl/DolphinCommandHandler.java, line 11 [r1] (raw file):
Based on the factory calling a Dolphin command was needed in serval points. That's why I externalized the functionality in a separate class. Since all classes that use the functionality are private classes (all *Impl) I don't think that we need an interface here.
Added Header & NullChecks


java-client/src/main/java/com/canoo/dolphin/client/impl/DolphinCommandHandler.java, line 19 [r1] (raw file):
I think I just copied it from the ClientContext. You are right.


java-client/src/test/java/com/canoo/dolphin/client/impl/TestDolphinCommandHandler.java, line 22 [r1] (raw file):
I think we should discuss this. Michael used Test as a prefix in all classes in the client. That's why I did it this way. I'm fine with any naming convention. For now I won't change it but we should define a naming convention and then refactor all tests.


java-client/src/test/java/com/canoo/dolphin/client/impl/TestDolphinCommandHandler.java, line 47 [r1] (raw file):
Done


Comments from the review on Reviewable.io

@hendrikebbers
Member

@aalmiray does the last version fit to your needs?

@aalmiray
Contributor
aalmiray commented Mar 1, 2016

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


java-client/src/main/java/com/canoo/dolphin/client/ClientContext.java, line 44 [r1] (raw file):
thanks for the explanation :-) it should be in the javadoc too


Comments from the review on Reviewable.io

hendrikebbers added some commits Mar 2, 2016
@hendrikebbers hendrikebbers JavaDoc
f1dabb4
@hendrikebbers hendrikebbers Merge branch 'master' into controller_proxy_factory
# Conflicts:
#	java-client/src/main/java/com/canoo/dolphin/client/ClientContextFactory.java
#	java-client/src/main/java/com/canoo/dolphin/client/impl/ClientContextImpl.java
6149880
@hendrikebbers
Member

Review status: 13 of 16 files reviewed at latest revision, 1 unresolved discussion.


java-client/src/main/java/com/canoo/dolphin/client/impl/ControllerProxyFactory.java, line 12 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@hendrikebbers hendrikebbers merged commit 474c935 into master Mar 2, 2016

3 of 4 checks passed

code-review/reviewable Review in progress: 13 of 16 files reviewed, 1 unresolved discussion
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 (+0.08%) to 24.69%
Details
@hendrikebbers hendrikebbers deleted the controller_proxy_factory 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