Make visitedWithNS and visitedWithoutNS not static

[LonelyMidoriya]

Summary by CodeRabbit

• Refactor
  • Internal architectural improvements to structure element processing for enhanced code maintainability and flexibility.

[coderabbitai]

📝 Walkthrough

Walkthrough

TaggedPDFHelper is refactored from a static utility class to an instance-based class. Its constructor changes from private to public, static fields become instance fields, and static methods become instance methods. PDStructElem is updated to instantiate TaggedPDFHelper instead of invoking static methods.

Changes

Cohort / File(s)	Summary
Static-to-Instance Pattern Conversion src/main/java/org/verapdf/tools/TaggedPDFHelper.java	Converted visitedWithNS and visitedWithoutNS from static to instance fields. Made constructor public. Changed getDefaultStructureType(), getRoleMapToSameNamespaceTag(), isCircularMappingExist(), addVisited(), and isVisited() from static to instance methods with per-instance state management.
Usage Update src/main/java/org/verapdf/pd/structure/PDStructElem.java	Updated two call sites to instantiate TaggedPDFHelper with new TaggedPDFHelper() and invoke methods on the instance instead of calling static methods.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 With whiskers twitching and a curious hop,
I've watched the static methods transform and pop!
From shared state to instances so clean,
Each helper now keeps state between,
No more tangled threads—just clarity in spring! 🌱

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the primary change: converting static fields visitedWithNS and visitedWithoutNS to instance fields, which is the core structural change in the pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch tagged

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (3)

src/main/java/org/verapdf/tools/TaggedPDFHelper.java (2)

171-173: Stale constructor comment.

The // disable default constructor comment no longer matches reality — the constructor is now public and the class is explicitly instantiated by callers. Please update or remove the comment to avoid misleading future readers.

✏️ Proposed tweak

