small-code-quality-improvements #3

TheRealHaui · 2017-09-23T18:51:31Z

Made small code quality improvements.

desruisseaux · 2017-09-23T20:06:01Z

core/sis-utility/src/main/java/org/apache/sis/util/collection/RangeSet.java

@@ -412,7 +412,7 @@ private boolean isSorted() {
        if (array == null) {
            return true;
        }
-        final boolean strict = isMinIncluded | isMaxIncluded;
+        final boolean strict = isMinIncluded || isMaxIncluded;


Actually, if the compiler were generating code verbatim, the use of | operator would be more efficient than || because a bitwise OR operation is faster than a jump on modern processor. But I guess that in practice, optimization done by the compiler results in the same assembly code in the end.

Well, that astonished me due to the general known logical vs. bitwise stuff.
However, I've learned something and you're completely right -
http://javarevisited.blogspot.co.at/2015/01/difference-between-bitwsie-and-logical.html
Thank you!
I am going to undo this change.

desruisseaux · 2017-09-23T20:10:15Z

core/sis-utility/src/main/java/org/apache/sis/xml/Pooled.java

-                    if (versionGML.compareTo(LegacyNamespaces.VERSION_3_2_1) < 0) {
-                        return FilterVersion.GML31;
-                    }
+                if (versionGML != null && versionGML.compareTo(LegacyNamespaces.VERSION_3_2_1) < 0) {


The reason why this condition was keep in two separated if statement is because we currently check for only one version (GML 3.2.1), but we are likely to add new versions in the future. So the code may become something like:

if (version GML != null) { if (versionGML.compareTo(VERSION_3_2_1) < 0) do something if (versionGML.compareTo(VERSION_3_3) >= 0) do something else etc. }

Going to undo this too.

desruisseaux · 2017-09-25T14:15:31Z

core/sis-utility/src/main/java/org/apache/sis/math/Statistics.java

@@ -310,7 +310,6 @@ public void accept(final long sample) {
    public void combine(final Statistics stats) {
        ArgumentChecks.ensureNonNull("stats", stats);

-        // "if (a < b)" is equivalent to "if (!isNaN(a) && a < b)".


Why removing the comment?

It seemed unnecessary.
Therefore I removed it to increase readability.

Going to undo the change.

The key element in the comment was the isNaN(a) part. Not so many developers are aware that (a < b) and !(a >= b) are not equivalent when using floating point numbers.

desruisseaux · 2017-09-25T14:20:14Z

core/sis-utility/src/main/java/org/apache/sis/util/collection/WeakHashSet.java

@@ -146,7 +146,6 @@ public WeakHashSet(final Class<E> type) {
        @SuppressWarnings("unchecked")
        @Workaround(library="JDK", version="1.7")
        final Entry[] table = (Entry[]) Array.newInstance(Entry.class, MIN_CAPACITY);
-//      table = new Entry[size];


Just before this comment, they were another comment explaining why we keep this comment.

Missed that comment.
Going to undo.

desruisseaux · 2017-09-25T14:21:06Z

core/sis-utility/src/main/java/org/apache/sis/util/collection/WeakValueHashMap.java

@@ -464,6 +463,7 @@ public V put(final K key, final V value) throws NullArgumentException {
     *
     * @since 0.7
     */
+    @Override


In this particular case, this annotation can not be applied on a branch targeting JDK7.

Thanks, going to undo.

desruisseaux · 2017-09-25T14:26:03Z

core/sis-utility/src/main/java/org/apache/sis/util/iso/DefaultScopedName.java

                case 2:  tail = parsedNames.get(1); break;
                case 1:  // fall through
                case 0:  throw new AssertionError(size);
+                default: tail = new DefaultScopedName(parsedNames.subList(1, size)); break;


The previous code had a logical ordering from highest value (where default stands for all values greater than 2) to zero.

I considered it to be a small improvement due to https://sonarcloud.io/organizations/default/rules#rule_key=squid%3ASwitchLastCaseIsDefaultCheck

Going to undo.

In this switch the default is not for defensing programming, but is a case expected to be run in normal execution (actually most of the time for that switch statement).

desruisseaux · 2017-09-25T14:26:38Z

core/sis-utility/src/main/java/org/apache/sis/util/logging/LoggerAdapter.java

@@ -571,6 +571,7 @@ public void logp(final Level level, final String sourceClass, final String sourc
     *
     * @since 0.5
     */
+    @Override


In those particular cases, this annotation can not be added on a code targeting JDK7 platform, because this method had been added in JDK8.

git-svn-id: https://svn.apache.org/repos/asf/sis/branches/JDK8@1809637 13f79535-47bb-0310-9956-ffa450edef68

desruisseaux · 2017-09-25T15:27:24Z

Committed on 1f9c213

TheRealHaui · 2017-09-25T15:46:05Z

Okay, just in that minute saw you've already commited the changes you accepted.
Thanks!

Shall I push further changes I consider to be improvements?

TheRealHaui · 2017-09-25T15:55:15Z

Further changes I consider to be improvements general in the SIS project NOT in this pull request.

desruisseaux · 2017-09-25T15:57:01Z

If you wish that would nice! But I propose that you do the patch against JDK8 branch instead of trunk. Also when doing tests, I suggest the following:

Focus more on the method contract instead than existing method behavior. For example having toString() to throw a NullPointerException is not part of method contract.
Verify if an existing test is already doing what you want to test. Try to identify what is new in the test you are adding. This is not necessarily a new method, but may be the context in which the method is invoked.
Avoid, if possible, to mix downstream functionalities in a test. For example ArrayEnvelopeTest should not use GeneralEnvelope or ImmutableEnvelope.
Choose variable names saying what the data are instead than their type. For example coordinate instead of doubleArray.

Because the workaround may introduce two categories with same name, add a "#2", "#3", etc. suffix after duplicated names.

small-code-quality-improvements Made small code quality improvements.

f9dce79

desruisseaux reviewed Sep 23, 2017

View reviewed changes

small-code-quality-improvements Reversed not effective changes.

4dab1c8

desruisseaux reviewed Sep 25, 2017

View reviewed changes

asfgit pushed a commit that referenced this pull request Sep 25, 2017

Minor code cleaning submitted by Michael Hausegger on #3

1f9c213

git-svn-id: https://svn.apache.org/repos/asf/sis/branches/JDK8@1809637 13f79535-47bb-0310-9956-ffa450edef68

asfgit closed this Jun 11, 2018

asfgit pushed a commit that referenced this pull request Sep 28, 2022

Add a workaround for missing "no data" value in some GCOM files.

5c4559b

Because the workaround may introduce two categories with same name, add a "#2", "#3", etc. suffix after duplicated names.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

small-code-quality-improvements #3

small-code-quality-improvements #3

TheRealHaui commented Sep 23, 2017

desruisseaux Sep 23, 2017

TheRealHaui Sep 23, 2017

desruisseaux Sep 23, 2017

TheRealHaui Sep 23, 2017

desruisseaux Sep 25, 2017

TheRealHaui Sep 25, 2017

desruisseaux Oct 12, 2017

desruisseaux Sep 25, 2017

TheRealHaui Sep 25, 2017

desruisseaux Sep 25, 2017

TheRealHaui Sep 25, 2017

desruisseaux Sep 25, 2017

TheRealHaui Sep 25, 2017

desruisseaux Sep 25, 2017

desruisseaux Sep 25, 2017

TheRealHaui Sep 25, 2017

desruisseaux commented Sep 25, 2017

TheRealHaui commented Sep 25, 2017

TheRealHaui commented Sep 25, 2017

desruisseaux commented Sep 25, 2017

small-code-quality-improvements #3

small-code-quality-improvements #3

Conversation

TheRealHaui commented Sep 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desruisseaux commented Sep 25, 2017

TheRealHaui commented Sep 25, 2017

TheRealHaui commented Sep 25, 2017

desruisseaux commented Sep 25, 2017