#27: fixed sonar findings (#86)

AnastasiiaSergienko · web-flow · commit e28937cff376 · 2020-07-06T11:03:09.000+02:00
* #27: fixed sonar findings
diff --git a/doc/changes/changes-4.0.0.md b/doc/changes/changes-4.0.0.md
@@ -11,6 +11,10 @@
 * #76: Added `SELECT FROM VALUES ... AS` support.
 * #77: Added `SELECT FROM (SELECT ...)` support.
 
+## Refactoring
+
+* #27: Fix sonar findings.
+
 ## Dependency updates
  
 * Added `org.sonatype.ossindex.maven:ossindex-maven-plugin:3.1.0`
diff --git a/src/main/java/com/exasol/datatype/type/IntervalDayToSecond.java b/src/main/java/com/exasol/datatype/type/IntervalDayToSecond.java
@@ -3,7 +3,7 @@
 import com.exasol.sql.ColumnDefinitionVisitor;
 
 /**
- * This class implements the Exasol-proprietary data type interval day to second
+ * This class implements the Exasol-proprietary data type interval day to second.
  */
 public class IntervalDayToSecond implements DataType {
     private static final String NAME = "INTERVAL DAY(%s) TO SECOND(%s)";
@@ -48,7 +48,7 @@ public int getYearPrecision() {
     }
 
     /**
-     * @return millisecond precisiom
+     * @return millisecond precision
      */
     public int getMillisecondPrecision() {
         return this.millisecondPrecision;
diff --git a/src/main/java/com/exasol/datatype/value/IntervalDayToSecond.java b/src/main/java/com/exasol/datatype/value/IntervalDayToSecond.java
@@ -26,6 +26,7 @@
  * This is also the recommended way to represent the interval values in other systems which do
  * not natively support this data type.
  */
+@java.lang.SuppressWarnings("squid:S4784") //regular expression is safe here
 public final class IntervalDayToSecond extends AbstractInterval {
     private static final int SIGN_MATCHING_GROUP = 1;
     private static final int DAYS_MATCHING_GROUP = 2;
diff --git a/src/main/java/com/exasol/datatype/value/IntervalYearToMonth.java b/src/main/java/com/exasol/datatype/value/IntervalYearToMonth.java
@@ -23,6 +23,7 @@
  * also the recommended way to represent the interval values in other systems which do not
  * natively support this data type.
  */
+@java.lang.SuppressWarnings("squid:S4784") //regular expression is safe here
 public final class IntervalYearToMonth extends AbstractInterval {
     private static final int SIGN_MATCHING_GROUP = 1;
     private static final int YEARS_MATCHING_GROUP = 2;
diff --git a/src/main/java/com/exasol/sql/dql/select/rendering/SelectRenderer.java b/src/main/java/com/exasol/sql/dql/select/rendering/SelectRenderer.java
@@ -105,20 +105,22 @@ public void visit(final OrderByClause orderByClause) {
         appendKeyWord(" ORDER BY ");
         appendListOfValueExpressions(orderByClause.getColumnReferences());
         final Boolean desc = orderByClause.getDesc();
-        appendStringDependingOnBoolean(desc, " DESC", " ASC");
+        if (desc != null) {
+            appendStringDependingOnBoolean(desc, " DESC", " ASC");
+        }
         final Boolean nullsFirst = orderByClause.getNullsFirst();
-        appendStringDependingOnBoolean(nullsFirst, " NULLS FIRST", " NULLS LAST");
+        if (nullsFirst != null) {
+            appendStringDependingOnBoolean(nullsFirst, " NULLS FIRST", " NULLS LAST");
+        }
         setLastVisited(orderByClause);
     }
 
-    private void appendStringDependingOnBoolean(final Boolean booleanValue, final String string1,
+    private void appendStringDependingOnBoolean(final boolean booleanValue, final String string1,
             final String string2) {
-        if (booleanValue != null) {
-            if (booleanValue) {
-                appendKeyWord(string1);
-            } else {
-                appendKeyWord(string2);
-            }
+        if (booleanValue) {
+            appendKeyWord(string1);
+        } else {
+            appendKeyWord(string2);
         }
     }
 
diff --git a/src/main/java/com/exasol/sql/expression/rendering/AbstractExpressionRenderer.java b/src/main/java/com/exasol/sql/expression/rendering/AbstractExpressionRenderer.java
@@ -61,11 +61,10 @@ protected void appendAutoQuoted(final String identifier) {
     }
 
     protected void appendCommaWhenNeeded(final ValueExpression valueExpression) {
-        if (this.lastVisited != null && !(valueExpression.getParent() instanceof BinaryArithmeticExpression)) {
-            if (this.lastVisited.isSibling(valueExpression) || (valueExpression.getParent() != this.lastVisited
-                    && this.lastVisited.getClass().equals(valueExpression.getClass()))) {
-                append(", ");
-            }
+        if ((this.lastVisited != null && !(valueExpression.getParent() instanceof BinaryArithmeticExpression))
+                && (this.lastVisited.isSibling(valueExpression) || (valueExpression.getParent() != this.lastVisited
+                        && this.lastVisited.getClass().equals(valueExpression.getClass())))) {
+            append(", ");
         }
     }
 
diff --git a/src/main/java/com/exasol/util/AbstractBottomUpTreeNode.java b/src/main/java/com/exasol/util/AbstractBottomUpTreeNode.java
@@ -1,21 +1,12 @@
 package com.exasol.util;
 
-import java.util.*;
+import java.util.Arrays;
+import java.util.List;
 
 /**
  * This is an abstract base class for nodes in a tree structure.
  */
-public abstract class AbstractBottomUpTreeNode implements TreeNode {
-    private TreeNode parent = null;
-    private final List<TreeNode> children;
-
-    /**
-     * Create a new instance of a {@link AbstractBottomUpTreeNode} that serves as leaf node for a tree.
-     */
-    public AbstractBottomUpTreeNode() {
-        this.children = Collections.emptyList();
-    }
-
+public abstract class AbstractBottomUpTreeNode extends AbstractTree {
     /**
      * Create a new instance of a {@link AbstractBottomUpTreeNode}.
      *
@@ -68,49 +59,14 @@ public TreeNode getRoot() {
         }
     }
 
-    @Override
-    public TreeNode getParent() {
-        return this.parent;
-    }
-
     @Override
     public void addChild(final TreeNode child) {
         throw new UnsupportedOperationException("Node \"" + child.toString()
                 + "\" can only be added as child node in parent constructor in a bottom-up tree.");
     }
 
-    @Override
-    public List<TreeNode> getChildren() {
-        return this.children;
-    }
-
-    @Override
-    public TreeNode getChild(final int index) {
-        return this.children.get(index);
-    }
-
-    @Override
-    public boolean isRoot() {
-        return (this == getRoot());
-    }
-
-    @Override
-    public boolean isChild() {
-        return (this.parent != null);
-    }
-
-    @Override
-    public boolean isSibling(final TreeNode node) {
-        return (this.parent != null) && (this.getParent().getChildren().contains(node));
-    }
-
     @Override
     public void setParent(final TreeNode parent) {
         this.parent = parent;
     }
-
-    @Override
-    public boolean isFirstSibling() {
-        return (this.parent != null) && (this.getParent().getChild(0) == this);
-    }
 }
diff --git a/src/main/java/com/exasol/util/AbstractTree.java b/src/main/java/com/exasol/util/AbstractTree.java
@@ -0,0 +1,47 @@
+package com.exasol.util;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * An abstract base for {@link TreeNode} implementations.
+ */
+public abstract class AbstractTree implements TreeNode {
+    protected TreeNode parent = null;
+    protected List<TreeNode> children = new ArrayList<>();
+
+    @Override
+    public TreeNode getParent() {
+        return this.parent;
+    }
+
+    @Override
+    public List<TreeNode> getChildren() {
+        return this.children;
+    }
+
+    @Override
+    public TreeNode getChild(final int index) {
+        return this.children.get(index);
+    }
+
+    @Override
+    public boolean isRoot() {
+        return (this == getRoot());
+    }
+
+    @Override
+    public boolean isChild() {
+        return (this.parent != null);
+    }
+
+    @Override
+    public boolean isFirstSibling() {
+        return (this.parent != null) && (this.getParent().getChild(0) == this);
+    }
+
+    @Override
+    public boolean isSibling(final TreeNode node) {
+        return (this.parent != null) && (this.getParent().getChildren().contains(node));
+    }
+}
diff --git a/src/main/java/com/exasol/util/AbstractTreeNode.java b/src/main/java/com/exasol/util/AbstractTreeNode.java
@@ -1,15 +1,10 @@
 package com.exasol.util;
 
-import java.util.ArrayList;
-import java.util.List;
-
 /**
  * This is an abstract base class for nodes in a tree structure.
  */
-public abstract class AbstractTreeNode implements TreeNode {
+public abstract class AbstractTreeNode extends AbstractTree {
     private TreeNode root;
-    private TreeNode parent;
-    private final List<TreeNode> children = new ArrayList<>();
 
     /**
      * Create a new instance of a {@link AbstractTreeNode} that serves as root for a tree.
@@ -43,44 +38,9 @@ public TreeNode getRoot() {
         return this.root;
     }
 
-    @Override
-    public TreeNode getParent() {
-        return this.parent;
-    }
-
     @Override
     public void addChild(final TreeNode child) {
         this.children.add(child);
         child.setParent(this);
     }
-
-    @Override
-    public List<TreeNode> getChildren() {
-        return this.children;
-    }
-
-    @Override
-    public TreeNode getChild(final int index) {
-        return this.children.get(index);
-    }
-
-    @Override
-    public boolean isRoot() {
-        return (this == getRoot());
-    }
-
-    @Override
-    public boolean isChild() {
-        return (this.parent != null);
-    }
-
-    @Override
-    public boolean isFirstSibling() {
-        return (this.parent != null) && (this.getParent().getChild(0) == this);
-    }
-
-    @Override
-    public boolean isSibling(final TreeNode node) {
-        return (this.parent != null) && (this.getParent().getChildren().contains(node));
-    }
 }
diff --git a/src/test/java/com/exasol/sql/dml/merge/MatchedClauseTest.java b/src/test/java/com/exasol/sql/dml/merge/MatchedClauseTest.java
@@ -7,7 +7,7 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
-public class MatchedClauseTest {
+class MatchedClauseTest {
     private MatchedClause matched;
 
     @BeforeEach
diff --git a/src/test/java/com/exasol/sql/dml/merge/NotMatchedClauseTest.java b/src/test/java/com/exasol/sql/dml/merge/NotMatchedClauseTest.java
@@ -7,7 +7,7 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
-public class NotMatchedClauseTest {
+class NotMatchedClauseTest {
     private NotMatchedClause notMatched;
 
     @BeforeEach
diff --git a/src/test/java/com/exasol/sql/expression/TestBooleanTerm.java b/src/test/java/com/exasol/sql/expression/TestBooleanTerm.java
@@ -50,31 +50,39 @@ void testOperationFromUpperCaseYieldsNot() {
     // [utest->dsn~boolean-operators~1]
     @Test
     void testOperationFromUnknownOperatorThrowsException() {
-        assertThrows(IllegalArgumentException.class, () -> BooleanTerm.operation("illegal", not(true)));
+        final BooleanExpression notTrue = not(true);
+        assertThrows(IllegalArgumentException.class, () -> BooleanTerm.operation("illegal", notTrue));
     }
 
     // [utest->dsn~boolean-operators~1]
     @Test
     void testOperationFromNotWithMoreOrLessThanOneOperandThrowsException() {
-        assertThrows(IllegalArgumentException.class, () -> BooleanTerm.operation("not", not(true), not(false)));
+        final BooleanExpression notTrue = not(true);
+        final BooleanExpression notFalse = not(false);
+        assertThrows(IllegalArgumentException.class, () -> BooleanTerm.operation("not", notTrue, notFalse));
     }
 
     // [utest->dsn~boolean-operators~1]
     @Test
     void testOperationFromUpperCaseUnknownOperatorThrowsException() {
-        assertThrows(IllegalArgumentException.class, () -> BooleanTerm.operation("ILLEGAL", not(true)));
+        final BooleanExpression notTrue = not(true);
+        assertThrows(IllegalArgumentException.class, () -> BooleanTerm.operation("ILLEGAL", notTrue));
     }
 
     // [utest->dsn~boolean-operators~1]
     @Test
     void testOperationFromUpperCaseNotWithMoreOrLessThanOneOperandThrowsException() {
-        assertThrows(IllegalArgumentException.class, () -> BooleanTerm.operation("NOT", not(true), not(false)));
+        final BooleanExpression notTrue = not(true);
+        final BooleanExpression notFalse = not(false);
+        assertThrows(IllegalArgumentException.class, () -> BooleanTerm.operation("NOT", notTrue, notFalse));
     }
 
     // [utest->dsn~boolean-operators~1]
     @Test
     void testOperationFromNullOperatorThrowsException() {
-        assertThrows(NullPointerException.class, () -> BooleanTerm.operation(null, not(true), not(false)));
+        final BooleanExpression notTrue = not(true);
+        final BooleanExpression notFalse = not(false);
+        assertThrows(NullPointerException.class, () -> BooleanTerm.operation(null, notTrue, notFalse));
     }
 
     // [utest->dsn~boolean-operation.comparison.constructing-from-strings~1]
diff --git a/src/test/java/com/exasol/sql/expression/function/exasol/ExasolAggregateFunctionTest.java b/src/test/java/com/exasol/sql/expression/function/exasol/ExasolAggregateFunctionTest.java
@@ -11,7 +11,7 @@
 import com.exasol.sql.dql.select.Select;
 import com.exasol.sql.expression.BooleanTerm;
 
-public class ExasolAggregateFunctionTest {
+class ExasolAggregateFunctionTest {
     @Test
     void testAggregateFunctionCoalesce() {
         final Select select = StatementFactory.getInstance().select() //
diff --git a/src/test/java/com/exasol/sql/expression/function/exasol/ExasolAnalyticFunctionTest.java b/src/test/java/com/exasol/sql/expression/function/exasol/ExasolAnalyticFunctionTest.java
@@ -11,7 +11,7 @@
 import com.exasol.sql.dql.select.Select;
 import com.exasol.sql.expression.BooleanTerm;
 
-public class ExasolAnalyticFunctionTest {
+class ExasolAnalyticFunctionTest {
     @Test
     void testAggregateFunctionCoalesce() {
         final Select select = StatementFactory.getInstance().select() //
diff --git a/src/test/java/com/exasol/util/TestAbstractBottomUpTreeNode.java b/src/test/java/com/exasol/util/TestAbstractBottomUpTreeNode.java
@@ -63,8 +63,9 @@ void testIsFirstSiblingOnSecondChild() {
     }
 
     @Test
-    void testAddingChildAfterConstructurThrowsExpection() {
-        assertThrows(UnsupportedOperationException.class, () -> this.node.addChild(new DummyBottomUpTreeNode()));
+    void testAddingChildAfterConstructorThrowsException() {
+        final DummyBottomUpTreeNode child = new DummyBottomUpTreeNode();
+        assertThrows(UnsupportedOperationException.class, () -> this.node.addChild(child));
     }
 
     @Test
diff --git a/src/test/java/com/exasol/util/TestAbstractTreeNode.java b/src/test/java/com/exasol/util/TestAbstractTreeNode.java
@@ -97,14 +97,13 @@ void testGetParent() {
 
     @Test
     void testSetParentToNullThrowsException() {
-        assertThrows(IllegalArgumentException.class, () -> new DummyTreeNode().setParent(null));
+        final DummyTreeNode dummyTreeNode = new DummyTreeNode();
+        assertThrows(IllegalArgumentException.class, () -> dummyTreeNode.setParent(null));
     }
 
     @Test
     void testSetParentToSelfThrowsException() {
-        assertThrows(IllegalArgumentException.class, () -> {
-            final DummyTreeNode abstractNode = new DummyTreeNode();
-            abstractNode.setParent(abstractNode);
-        });
+        final DummyTreeNode abstractNode = new DummyTreeNode();
+        assertThrows(IllegalArgumentException.class, () -> abstractNode.setParent(abstractNode));
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -61,11 +61,10 @@ protected void appendAutoQuoted(final String identifier) {`
`61`	`61`	`}`
`62`	`62`
`63`	`63`	`protected void appendCommaWhenNeeded(final ValueExpression valueExpression) {`
`64`		`- if (this.lastVisited != null && !(valueExpression.getParent() instanceof BinaryArithmeticExpression)) {`
`65`		`- if (this.lastVisited.isSibling(valueExpression) \|\| (valueExpression.getParent() != this.lastVisited`
`66`		`- && this.lastVisited.getClass().equals(valueExpression.getClass()))) {`
`67`		`- append(", ");`
`68`		`- }`
	`64`	`+ if ((this.lastVisited != null && !(valueExpression.getParent() instanceof BinaryArithmeticExpression))`
	`65`	`+ && (this.lastVisited.isSibling(valueExpression) \|\| (valueExpression.getParent() != this.lastVisited`
	`66`	`+ && this.lastVisited.getClass().equals(valueExpression.getClass())))) {`
	`67`	`+ append(", ");`
`69`	`68`	`}`
`70`	`69`	`}`
`71`	`70`