[Tradfri] Improved gateway firmware version check

[cweitkamp]

• Added better version check
• Add Test Units for version check

Closes #4904

This is a first version which I started to implement in another binding. I guess it can be enhanced a lot. Please give me some hints if you have any.

Signed-off-by: Christoph Weitkamp github@christophweitkamp.de

[maggu2810]

You should not use @NonNull but @NonNullByDefault and @Nullable.

[maggu2810]

Still missing

[cweitkamp]

To be honest, I'm not very familiar with these annotations. Used them the first time for now. Please be patient with me on that. Hopefully I did it right now. :-)

[maggu2810]

As you already dereference the version object I don't think you need to check above for null and throw a IAE. A NPE is also okay if you annotate that a non-null reference should be given.
Both are runtime exceptions, any reason to add a check and throw an IAE instead of the NPE here?

[maggu2810]

So, why use @NonNull if you add code to handle it without any error?

[maggu2810]

I would prefer to split the version on object creation once.
If you hold a version reference for multiple comparisons this could speed up the process a little bit.

[cweitkamp]

Yes of course. I thought about that too. A StringTokenizer is not an option, am I right?

[maggu2810]

As long as the version uses only integers separated by the version delimiter wouldn't it be much easier to ... in the constructor:

• split the version using the version delimiter into a string array
• create an integer list (e.g. vector) using the same size as the string array
• parse every string part and store it to the correspondent integer part

The compare to could act similar to this one:

    public static void main(String[] args) {
        System.out.println(compare(Arrays.asList(1, 2, 0, 0), Arrays.asList(1, 2)));
        System.out.println(compare(Arrays.asList(1, 2, 1, 0), Arrays.asList(1, 2)));
        System.out.println(compare(Arrays.asList(1, 2, 0, 0), Arrays.asList(1, 2, 1)));
    }

    public static int compare(List<Integer> myParts, List<Integer> otherParts) {
        final int minPartsSize = Math.min(myParts.size(), otherParts.size());

        for (int i = 0; i < minPartsSize; ++i) {
            final int diff = myParts.get(i) - otherParts.get(i);
            if (diff == 0) {
                continue;
            } else if (diff > 0) {
                return 1;
            } else {
                return -1;
            }
        }

        for (int i = minPartsSize; i < myParts.size(); ++i) {
            if (myParts.get(i) != 0) {
                return 1;
            }
        }

        for (int i = minPartsSize; i < otherParts.size(); ++i) {
            if (otherParts.get(i) != 0) {
                return -1;
            }
        }

        return 0;
    }

[maggu2810]

Wouldn't it be better to parse the strings to integer only once instead on every comparison?

private final List<Integer> parts;

public ...(final String version) {
    parts = Arrays.stream(version.split(VERSION_DELIMITER)).map(part -> Integer.parseInt(part))
            .collect(Collectors.toList());
}

[cweitkamp]

I did. Thanks.

[maggu2810]

Do we need that function at all?

[cweitkamp]

Yes, I think so. How should we access the parts of the other TradfriVersion object in the compareTo() method?

[maggu2810]

How should we access the parts of the other ...

Hm, you can access all the private members as long as you work with an instance of the same class. So, as written in my code example you can simple access other.parts the same way as this.parts.

See e.g. https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html
"The private modifier specifies that the member can only be accessed in its own class."

[cweitkamp]

I see. Cool. Thanks for your support and pointing that out. I'm going to use package-private access to allow using it in the test case.

[maggu2810]

Still missing

[cweitkamp]

@maggu2810 Thanks for your review. I picked it up at the same time you answered again.

[maggu2810]

Thank you for your work!

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/cleaning-up-warning-does-anyone-know-meaning-of-this/46762/2