Merge branch 'master' into branch-coverage-#6

Author: mantaur

Diff for: report.md

@@ -144,14 +144,65 @@ It is stated that `replacementNode` can be null and that `nodeToRemoved` can't b
 Could be stated in the documentation that if the root of the BST is null the function simply returns an empty array. Shouldn't have to browse through the code to find this fact. Although it is stated that the input `order` determines how the array is sorted, this could be explained in more detail.
 
 
+#### @Kubha99
+| Function                                   | nLOC | `lizard` CCN | Manual CCN |
+| --------                                   | ---: | ---------: | ---------: |
+| `RedBlackTree.balanceAfterDelete`           | 72   | 22         | 21         |
+| `RedBlackTree.balanceAfterInsert`          | 46   | 18         | 15         |
 
+##### 1.
 
+Like @psalqvist, I set CCN=2 and for each `if`, `for`, `while`, `else if`, `&&` and `||` (not for `else`) i added 1 and for each return i subtracted with 1. There was a slight difference between the results computed from manual count and the counting from lizard tool.
 
+##### 2. 
+There does exist a similarity between the length of the function and the complexity for it. 
 
-#### @Kubha99
+##### 3. 
+###### balanceAfterInsert
+Has input of `RedBlackNode` as input and balances the given tree, this function is called once we insert a node in the tree
+
+###### balanceAfterDelete
+Similar to `balanceAfterInsert` this function takes a `RedBlackNode` and balances the tree after a delete
+
+##### 4. 
+In `balanceAfterDelete` there exists a case where an execption needs to be handled. 
+
+##### 5. 
+Both of the functions are well documented and there exists line comments explaining what operations are done and sometimes why. 
+There also exists documentation for input as well as output.
 
 #### @ekorre1001
 
+| Function                                   | nLOC | `lizard` CCN | Manual CCN |
+| --------                                   | ---: | ---------: | ---------: |
+| `BTree.validateNode`                       | 59   | 22         | 6          |
+| `Multiplication.multiplyUsingFFT`          | 68   | 21         | 18         |
+
+##### 1.
+
+Like @psalqvist, we start with CCN = 2 then add 1 for `if`, `for`, `while`, `else if`, `&&` and `||` (not for `else`), and subtract 1 to CCN when reaching a `return` statement. The result shows a lower manual CCN. It is because that in both functions there are more than 1 return point, especially in the `BTree.validateNode`, almost every `if` statement is followed by a `return` code. 
+
+##### 2.
+Looking at the CCN result from lizard it seems like NLOC correlates with CCN. The manual count for `Multiplication.multiplyUsingLogs` also indicates that.
+
+##### 3.
+###### validateNode
+Takes a `node` object as input and validates the node according to the B-Tree invariants. Returns `True` if valid, else `false`.
+
+###### multiplyUsingFFT
+Takes two `string` objects as input and extract the numbers, then multiply the two numbers using Fast Fourier transform method. Returns the result as a `string`.
+
+##### 4.
+There are no exceptions in either of the functions.
+
+##### 5.
+###### validateNode
+This function is well documented since there are many comments that explain what some part of the function does. It is also clear what the input and output are.
+
+###### multiplyUsingFFT
+There are no comments in the code which makes it harder for the reader to know what each part does. However, if you are familiar with the FFT method it can be intuitive. 
+
+
 
 
 ## Refactoring
@@ -164,6 +215,121 @@ Carried out refactoring (optional, P+):
 
 git diff ...
 
+
+### @nolanderc: `BinaryHeapArray.heapDown`
+
+The main goal of refactoring should be to reduce the amount of duplication in the `if` statements. For example, there is one that looks like this:
+```java
+if ((type == Type.MIN && left != null && right != null && value.compareTo(left) > 0 && value.compareTo(right) > 0)
+ || (type == Type.MAX && left != null && right != null && value.compareTo(left) < 0 && value.compareTo(right) < 0) {
+    ...
+}
+```
+Here the checks for `left != null` and `right != null` are duplicated twice each. Also the comparisons against `value` are repeated twice, but with different operators (`<` and `>`). This pattern is repeated an additional 2-4 times, depending on how you count. 
+
+We can reduce the cyclomatic complexity of this code by restructuring the code so that each check for null only happens once, and which comparison to do against the value is determined by the `type` only a single time. Applying these changes results in something the following:
+
+```java
+// determine the order we want nodes in once
+int desiredOrder = (type == Type.MIN) ? -1 : 1;
+int undesiredOrder = -desiredOrder;
+
+// Do checks against null and perform comparisons against the parent value.
+// If their order does not match the desired, we need to swap them.
+boolean leftShouldSwap = left != null && Integer.signum(value.compareTo(left)) == undesiredOrder;
+boolean rightShouldSwap = right != null && Integer.signum(value.compareTo(right)) == undesiredOrder;
+
+// handle different cases depending on which child needs to swap with the parent
+if (leftShouldSwap & rightShouldSwap) {
+    // if both need to swap, make sure that swapping preserves the desired order
+    return (Integer.signum(left.compareTo(right)) == desiredOrder) ? leftIndex : rightIndex;
+} else if (leftShouldSwap) {
+    return leftIndex;
+} else if (rightShouldSwap) {
+    return rightIndex;
+} else {
+    return -1;
+}
+```
+
+This is essentially everything the old 46 nLOC function did, but with multiple levels of nested `if` statements and convuluted logic. In the end, the new version uses two functions with a cyclomatic complexity of 2 and 10, respectively. Compare this to the old version which had a cyclomatic complexity of 41.
+
+For a full diff, run the following:
+```sh
+git show 9b4599289de7c734e4cd7364ce8535fc7d32be90
+```
+
+### @ekorre1001: `Multiplication.multiplyUsingFFT`
+
+To improve the cyclomatic complexity we want to either remove or reduce the use of `if`, `for`, `while`, `else if`, `&&` and `||`. In the multiplyUsingFFT we can find a lot of if statements used to investigate both of the input numbers since they are strings. For instance the following is used to check whether the product is negative.
+
+```java
+if ((a.charAt(0) == '-' && b.charAt(0) != '-') || (a.charAt(0) != '-' && b.charAt(0) == '-')) {
+      negative = true;
+}
+```
+This can be improved by converting both of the strings to integers and check if the product is negative or not.
+
+```java
+int x = Integer.parseInt(a);
+int y = Integer.parseInt(b);
+if(x*y < 0) negative = true;
+```
+By doing this we have reduced the cyclomatic complexity but the drawback is that we initiate new variables and we are dependent on another library.
+
+Lastly, we can remove the use of some commonly used operations by promoting them to a function. For instance, since both strings are edited to remove the minus symbol (the following code), we can reduce the complexity by adding a helper function that gets called for each string. The drawback is of course those additional functions.
+```java
+if (a.charAt(0) == '-') {
+    a = a.substring(1);
+}
+if (b.charAt(0) == '-') {
+    b = b.substring(1);
+}
+```
+
+In the end, we managed to reduce the CCN from 21 to 13.
+
+Git diff: Check the refactor section [here](https://docs.google.com/document/d/1qRhKoisnicSaKS3oRQEs6EaFpCoqO1QLV4kNYcLeAFo/edit?usp=sharing).
+
+### `BTree.validateNode`
+
+As with the previous function we aim to reduce the use of `if`, `for`, `while`, `else if`, `&&` and `||`. In this case a lot of if statements were used to check a specific condition and directly followed by a `return`. For example the following:
+
+```java
+if (keySize < minKeySize) {
+    return false;
+} else if (keySize > maxKeySize) {
+    return false;
+} else if (childrenSize == 0) {
+    return true;
+} else if (keySize != (childrenSize - 1)) {
+    return false;
+} else if (childrenSize < minChildrenSize) {
+    return false;
+} else if (childrenSize > maxChildrenSize) {
+    return false;
+}
+```
+
+This can be reduced by creating a helper function that does the same thing and then we just need to check the value returned by the helper function. The code above can be replaced with the following:
+
+```java
+// make the check in another function and save the result
+int checkNonRoot = validateNonRootHelper(keySize, childrenSize);
+// return the corresponding boolean
+if (checkNonRoot == 0) {
+    return false;
+} else if (checkNonRoot == 1) {
+    return true;
+}
+```
+The drawback is that we need to add a few additional functions and variables.
+
+In the end, we managed to reduce the CCN from 22 to 14.
+
+Git diff: Check the refactor section [here](https://docs.google.com/document/d/1qRhKoisnicSaKS3oRQEs6EaFpCoqO1QLV4kNYcLeAFo/edit?usp=sharing).
+
+
 ## Coverage
 
 ### Tools
@@ -173,6 +339,12 @@ Document your experience in using a "new"/different coverage tool.
 How well was the tool documented? Was it possible/easy/difficult to
 integrate it with your build environment?
 
+### NOTES
+
+#### @ekorre1001
+
+The code coverage tool I am using is OpenClover which works with the Ant build tool. I followed the provided quick start guide and managed to integrate it with Ant. It was quite easy to do the setup since only a few steps were required, although a few things did not work out initially.
+
 ### Your own coverage tool
 
 Show a patch (or link to a branch) that shows the instrumented code to
@@ -186,6 +358,20 @@ git diff ...
 What kinds of constructs does your tool support, and how accurate is
 its output?
 
+#### @nolanderc
+
+See `git show 9ef482092816b3367f4d4b45214821ee11019fe3`
+
+It supports `if` statements, `while` loops and `for` loops. The output should
+show exactly how many times a code block was executed, and also notify if a code
+block has not been executed. Example:
+
+```
+BranchCoverageTest.branchCoverageWithNotReached:
+  - entry point: 1
+  - not reached: 0 <-- NOT REACHED
+```
+
 ### Evaluation
 
 1. How detailed is your coverage measurement?
@@ -208,6 +394,49 @@ git diff ...
 
 Number of test cases added: two per team member (P) or at least four (P+).
 
+### NOTES
+
+
+#### @ekorre1001
+
+Changed to another function because:
+
+BTree::validateNode: Barely any test for the data structure and no test for this specific function which makes it harder to understand.
+
+Multiplication::multiplyUsingFFT: Above 98% code coverage
+
+The requirements + report can be found [here](https://docs.google.com/document/d/1qRhKoisnicSaKS3oRQEs6EaFpCoqO1QLV4kNYcLeAFo/edit?usp=sharing)
+
+5 Test cases added and can be found in the PrimeTest.java
+
+
+#### @nolanderc
+
+`BinaryHeapArray.heapDown` before: 94.1%, after: 100.0%.
+
+Branch coverage report: https://docs.google.com/document/d/1IVLQTIkl8IiemVskQ_JFw1sNe5EBXXEWpTxYIQ4-NV0/edit?usp=sharing
+
+For new test cases, see:
+
+```sh
+git show 395749c08a93e8b8a063e87849e44543fe1189ac
+```
+
+Since the `heapDown` function already had such high branch coverage I also wrote
+tests for `BinaryHeapArray.validateNode`:
+
+`BinaryHeapArray.validateNode` before: 57.7%, after: 100.0%.
+
+```sh
+git show 8e6d8c7d8d36417133fe66f2c8d80a153d23b3f9 
+```
+
+While writing these tests, I also found a bug in the source code. The fix can be seen here: 
+
+```sh
+git show 12d42662a86bc770915c15f5d2d24d877b107c06
+```
+
 ## Self-assessment: Way of working
 
 Current state according to the Essence standard: ...

Diff for: src/com/jwetherell/algorithms/data_structures/BinaryHeap.java

@@ -106,6 +106,16 @@ public BinaryHeapArray(Type type) {
             this.type = type;
         }
 
+        /**
+         * Create a heap from an already "heapified" array.
+         * If this is not the case (ie. the array is in arbitrary order, behaviour is undefined).
+         * @param items The heapified array
+         */
+        public void setArrayUnsafe(T[] items) {
+            this.size = items.length;
+            this.array = Arrays.copyOf(items, Integer.max(items.length, MINIMUM_SIZE));
+        }
+
         /**
          * {@inheritDoc}
          */
@@ -305,17 +315,19 @@ private boolean validateNode(int index) {
                 T left = this.array[leftIndex];
                 if ((type == Type.MIN && value.compareTo(left) < 0) 
                     || (type == Type.MAX && value.compareTo(left) > 0)) {
-                    return validateNode(leftIndex);
+                    if (!validateNode(leftIndex)) return false;
+                } else {
+                    return false;
                 }
-                return false;
             }
             if (rightIndex != Integer.MIN_VALUE && rightIndex < size) {
                 T right = this.array[rightIndex];
                 if ((type == Type.MIN && value.compareTo(right) < 0)
                     || (type == Type.MAX && value.compareTo(right) > 0)) {
-                    return validateNode(rightIndex);
+                    if (!validateNode(rightIndex)) return false;
+                } else {
+                    return false;
                 }
-                return false;
             }
 
             return true;

Diff for: test/com/jwetherell/algorithms/data_structures/test/BinaryHeapTests.java

@@ -1,6 +1,8 @@
 package com.jwetherell.algorithms.data_structures.test;
 
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 
 import java.util.Collection;
@@ -81,4 +83,62 @@ public void testMaxHeap() {
         tHeapNull.clear();
         assertNull(tHeapNull.getHeadValue()); // we expect null here
     }
+
+    // Exercices case in `BinaryHeapArray.heapDown` when the parent is less than
+    // both its children, but the children are equal
+    @Test
+    public void testHeapDownChildrenLessButEqualMax() {
+        BinaryHeap.BinaryHeapArray<Integer> aMaxHeap = new BinaryHeap.BinaryHeapArray<Integer>(BinaryHeap.Type.MAX);
+        aMaxHeap.add(7);
+        aMaxHeap.add(3);
+        aMaxHeap.add(3);
+        aMaxHeap.add(2);
+        assertEquals(7, (int)aMaxHeap.removeHead());
+        assertEquals(3, (int)aMaxHeap.removeHead());
+        assertEquals(3, (int)aMaxHeap.removeHead());
+        assertEquals(2, (int)aMaxHeap.removeHead());
+        assertNull(aMaxHeap.removeHead());
+    }
+
+    // Exercices case in `BinaryHeapArray.heapDown` when the parent is greater than
+    // both its children, but the children are equal
+    @Test
+    public void testHeapDownChildrenLessButEqualMin() {
+        BinaryHeap.BinaryHeapArray<Integer> aMinHeap = new BinaryHeap.BinaryHeapArray<Integer>(BinaryHeap.Type.MIN);
+        aMinHeap.add(2);
+        aMinHeap.add(3);
+        aMinHeap.add(3);
+        aMinHeap.add(7);
+        assertEquals(2, (int)aMinHeap.removeHead());
+        assertEquals(3, (int)aMinHeap.removeHead());
+        assertEquals(3, (int)aMinHeap.removeHead());
+        assertEquals(7, (int)aMinHeap.removeHead());
+        assertNull(aMinHeap.removeHead());
+    }
+
+    @Test
+    public void testValidateNodeValid() {
+        BinaryHeap.BinaryHeapArray<Integer> heap = new BinaryHeap.BinaryHeapArray<Integer>(BinaryHeap.Type.MAX);
+        heap.setArrayUnsafe(new Integer[]{ 3, 2, 1 });
+        assertTrue(heap.validate());
+    }
+
+    // Improves branch coverage in `validateNode` from 57.7% to 65.4%
+    @Test
+    public void testValidateNodeMaxInvalidLeft() {
+        BinaryHeap.BinaryHeapArray<Integer> heap = new BinaryHeap.BinaryHeapArray<Integer>(BinaryHeap.Type.MAX);
+
+        // An invalid heap (wrong ordering) should be flagged as invalid
+        heap.setArrayUnsafe(new Integer[]{ 3, 4, 1 });
+        assertFalse(heap.validate());
+    }
+
+    // Improves branch coverage in `validateNode` from 65.4% to 81.2%
+    @Test
+    public void testValidateNodeMaxInvalidRight() {
+        BinaryHeap.BinaryHeapArray<Integer> heap = new BinaryHeap.BinaryHeapArray<Integer>(BinaryHeap.Type.MAX);
+        // Check that invalid order is checked for right child as well
+        heap.setArrayUnsafe(new Integer[]{ 3, 2, 4 });
+        assertFalse(heap.validate());
+    }
 }