-	public TaggedPDFHelper() {
-		// disable default constructor
-	}
+	public TaggedPDFHelper() {
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/verapdf/tools/TaggedPDFHelper.java` around lines 171 - 173,
The constructor comment for TaggedPDFHelper is stale: update or remove the
misleading "// disable default constructor" comment in the public
TaggedPDFHelper() constructor so it reflects reality (either remove it entirely
or replace it with a short comment such as "public no-arg constructor used by
callers"); ensure the change is made where the TaggedPDFHelper() constructor is
declared.

---

179-180: visitedWithNS.clear() / visitedWithoutNS.clear() are redundant given the new call pattern, but intentional for reuse-safety — consider documenting.

With the new new TaggedPDFHelper() pattern at each call site in PDStructElem, the explicit .clear() calls at the top of getDefaultStructureType, getRoleMapToSameNamespaceTag, and isCircularMappingExist are no-ops on a freshly constructed instance. They are still useful if a caller ever reuses a single TaggedPDFHelper instance across multiple invocations, so keeping them is defensible — just worth a brief comment so nobody later "optimizes" them away and re-introduces the cross-call state leak this PR is fixing.

Alternatively, if reuse is not a supported pattern, consider tightening the contract: make the three traversal methods package-private and expose only static facades (TaggedPDFHelper.getDefaultStructureType(...) etc.) that internally do new TaggedPDFHelper().<method>(type). That keeps the public API static/stateless (as it was) while preserving per-call state isolation.

Also applies to: 200-201, 231-232

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/verapdf/tools/TaggedPDFHelper.java` around lines 179 - 180,
The explicit visitedWithNS.clear() and visitedWithoutNS.clear() calls in
TaggedPDFHelper (seen at the start of getDefaultStructureType,
getRoleMapToSameNamespaceTag, and isCircularMappingExist) look redundant given
new per-call new TaggedPDFHelper() usage but are intentionally defensive for
reuse-safety; either add a short comment above those clear() calls explaining
they are deliberate to avoid cross-call state if an instance is ever reused, or
tighten the API by making those three methods package-private and add static
facade methods (e.g., public static getDefaultStructureType(...),
getRoleMapToSameNamespaceTag(...), isCircularMappingExist(...)) that internally
do new TaggedPDFHelper().<method>(...) so per-call state isolation is
guaranteed—pick one approach and apply consistently to the three referenced
methods.

src/main/java/org/verapdf/pd/structure/PDStructElem.java (1)

118-124: Call-site update looks correct.

Instantiating a fresh TaggedPDFHelper per call correctly isolates the previously-shared visitedWithNS / visitedWithoutNS state across concurrent or recursive role-map resolutions, which is the point of this PR. The extra HashMap/HashSet allocation per call is negligible for role-map resolution and not on a hot path.

One small polish opportunity: both sites repeat new TaggedPDFHelper().<method>(...). If more call sites get added, consider pushing the new TaggedPDFHelper() inside TaggedPDFHelper itself as static facade methods (e.g. TaggedPDFHelper.resolveDefaultStructureType(type) / resolveRoleMapToSameNamespaceTag(type)) so callers don't need to know about the instance lifecycle. Optional, not required for this PR.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/verapdf/pd/structure/PDStructElem.java` around lines 118 -
124, Both call sites repeatedly create a new TaggedPDFHelper instance (in
PDStructElem.getDefaultStructureType and
PDStructElem.getRoleMapToSameNamespaceTag) which is fine but can be polished:
add static facade methods on TaggedPDFHelper (e.g.
TaggedPDFHelper.resolveDefaultStructureType(StructureType) and
TaggedPDFHelper.resolveRoleMapToSameNamespaceTag(StructureType)) that internally
instantiate a TaggedPDFHelper and delegate to the instance methods, then update
PDStructElem.getDefaultStructureType and getRoleMapToSameNamespaceTag to call
those new static methods to centralize lifecycle and simplify callers.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/main/java/org/verapdf/pd/structure/PDStructElem.java`:
- Around line 118-124: Both call sites repeatedly create a new TaggedPDFHelper
instance (in PDStructElem.getDefaultStructureType and
PDStructElem.getRoleMapToSameNamespaceTag) which is fine but can be polished:
add static facade methods on TaggedPDFHelper (e.g.
TaggedPDFHelper.resolveDefaultStructureType(StructureType) and
TaggedPDFHelper.resolveRoleMapToSameNamespaceTag(StructureType)) that internally
instantiate a TaggedPDFHelper and delegate to the instance methods, then update
PDStructElem.getDefaultStructureType and getRoleMapToSameNamespaceTag to call
those new static methods to centralize lifecycle and simplify callers.

In `@src/main/java/org/verapdf/tools/TaggedPDFHelper.java`:
- Around line 171-173: The constructor comment for TaggedPDFHelper is stale:
update or remove the misleading "// disable default constructor" comment in the
public TaggedPDFHelper() constructor so it reflects reality (either remove it
entirely or replace it with a short comment such as "public no-arg constructor
used by callers"); ensure the change is made where the TaggedPDFHelper()
constructor is declared.
- Around line 179-180: The explicit visitedWithNS.clear() and
visitedWithoutNS.clear() calls in TaggedPDFHelper (seen at the start of
getDefaultStructureType, getRoleMapToSameNamespaceTag, and
isCircularMappingExist) look redundant given new per-call new TaggedPDFHelper()
usage but are intentionally defensive for reuse-safety; either add a short
comment above those clear() calls explaining they are deliberate to avoid
cross-call state if an instance is ever reused, or tighten the API by making
those three methods package-private and add static facade methods (e.g., public
static getDefaultStructureType(...), getRoleMapToSameNamespaceTag(...),
isCircularMappingExist(...)) that internally do new
TaggedPDFHelper().<method>(...) so per-call state isolation is guaranteed—pick
one approach and apply consistently to the three referenced methods.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6d41aac6-7598-43c1-8afe-cc5c61c87275

📥 Commits

Reviewing files that changed from the base of the PR and between 3bc0c31 and 8563c52.

📒 Files selected for processing (2)

• src/main/java/org/verapdf/pd/structure/PDStructElem.java
• src/main/java/org/verapdf/tools/TaggedPDFHelper.java