From 49857b624aa8446a0827cb6ea9239eaf8e2e0f27 Mon Sep 17 00:00:00 2001 From: pingpingy1 Date: Thu, 22 Feb 2024 15:52:19 +0900 Subject: [PATCH 1/2] Set to default strategies if null is given In the current implementation, it is possible to set the `nanStrategy` and `tiesStrategy` of `NaturalRanking` to `null`, even though they will inevitably throw NPEs upon any calls to `rank` or `resolveTie`. This commit modifies the constructor such that if `null` is given to either of the strategies, then they are set to the default strategies. --- .../math4/legacy/stat/ranking/NaturalRanking.java | 4 ++-- .../math4/legacy/stat/ranking/NaturalRankingTest.java | 11 +++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRanking.java b/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRanking.java index 978c9e134c..f8adaea155 100644 --- a/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRanking.java +++ b/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRanking.java @@ -158,8 +158,8 @@ public NaturalRanking(NaNStrategy nanStrategy, private NaturalRanking(NaNStrategy nanStrategy, TiesStrategy tiesStrategy, UniformRandomProvider random) { - this.nanStrategy = nanStrategy; - this.tiesStrategy = tiesStrategy; + this.nanStrategy = nanStrategy != null ? nanStrategy : DEFAULT_NAN_STRATEGY; + this.tiesStrategy = tiesStrategy != null ? tiesStrategy : DEFAULT_TIES_STRATEGY; this.random = random; } diff --git a/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRankingTest.java b/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRankingTest.java index 5f4c512844..10c816a8a6 100644 --- a/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRankingTest.java +++ b/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRankingTest.java @@ -279,4 +279,15 @@ public void testNoNaNsFailed() { double[] ranks = ranking.rank(data); TestUtils.assertEquals(data, ranks, 0d); } + + /** + * Tests NaturalRanking constructor with null strategies as inputs. + * These should be switched to the default strategies. + */ + @Test + public void testNullStrategies() { + NaturalRanking ranking = new NaturalRanking((NaNStrategy) null, (TiesStrategy) null); + Assert.assertEquals(ranking.getNanStrategy(), NaturalRanking.DEFAULT_NAN_STRATEGY); + Assert.assertEquals(ranking.getTiesStrategy(), NaturalRanking.DEFAULT_TIES_STRATEGY); + } } From 4e88264c6ad71839f2550065869702f0b5c7e9b4 Mon Sep 17 00:00:00 2001 From: pingpingy1 Date: Thu, 22 Feb 2024 17:53:25 +0900 Subject: [PATCH 2/2] Apply suggestions from code review As suggested by @aherbert, replaces the default strategies with fail-fast `null` guards. --- .../math4/legacy/stat/ranking/NaturalRanking.java | 5 +++-- .../math4/legacy/stat/ranking/NaturalRankingTest.java | 9 ++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRanking.java b/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRanking.java index f8adaea155..6f5f5e60b2 100644 --- a/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRanking.java +++ b/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRanking.java @@ -21,6 +21,7 @@ import java.util.Arrays; import java.util.Iterator; import java.util.List; +import java.util.Objects; import org.apache.commons.rng.UniformRandomProvider; import org.apache.commons.rng.simple.RandomSource; @@ -158,8 +159,8 @@ public NaturalRanking(NaNStrategy nanStrategy, private NaturalRanking(NaNStrategy nanStrategy, TiesStrategy tiesStrategy, UniformRandomProvider random) { - this.nanStrategy = nanStrategy != null ? nanStrategy : DEFAULT_NAN_STRATEGY; - this.tiesStrategy = tiesStrategy != null ? tiesStrategy : DEFAULT_TIES_STRATEGY; + this.nanStrategy = Objects.requireNonNull(nanStrategy, "nanStrategy"); + this.tiesStrategy = Objects.requireNonNull(tiesStrategy, "tiesStrategy"); this.random = random; } diff --git a/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRankingTest.java b/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRankingTest.java index 10c816a8a6..e53cc433c1 100644 --- a/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRankingTest.java +++ b/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/ranking/NaturalRankingTest.java @@ -286,8 +286,11 @@ public void testNoNaNsFailed() { */ @Test public void testNullStrategies() { - NaturalRanking ranking = new NaturalRanking((NaNStrategy) null, (TiesStrategy) null); - Assert.assertEquals(ranking.getNanStrategy(), NaturalRanking.DEFAULT_NAN_STRATEGY); - Assert.assertEquals(ranking.getTiesStrategy(), NaturalRanking.DEFAULT_TIES_STRATEGY); + Assert.assertThrows(NullPointerException.class, () -> { + new NaturalRanking((NaNStrategy) null, (TiesStrategy) null); + }); + Assert.assertThrows(NullPointerException.class, () -> { + new NaturalRanking(NaturalRanking.DEFAULT_NAN_STRATEGY, (TiesStrategy) null); + }); } }