You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/09/07 10:27:34 UTC
[GitHub] [commons-numbers] aherbert commented on a diff in pull request #123: NUMBERS-186 added additional complex list operations
aherbert commented on code in PR #123:
URL: https://github.com/apache/commons-numbers/pull/123#discussion_r964653374
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -348,4 +433,25 @@ public void replaceAll(ComplexUnaryOperator<Void> operator) {
}
modCount++;
}
+
+ /**
+ * Replaces each element of the list with the result of applying the action to that element.
Review Comment:
This javadoc is wrong. Look at the javadoc for `forEach(Consumer<Complex>)` and adapt as appropriate.
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
Review Comment:
Please stop creating larger lists using duplication. The list elements should be unique. Create the list using random complex numbers or a sequence.
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -149,6 +151,30 @@ public Complex get(int index) {
return Complex.ofCartesian(realAndImagParts[i], realAndImagParts[i + 1]);
}
+ /**
+ * Gets the real part \( a \) of the complex number \( (a + i b) \) at the indexed position of the list.
+ *
+ * @param index Index of the element's real part to get.
Review Comment:
`Index of the complex number.`
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+ for (int i = 0; i < list.size(); i++) {
+ Assertions.assertEquals(list.get(i).getReal(), list.getReal(i));
+ }
+ for (int i = 0; i < list.size(); i++) {
+ Assertions.assertEquals(list.get(i).getImaginary(), list.getImaginary(i));
+ }
+ }
+
+ @Test
+ void testSetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.setReal(2, 200);
+ list.setImaginary(2, 1);
+ Assertions.assertEquals(Complex.ofCartesian(200, 1), list.get(2));
Review Comment:
Test set at all positions:
```Java
for (int i = 0; i < list.size(); i++) {
final double value = Math.PI * i;
list.setReal(i, value);
Assertions.assertEquals(value, list.get(i).getReal());
}
```
Also test set/get with invalid index positions (-1, size, size+1).
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -177,6 +203,63 @@ public Complex set(int index, Complex element) {
return oldValue;
}
+ /**
+ * Replaces the complex element's real part at the specified position
Review Comment:
change `element` to `number`. Element is for a generic list; since this is typed to Complex then all element occurrences should be changed to `complex number`
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+ for (int i = 0; i < list.size(); i++) {
+ Assertions.assertEquals(list.get(i).getReal(), list.getReal(i));
+ }
+ for (int i = 0; i < list.size(); i++) {
+ Assertions.assertEquals(list.get(i).getImaginary(), list.getImaginary(i));
+ }
+ }
+
+ @Test
+ void testSetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.setReal(2, 200);
+ list.setImaginary(2, 1);
+ Assertions.assertEquals(Complex.ofCartesian(200, 1), list.get(2));
+ }
+
+ @Test
+ void testToArrayRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+
+ double[] expectedReal = {42, 42, 200, 42};
+ double[] actualReal = list.toArrayReal();
+ for (int i = 0; i < expectedReal.length; i++) {
Review Comment:
Use `Assertions.assertArrayEquals`
##########
commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexConsumer.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.numbers.complex;
+
+/**
+ * Represents an operation that accepts two double input arguments and returns no result.
+ *
+ * <p>This is a functional interface whose functional method is
+ * {@link #apply(double, double)}.
+ *
+ * @since 1.1
+ */
+@FunctionalInterface
+public interface ComplexConsumer {
+
+ /**
+ * Represents a function that accepts real and imaginary part of complex number and returns no result.
+ * @param real Real part \( a \) of the complex number \( (a + ib) \).
+ * @param imaginary Imaginary part \( b \) of the complex number \( (a + ib) \).
+ */
+ void apply(double real, double imaginary);
Review Comment:
java.util.function.Consumer has the method `accept`
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+ for (int i = 0; i < list.size(); i++) {
+ Assertions.assertEquals(list.get(i).getReal(), list.getReal(i));
+ }
+ for (int i = 0; i < list.size(); i++) {
+ Assertions.assertEquals(list.get(i).getImaginary(), list.getImaginary(i));
Review Comment:
This can be placed in the same loop as above. Add a simple message to the assertion, e.g. "real".
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+ for (int i = 0; i < list.size(); i++) {
+ Assertions.assertEquals(list.get(i).getReal(), list.getReal(i));
+ }
+ for (int i = 0; i < list.size(); i++) {
+ Assertions.assertEquals(list.get(i).getImaginary(), list.getImaginary(i));
+ }
+ }
+
+ @Test
+ void testSetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.setReal(2, 200);
+ list.setImaginary(2, 1);
+ Assertions.assertEquals(Complex.ofCartesian(200, 1), list.get(2));
+ }
+
+ @Test
+ void testToArrayRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
Review Comment:
Create a better random list.
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -348,4 +433,25 @@ public void replaceAll(ComplexUnaryOperator<Void> operator) {
}
modCount++;
}
+
+ /**
+ * Replaces each element of the list with the result of applying the action to that element.
+ *
+ * @param action The action to apply to each element.
+ */
+ public void forEach(ComplexConsumer action) {
+ Objects.requireNonNull(action);
+ final double[] parts = this.realAndImagParts;
+ final int m = size;
+ final int expectedModCount = modCount;
Review Comment:
The mod count is increased when the list is structurally modified. Here no modifications are occurring so this can be removed.
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -177,6 +203,63 @@ public Complex set(int index, Complex element) {
return oldValue;
}
+ /**
+ * Replaces the complex element's real part at the specified position
+ * in this list with the specified real element.
+ *
+ * @param index Index of the element's real part to replace.
+ * @param real Real part \( a \) of the complex number \( (a +ib) \).
+ */
+ public void setReal(int index, double real) {
+ rangeCheck(index);
+ final int i = index << 1;
+ realAndImagParts[i] = real;
+ }
+
+ /**
+ * Replaces the complex element's imaginary part at the specified position
+ * in this list with the specified imaginary element.
+ * @param index Index of the element's imaginary part to replace.
+ * @param imaginary Imaginary part \( b \) of the complex number \( (a +ib) \).
+ */
+ public void setImaginary(int index, double imaginary) {
+ rangeCheck(index);
+ final int i = index << 1;
+ realAndImagParts[i + 1] = imaginary;
+ }
+
+ /**
+ * Returns an array containing all the complex element's real parts in
+ * the list in proper sequence (from first to last element).
+ *
+ * @return Array of real parts.
+ */
+ double[] toArrayReal() {
+ final int length = size;
+ double[] real = new double[length];
+ for (int i = 0; i < length; i++) {
+ final int index = i << 1;
Review Comment:
Again this `index` can be inlined
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -177,6 +203,63 @@ public Complex set(int index, Complex element) {
return oldValue;
}
+ /**
+ * Replaces the complex element's real part at the specified position
+ * in this list with the specified real element.
+ *
+ * @param index Index of the element's real part to replace.
+ * @param real Real part \( a \) of the complex number \( (a +ib) \).
+ */
+ public void setReal(int index, double real) {
+ rangeCheck(index);
+ final int i = index << 1;
+ realAndImagParts[i] = real;
+ }
+
+ /**
+ * Replaces the complex element's imaginary part at the specified position
+ * in this list with the specified imaginary element.
+ * @param index Index of the element's imaginary part to replace.
Review Comment:
Add an empty line
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+ for (int i = 0; i < list.size(); i++) {
+ Assertions.assertEquals(list.get(i).getReal(), list.getReal(i));
+ }
+ for (int i = 0; i < list.size(); i++) {
+ Assertions.assertEquals(list.get(i).getImaginary(), list.getImaginary(i));
+ }
+ }
+
+ @Test
+ void testSetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.setReal(2, 200);
+ list.setImaginary(2, 1);
+ Assertions.assertEquals(Complex.ofCartesian(200, 1), list.get(2));
+ }
+
+ @Test
+ void testToArrayRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+
+ double[] expectedReal = {42, 42, 200, 42};
Review Comment:
double[] expected = list.stream().mapToDouble(Complex::getReal).toArray();
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -149,6 +151,30 @@ public Complex get(int index) {
return Complex.ofCartesian(realAndImagParts[i], realAndImagParts[i + 1]);
}
+ /**
+ * Gets the real part \( a \) of the complex number \( (a + i b) \) at the indexed position of the list.
+ *
+ * @param index Index of the element's real part to get.
+ * @return The real part.
+ */
+ public double getReal(int index) {
+ rangeCheck(index);
+ final int i = index << 1;
Review Comment:
No need for this variable, just perform the index doubling inline
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
Review Comment:
`testForEach...`
This test can be parameterized for size. This will eliminate duplication:
```Java
@ParameterizedTest
@ValuesSource(ints = {0, 10})
void testForEachComplexConsumer(int size) {
List<Complex> data1 = generateList(size);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org