Skip to content
This repository has been archived by the owner. It is now read-only.

Improving scale transformation service #4439

Merged
merged 1 commit into from Oct 19, 2017

Conversation

@clinique
Copy link
Contributor

@clinique clinique 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
Copy link
Contributor Author

@clinique clinique commented Oct 18, 2017

@martinvw here is the PR for the scale transformation service

@ThomDietrich
Copy link
Contributor

@ThomDietrich ThomDietrich commented Oct 18, 2017

Related to openhab/openhab-docs#486

Thanks!

Copy link
Contributor

@martinvw martinvw 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() {
Copy link
Contributor

@martinvw martinvw Oct 18, 2017

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());
Copy link
Contributor

@martinvw martinvw Oct 18, 2017

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 + "'"));

Copy link
Contributor Author

@clinique clinique Oct 19, 2017

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);

Copy link
Contributor

@martinvw martinvw Oct 18, 2017

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());
Copy link
Contributor

@martinvw martinvw Oct 18, 2017

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>();
Copy link
Contributor

@martinvw martinvw Oct 18, 2017

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);
Copy link
Contributor

@martinvw martinvw Oct 18, 2017

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

Copy link
Contributor Author

@clinique clinique Oct 19, 2017

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

Copy link
Contributor

@martinvw martinvw Oct 19, 2017

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 {
Copy link
Contributor

@martinvw martinvw Oct 18, 2017

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

Copy link
Contributor

@martinvw martinvw Oct 18, 2017

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

Copy link
Contributor

@martinvw martinvw Oct 18, 2017

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";
Copy link
Contributor

@martinvw martinvw Oct 18, 2017

Should we name the variable evaluationOrder :-)

to ensure that they match in the same order than the scale definition file as discussed
in PR eclipse-archived#486
Corrections following code review

Signed-off-by: Gaël L'hopital <glhopital@gmail.com>
@clinique clinique force-pushed the Improve_Scale_Transformation branch from d927162 to 47c9013 Oct 19, 2017
Copy link
Contributor

@martinvw martinvw left a comment

Thanks great!

@sjsf sjsf merged commit a5330c0 into eclipse-archived:master Oct 19, 2017
2 checks passed
@sjsf
Copy link
Contributor

@sjsf sjsf commented Oct 19, 2017

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants