From eca58bcc6a5ab24f0f44c3f151f1693fef2f8bc4 Mon Sep 17 00:00:00 2001 From: Vest Date: Fri, 26 Jun 2026 12:32:30 +0200 Subject: [PATCH 1/2] TypeSafeConstant siblings: use Map.computeIfAbsent in getConstant Collapses the three get/null-check/put bodies in Type.getConstant, Region.getConstant, and DisplayLocation.getConstant into a single computeIfAbsent call. Behaviour-identical and one map traversal instead of two. SonarLint java:S3824. The lambda captures `name` from the enclosing scope rather than the map-key parameter `k`, because CaseInsensitiveMap.computeIfAbsent passes a CaseInsensitiveString wrapper (via resolveObject) rather than the original String; a `Type::new` method reference would not satisfy the Function signature. The behaviour is identical to the prior code since CaseInsensitiveString.toString() returns the original string anyway. Does NOT touch the sibling "lookup-or-throw" methods (Region.valueOf, DisplayLocation.valueOf) - those have the intentionally-different shape `get(name); if null throw`. Verified: 12241 tests across pcgen.cdom.enumeration.*, pcgen.cdom.facet.*, pcgen.core.*, plugin.lsttokens.* pass with 0 failures. SpotBugs total unchanged at 77 (this is not a SpotBugs PR; the check is for regression sanity). --- code/src/java/pcgen/cdom/enumeration/DisplayLocation.java | 8 +------- code/src/java/pcgen/cdom/enumeration/Region.java | 8 +------- code/src/java/pcgen/cdom/enumeration/Type.java | 8 +------- 3 files changed, 3 insertions(+), 21 deletions(-) diff --git a/code/src/java/pcgen/cdom/enumeration/DisplayLocation.java b/code/src/java/pcgen/cdom/enumeration/DisplayLocation.java index eebd4d886d3..654ccb5b520 100644 --- a/code/src/java/pcgen/cdom/enumeration/DisplayLocation.java +++ b/code/src/java/pcgen/cdom/enumeration/DisplayLocation.java @@ -101,13 +101,7 @@ public String getComparisonString() */ public static DisplayLocation getConstant(String name) { - DisplayLocation type = typeMap.get(name); - if (type == null) - { - type = new DisplayLocation(name); - typeMap.put(name, type); - } - return type; + return typeMap.computeIfAbsent(name, k -> new DisplayLocation(name)); } /** diff --git a/code/src/java/pcgen/cdom/enumeration/Region.java b/code/src/java/pcgen/cdom/enumeration/Region.java index 8995713ea64..9f4b7958921 100644 --- a/code/src/java/pcgen/cdom/enumeration/Region.java +++ b/code/src/java/pcgen/cdom/enumeration/Region.java @@ -99,13 +99,7 @@ public int getOrdinal() public static Region getConstant(String name) { initializeTypeMap(); - Region region = typeMap.get(name); - if (region == null) - { - region = new Region(name); - typeMap.put(name, region); - } - return region; + return typeMap.computeIfAbsent(name, k -> new Region(name)); } /** diff --git a/code/src/java/pcgen/cdom/enumeration/Type.java b/code/src/java/pcgen/cdom/enumeration/Type.java index 31c50e48a08..7ea93eccce6 100644 --- a/code/src/java/pcgen/cdom/enumeration/Type.java +++ b/code/src/java/pcgen/cdom/enumeration/Type.java @@ -172,13 +172,7 @@ public String getComparisonString() */ public static Type getConstant(String name) { - Type type = TYPE_MAP.get(name); - if (type == null) - { - type = new Type(name); - TYPE_MAP.put(name, type); - } - return type; + return TYPE_MAP.computeIfAbsent(name, k -> new Type(name)); } /** From 4c1c74e7d191dab0bba3ae8ed2a7a6b1d47dd1c8 Mon Sep 17 00:00:00 2001 From: Vest Date: Fri, 26 Jun 2026 12:46:58 +0200 Subject: [PATCH 2/2] Type/Region/DisplayLocation: pin intern contract with unit tests The three getConstant factories rely on a singleton-by-name intern contract: getConstant("X") and getConstant("X") must return the same instance, getConstant("X") and getConstant("x") must return the same instance (CaseInsensitiveMap-backed), and getConstant("X") / getConstant("Y") must return distinct instances. This contract was load-bearing across the codebase (Type as HashSet/HashMap key in DataSet/GameMode/etc., Region as HashMap key in BioSet) but had zero direct test coverage until now: - TypeTest had one incidental test that didn't assert intern identity. - RegionTest and DisplayLocationTest didn't exist. Adds: - TypeTest: getConstantInternsByName, getConstantInternsCaseInsensitively, getConstantDistinguishesNames. - RegionTest (new file): same three. - DisplayLocationTest (new file): same three, plus two for valueOf (returns interned instance; throws on unknown name). Fixture names are namespaced ("TypeTest_*", "RegionTest_*", etc.) so they don't collide with data-load fixtures in other tests sharing the JVM. None of the three classes has a per-test reset hook on master (Type has no clearConstants at all; Region/DisplayLocation have one but no test currently calls it). Revert-and-rerun verified: temporarily replacing Type.getConstant with "return new Type(name);" caused getConstantInternsByName and getConstantInternsCaseInsensitively to fail (as designed), while the pre-existing testSortable kept passing -- confirming the pre-existing test was not exercising the intern invariant. --- .../cdom/enumeration/DisplayLocationTest.java | 75 +++++++++++++++++++ .../pcgen/cdom/enumeration/RegionTest.java | 54 +++++++++++++ .../test/pcgen/cdom/enumeration/TypeTest.java | 28 ++++++- 3 files changed, 156 insertions(+), 1 deletion(-) create mode 100644 code/src/test/pcgen/cdom/enumeration/DisplayLocationTest.java create mode 100644 code/src/test/pcgen/cdom/enumeration/RegionTest.java diff --git a/code/src/test/pcgen/cdom/enumeration/DisplayLocationTest.java b/code/src/test/pcgen/cdom/enumeration/DisplayLocationTest.java new file mode 100644 index 00000000000..e92edcea922 --- /dev/null +++ b/code/src/test/pcgen/cdom/enumeration/DisplayLocationTest.java @@ -0,0 +1,75 @@ +/* + * Copyright 2026 (C) Vest + * + * This library is free software; you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation; either version 2.1 of the License, or (at your option) + * any later version. + * + * This library is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more + * details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this library; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + */ +package pcgen.cdom.enumeration; + +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.junit.jupiter.api.Test; + +/** + * Pins the intern contract of {@link DisplayLocation#getConstant(String)} and + * the strict lookup of {@link DisplayLocation#valueOf(String)}. + * + * Fixture names are namespaced ("DisplayLocationTest_*") to avoid colliding + * with data-load fixtures registered by other tests sharing the JVM. + */ +class DisplayLocationTest +{ + + /** Two calls with the same name return the same interned instance. */ + @Test + void getConstantInternsByName() + { + assertSame(DisplayLocation.getConstant("DisplayLocationTest_Alpha"), + DisplayLocation.getConstant("DisplayLocationTest_Alpha")); + } + + /** The intern map is case-insensitive (CaseInsensitiveMap). */ + @Test + void getConstantInternsCaseInsensitively() + { + assertSame(DisplayLocation.getConstant("DisplayLocationTest_Beta"), + DisplayLocation.getConstant("displaylocationtest_beta")); + } + + /** Distinct names produce distinct instances. */ + @Test + void getConstantDistinguishesNames() + { + assertNotSame(DisplayLocation.getConstant("DisplayLocationTest_Gamma"), + DisplayLocation.getConstant("DisplayLocationTest_Delta")); + } + + /** valueOf returns the already-interned instance — the sibling method NOT touched in this PR. */ + @Test + void valueOfReturnsInternedInstance() + { + DisplayLocation registered = DisplayLocation.getConstant("DisplayLocationTest_Epsilon"); + assertSame(registered, DisplayLocation.valueOf("DisplayLocationTest_Epsilon")); + } + + /** valueOf throws on an unknown name (its lookup-or-throw contract). */ + @Test + void valueOfThrowsOnUnknownName() + { + assertThrows(IllegalArgumentException.class, + () -> DisplayLocation.valueOf("DisplayLocationTest_NeverRegistered")); + } +} diff --git a/code/src/test/pcgen/cdom/enumeration/RegionTest.java b/code/src/test/pcgen/cdom/enumeration/RegionTest.java new file mode 100644 index 00000000000..9870654622e --- /dev/null +++ b/code/src/test/pcgen/cdom/enumeration/RegionTest.java @@ -0,0 +1,54 @@ +/* + * Copyright 2026 (C) Vest + * + * This library is free software; you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation; either version 2.1 of the License, or (at your option) + * any later version. + * + * This library is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more + * details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this library; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + */ +package pcgen.cdom.enumeration; + +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertSame; + +import org.junit.jupiter.api.Test; + +/** + * Pins the intern contract of {@link Region#getConstant(String)}. + * + * Fixture names are namespaced ("RegionTest_*") to avoid colliding with + * data-load fixtures registered by other tests sharing the JVM. + */ +class RegionTest +{ + + /** Two calls with the same name return the same interned instance. */ + @Test + void getConstantInternsByName() + { + assertSame(Region.getConstant("RegionTest_Alpha"), Region.getConstant("RegionTest_Alpha")); + } + + /** The intern map is case-insensitive (CaseInsensitiveMap). */ + @Test + void getConstantInternsCaseInsensitively() + { + assertSame(Region.getConstant("RegionTest_Beta"), Region.getConstant("regiontest_beta")); + } + + /** Distinct names produce distinct instances. */ + @Test + void getConstantDistinguishesNames() + { + assertNotSame(Region.getConstant("RegionTest_Gamma"), Region.getConstant("RegionTest_Delta")); + } +} diff --git a/code/src/test/pcgen/cdom/enumeration/TypeTest.java b/code/src/test/pcgen/cdom/enumeration/TypeTest.java index 9f648e4d32e..2698596152a 100644 --- a/code/src/test/pcgen/cdom/enumeration/TypeTest.java +++ b/code/src/test/pcgen/cdom/enumeration/TypeTest.java @@ -18,6 +18,8 @@ package pcgen.cdom.enumeration; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertSame; import java.util.Collection; import java.util.HashSet; @@ -28,12 +30,15 @@ /** * The Class {@code TypeTest} tests that the Type * class is functioning correctly. + * + * Fixture names are namespaced ("TypeTest_*") so they don't collide with + * data-load fixtures registered by other tests sharing the JVM. */ class TypeTest { /** * Test whether type can be sorted, by adding it to a hashset. - * Added to check fix on Bug with tracker nr. 2413116 + * Added to check fix on Bug with tracker nr. 2413116 */ @Test void testSortable() @@ -45,4 +50,25 @@ void testSortable() }, "type can't be sorted by adding to hashset") ; } + /** Two calls with the same name return the same interned instance. */ + @Test + void getConstantInternsByName() + { + assertSame(Type.getConstant("TypeTest_Alpha"), Type.getConstant("TypeTest_Alpha")); + } + + /** The intern map is case-insensitive (CaseInsensitiveMap). */ + @Test + void getConstantInternsCaseInsensitively() + { + assertSame(Type.getConstant("TypeTest_Beta"), Type.getConstant("typetest_beta")); + } + + /** Distinct names produce distinct instances. */ + @Test + void getConstantDistinguishesNames() + { + assertNotSame(Type.getConstant("TypeTest_Gamma"), Type.getConstant("TypeTest_Delta")); + } + }