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

adapt ConfigOptionProvider javadoc to reality #3761

Merged
merged 1 commit into from Jun 30, 2017

Conversation

sjsf
Copy link
Contributor

@sjsf sjsf commented Jun 28, 2017

...as several implementations return null. Also, it does not make
much sense to instantiate empty collections all the time, as the
majority of call will be returned with no results.

Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com

@@ -28,8 +28,7 @@
* the parameter name for which the requested options shall be returned
* @param locale
* locale
* @return the configuration options provided by this provider (not
* null, could be empty)
* @return the configuration options provided by this provider if any (may be null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, can you add some information on which situations we should return an empty collection and on which one we should return null?
What makes the difference?

Can we also change the current implementations in the ESH repo to use a consistent logic.

E.g.

  • the URI is not handled => null, otherwise empty or filled collection
  • URI or param not handled => null, otherwise a collection that contains the parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the value of differentiating it?
I simply adapted to JavaDoc do explicitly allow returning null, i.e. to indicate that the caller is able to handle it.

@maggu2810
Copy link
Contributor

Isn't it much simpler to be able to use:

for (final ParameterOption option : provider.getParameterOptions()) {
    ...
}

instead of

final Collection<ParameterOption> options = provider.getParameterOptions();
if (options != null) {
    for (final ParameterOption option : provider.getParameterOptions()) {
        ...
    }
}

What is the value of differentiating it?

If there is no difference between an empty collection and null, I would prefer to change the implementation.
If Collections.empty... is used, no new instances are created, because this methods return singeltons.

Shouldn't be the documentation of the API be as detailed as possible?
Previously the developer has been told to return never null, but an empty collection.
Now he is allowed to return both.
I don't know in which situations I should return the one and in which the other.
Now the documentation of the interface is changed to fit some implementations of the interface.

@sjsf
Copy link
Contributor Author

sjsf commented Jun 28, 2017

Now the documentation of the interface is changed to fit some implementations of the interface.

No, it is changed to fit the implementation of the caller.

We have a similar pattern everywhere (e.g. ThingHandlerFactories, ConfigDescriptionProviders, StateDescriptionProvider,....): They all are allowed to return null in case they don't have anything to contribute.

@sjsf
Copy link
Contributor Author

sjsf commented Jun 30, 2017

I made the javadoc more clear so that it does not imply anymore that there is a difference between an empty collection and null.

Also, I changed the persistence service registry to return null in case it does not have anything. It was the only implementation that alway returned an empty collection.

@maggu2810
Copy link
Contributor

Thanks.

What about org.eclipse.smarthome.magic.binding.internal.MagicServiceImpl?
It also contains return Collections.emptyList(); twice.

...as several implementations return null. Also, it does not make
much sense to instantiate empty collections all the time, as the
majority of call will be returned with no results.

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
@sjsf
Copy link
Contributor Author

sjsf commented Jun 30, 2017

Ouch, well... Had it closed because I was switching between old branches and therefore missed it. Thanks for catching this!

@maggu2810 maggu2810 merged commit d3de694 into eclipse-archived:master Jun 30, 2017
@maggu2810
Copy link
Contributor

Travis failed because of the Magic bundle test.
The test requires an empty collection.
I pressed "merge" too fast.

@maggu2810 maggu2810 mentioned this pull request Jun 30, 2017
@maggu2810
Copy link
Contributor

See: #3781

kaikreuzer pushed a commit that referenced this pull request Jun 30, 2017
Related to: #3761 (comment)
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants