Numbers 186.complex list interleaved #124
Conversation
| */ | ||
| public class ComplexList extends AbstractList<Complex> { | ||
| public abstract class ComplexList extends AbstractList<Complex> { |
There was a problem hiding this comment.
Prevent extension of this class outside the inner classes by adding a private constructor. It may have to be package-private to pass the build checks (e.g. PMD).
| @@ -28,23 +28,9 @@ | |||
| import java.util.Objects; | |||
|
|
|||
| /** | |||
| * Resizable-double array implementation of the List interface. Implements all optional list operations, | |||
There was a problem hiding this comment.
Some of this javadoc still applies. The list provides efficient storage of complex numbers. It does not accept null complex objects.
| * @throws IllegalArgumentException if the specified double array doesn't have an even amount of doubles. | ||
| */ | ||
| ComplexInterleavedList(double[] fromArray) { | ||
| if (fromArray.length % 2 != 0) { |
There was a problem hiding this comment.
if ((fromArray.length & 1) != 0)
| @@ -37,6 +37,17 @@ public class ComplexListTest { | |||
|
|
|||
| private static final int MAX_CAPACITY = (Integer.MAX_VALUE - 9) / 2; | |||
|
|
|||
| @Test | |||
| void testFromArray() { | |||
| double[] fromArray1 = ThreadLocalRandom.current().doubles(6, -Math.PI, Math.PI).toArray(); | |||
There was a problem hiding this comment.
int size = 3;
... doubles(size * 2, ...
for (... ; i < size; ...
| Assertions.assertThrows(NullPointerException.class, () -> list1.set(1, null)); | ||
|
|
||
| List<Complex> list2 = generateList(3); | ||
| for (int i = 0; i < list2.size(); i++) { |
There was a problem hiding this comment.
This loop is setting all elements to null. Just set one of them:
list2.set(1, null)
| list2.set(i, null); | ||
| } | ||
| } | ||
| Assertions.assertThrows(NullPointerException.class, () -> list1.addAll(list2)); |
There was a problem hiding this comment.
Check that list1 has the same size after the NPE is thrown, i.e. it is unchanged.
You could create a copy of the list for reference and use assertEquals to compare the before and after.
Codecov Report
@@ Coverage Diff @@
## complex-gsoc-2022 #124 +/- ##
====================================================
Coverage 99.13% 99.13%
Complexity 1699 1699
====================================================
Files 65 65
Lines 4274 4274
Branches 836 836
====================================================
Hits 4237 4237
Misses 10 10
Partials 27 27 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -311,7 +324,7 @@ private static class ComplexInterleavedList extends ComplexList { | |||
| * @throws IllegalArgumentException if the specified double array doesn't have an even amount of doubles. | |||
| */ | |||
| ComplexInterleavedList(double[] fromArray) { | |||
| if (fromArray.length % 2 != 0) { | |||
| if ((fromArray.length & 1) != 0) { | |||
| throw new IllegalArgumentException("Length of array has to be even"); | |||
| } | |||
| realAndImagParts = fromArray; | |||
There was a problem hiding this comment.
Here the data is not defensively cloned. This is fine but the behaviour should be documented. Any public method should indicate that the argument array is used in-place; any external modifications to the array will be reflected in this List until a structural modification is made to the data storage, for example a resize.
| ComplexList list = ComplexList.from(fromArray1); | ||
| for (int i = 0; i < fromArray1.length >> 1; i++) { | ||
| for (int i = 0; i < size >> 1; i++) { |
There was a problem hiding this comment.
This should iterate over size, not size >> 1
| Assertions.assertThrows(NullPointerException.class, () -> list1.addAll(list2)); | ||
| Assertions.assertEquals(copy.size(), list1.size()); |
There was a problem hiding this comment.
You should also check addAll(int, Collection). The implementation, if done incorrectly, could move all elements to make space, then copy the new elements in and throw a NPE. This will leave the list in a broken state.
It may be better to check the contents of the list are the same using Assertions.assertEquals(copy, list1);
| * Constructor to prevent extension of this class outside inner clases. | ||
| */ | ||
| private ComplexList() { | ||
|
|
There was a problem hiding this comment.
Comment empty line: // Intentionally empty
| @@ -130,14 +131,12 @@ void testSetAddAndAddAllNullPointerException() { | |||
| Assertions.assertThrows(NullPointerException.class, () -> list1.add(0, null)); | |||
| Assertions.assertThrows(NullPointerException.class, () -> list1.set(1, null)); | |||
|
|
|||
| ComplexList copy = ComplexList.interleaved(); | |||
There was a problem hiding this comment.
Create this copy at the start of the method. When used at the end of the method this will also check the add methods do not result in modifications of the list.
| Assertions.assertThrows(NullPointerException.class, () -> list1.addAll(0, list2)); | ||
| copy.addAll(list1); |
There was a problem hiding this comment.
copy.addAll(list1) should be before all the operations on list1. Otherwise you are not comparing before (the copy) and after (list1).
| @@ -60,7 +60,7 @@ public abstract class ComplexList extends AbstractList<Complex> { | |||
| * Constructor to prevent extension of this class outside inner clases. | |||
| */ | |||
| private ComplexList() { | |||
|
|
|||
| //Intentional empty | |||
This PR refactors ComplexList to be abstract and gives a interleaved implementation as a private sub-class. This allows for multiple implementations of data storage.
Summary of changes:
ComplexList - made abstract with factory methods implemented in interleaved implementation.
ComplexInterleavedList - private sub-class with interleaved double array storage.
ComplexListTest - updates to calling ComplexList using ComplexInterleavedList.