Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving scale transformation service #4439

Merged
merged 1 commit into from Oct 19, 2017

Conversation

Projects
None yet
5 participants
@clinique
Copy link
Contributor

commented Oct 18, 2017

to ensure that they match in the same order than the scale definition file as discussed
in PR openhab/openhab-docs#486

Signed-off-by: Gaël L'hopital glhopital@gmail.com

@clinique

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2017

@martinvw here is the PR for the scale transformation service

@ThomDietrich

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

Related to openhab/openhab-docs#486

Thanks!

@martinvw
Copy link
Contributor

left a comment

Thanks @clinique nice that you resolved it, I really like that is more deterministic now!

I added some remarks, if you have any questions please let me know.

private static final long serialVersionUID = 3860553217028220119L;
private final HashSet<Object> keys = new LinkedHashSet<Object>();

public OrderedProperties() {

This comment has been minimized.

Copy link
@martinvw

martinvw Oct 18, 2017

Contributor

Consider leaving this constructor away, it will be added by the compiler anyway

}
Optional<Range> match = data.keySet().stream().filter(range -> range.contains(value)).findFirst();
if (match.isPresent()) {
return data.get(match.get());

This comment has been minimized.

Copy link
@martinvw

martinvw Oct 18, 2017

Contributor

I believe we can also do it like this:

return data.entrySet().stream().
    filter(e -> e.getKey().contains(value)).
    findFirst().
    map(Map.Entry::getValue).
    orElseThrow(() -> new TransformationException("No matching range for '" + source + "'"));

This comment has been minimized.

Copy link
@clinique

clinique Oct 19, 2017

Author Contributor

Thanks for your review @martinvw . So much left to do on so small changes :) I still have to progress.


String transformedResponse = processor.transform(evaluationorder, source);
Assert.assertEquals("first", transformedResponse);

This comment has been minimized.

Copy link
@martinvw

martinvw Oct 18, 2017

Contributor

Maybe leave out this newline, note that it also is a notice in the (upcoming) release of the static code analysis.

}

public Iterable<Object> orderedKeys() {
return Collections.list(keys());

This comment has been minimized.

Copy link
@martinvw

martinvw Oct 18, 2017

Contributor

I think that we can also return keys direct;ly I do not yet see the added value of the current solution:

Set<Object> orderedKeys() {
   return keys;
}
@@ -37,28 +41,50 @@
/** RegEx to extract a scale definition */
private static final Pattern LIMITS_PATTERN = Pattern.compile("(\\[|\\])(.*)\\.\\.(.*)(\\[|\\])");

class OrderedProperties extends Properties {
private static final long serialVersionUID = 3860553217028220119L;
private final HashSet<Object> keys = new LinkedHashSet<Object>();

This comment has been minimized.

Copy link
@martinvw

martinvw Oct 18, 2017

Contributor

Please use the diamond operator, ie replace the second occurrence of the same generic by <>


@Override
public Enumeration<Object> keys() {
return Collections.<Object> enumeration(keys);

This comment has been minimized.

Copy link
@martinvw

martinvw Oct 18, 2017

Contributor

Please use the diamond operator, ie replace the second occurrence of the same generic by <>

This comment has been minimized.

Copy link
@clinique

clinique Oct 19, 2017

Author Contributor

Diamond operator does not work here. So left it as is.

This comment has been minimized.

Copy link
@martinvw

martinvw Oct 19, 2017

Contributor

Hmm, sorry then Intellij had some other complaint about that line but its not that important :-)

@@ -37,28 +41,50 @@
/** RegEx to extract a scale definition */
private static final Pattern LIMITS_PATTERN = Pattern.compile("(\\[|\\])(.*)\\.\\.(.*)(\\[|\\])");

class OrderedProperties extends Properties {

This comment has been minimized.

Copy link
@martinvw

martinvw Oct 18, 2017

Contributor

I believe we should add the author and goal for this inner class as well.

This comment has been minimized.

Copy link
@martinvw

martinvw Oct 18, 2017

Contributor

Should this inner class be static I see no reason to have a reference to the the outer class.

This comment has been minimized.

Copy link
@martinvw

martinvw Oct 18, 2017

Contributor

Should we document that this implementation has limitations, eg iterating over the entryset will not be Ordered, removing elements will cause misery etc.

@Test
public void testEvaluationOrder() throws TransformationException {
// Ensures that only first matching scale as presented in the file is taken in account
String evaluationorder = "scale/evaluationorder.scale";

This comment has been minimized.

Copy link
@martinvw

martinvw Oct 18, 2017

Contributor

Should we name the variable evaluationOrder :-)

Improving scale transformation service
to ensure that they match in the same order than the scale definition file as discussed
in PR #486
Corrections following code review

Signed-off-by: Gaël L'hopital <glhopital@gmail.com>

@clinique clinique force-pushed the clinique:Improve_Scale_Transformation branch from d927162 to 47c9013 Oct 19, 2017

@martinvw
Copy link
Contributor

left a comment

Thanks great!

@sjka sjka merged commit a5330c0 into eclipse:master Oct 19, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details
@sjka

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.