[
https://issues.apache.org/jira/browse/ARIES-1062?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13648417#comment-13648417
]
John Ross commented on ARIES-1062:
----------------------------------
1. I agree the exception catching is unnecessary code. At one time, Subsystems
had a constructor that could throw these exceptions. Now it uses only the
implicit, default constructor.
2. It would be much easier to compare these particular changes in a patch. The
current code is concerned about not issuing a call to activate() for the cases
where service is an instance of IDirectoryFinder or Repository, so I was
initially concerned to see the suggested code always activating. However, I can
see that the call to activate is now harmless if it's already active or does
not have all of the required services. Still, this is a delicate area of the
code and we would need to be sure any changes here passed both the Aries tests
and OSGi CT.
3. I'm sure there are raging debates in the community about this, but
explicitly throwing an NPE to indicate that a null argument is not allowed and
would cause implicit NPEs post construction is an accepted practice. See, for
example, Effective Java, 2nd Edition, Item 60.
> org.apache.aries.subsystem.core.internal.Activator needs to be improved
> -----------------------------------------------------------------------
>
> Key: ARIES-1062
> URL: https://issues.apache.org/jira/browse/ARIES-1062
> Project: Aries
> Issue Type: Improvement
> Components: Subsystem
> Affects Versions: 1.0
> Reporter: TangYong
> Priority: Minor
>
> Currently, org.apache.aries.subsystem.core.internal.Activator class and
> org.apache.aries.subsystem.core.internal.SubsystemResolverHookFactory class
> have some curious coding as following:
> 1. activate()
> "new Subsystems()" can not throw SubsystemException.
> 2. addingService method
> I suggest the following writing:
> @Override
> public synchronized Object addingService(ServiceReference<Object>
> reference) {
> Object service = bundleContext.getService(reference);
>
> if (service instanceof IDirectoryFinder){
> finders.add((IDirectoryFinder)service);
> return service;
> }
> else if (service instanceof Repository){
> repositories.add((Repository)service);
> return service;
> }
> else if (service instanceof Coordinator){
> if (coordinator == null) {
> coordinator = (Coordinator)service;
> }
> }
> else if (service instanceof RegionDigraph) {
> if (regionDigraph == null) {
> regionDigraph = (RegionDigraph)service;
> }
> }
> else if (service instanceof Resolver) {
> if (resolver == null) {
> resolver = (Resolver)service;
> }
> }
> else if (service instanceof ModelledResourceManager) {
> if (modelledResourceManager == null) {
> modelledResourceManager =
> (ModelledResourceManager)service;
> }
> }
>
> activate();
> return service;
> }
> 3. SubsystemResolverHookFactory class's constructor
> I have never seen the following and explicitly throw NPE
> if (subsystems == null)
> throw new NullPointerException("Missing required parameter: subsystems");
> I suggest that we'd better use RuntimeException or IllegalArgumentException
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
